Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ Download the [latest desktop client](https://github.com/jeffvli/feishin/releases

If you're using a device running macOS 12 (Monterey) or higher, [check here](https://github.com/jeffvli/feishin/issues/104#issuecomment-1553914730) for instructions on how to remove the app from quarantine.

For media keys to work, you will be prompted to allow Feishin to be a Trusted Accessibility Client. After allowing, you will need to restart Feishin for the privacy settings to take effect.

#### Linux Notes

We provide a small install script to download the latest `.AppImage`, make it executable, and also download the icons required by Desktop Environments. Finally, it generates a `.desktop` file to add Feishin to your Application Launcher.
Expand Down
23 changes: 2 additions & 21 deletions src/main/features/core/player/media-keys.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,11 @@
import { BrowserWindow, globalShortcut, systemPreferences } from 'electron';
import { BrowserWindow, globalShortcut } from 'electron';

import { isLinux, isMacOS } from '../../../utils';
import { isLinux } from '../../../utils';
import { store } from '../settings';

import { PlayerType } from '/@/shared/types/types';

export const enableMediaKeys = (window: BrowserWindow | null) => {
if (isMacOS()) {
const shouldPrompt = store.get('should_prompt_accessibility', true) as boolean;
const shownWarning = store.get('shown_accessibility_warning', false) as boolean;
const trusted = systemPreferences.isTrustedAccessibilityClient(shouldPrompt);

if (shouldPrompt) {
store.set('should_prompt_accessibility', false);
}

if (!trusted && !shownWarning) {
window?.webContents.send('toast-from-main', {
message:
'Feishin is not a trusted accessibility client. Media keys will not work until this setting is changed',
type: 'warning',
});
store.set('shown_accessibility_warning', true);
}
}

Comment on lines -9 to -27

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it some more, macOS should probably remain with mediasession disabled by default because the user explicitly needs to accept the prompt for trusted accessibility before it can even work.

If the setting is enabled by default, then it would be confusing because the toggle would show that it's enabled, when in reality it's being blocked by the OS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me explain, app should not bind to keys and in case of macos with media session enabled it won't bind based on condition on line 12 same as no other mac app do. When you enable mediasession you give mac native "now playing" functionality control over playback so media keys would work same as headset keys do. Accessibility feature will not be called once and app will function just fine.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessibility feature will not be called once and app will function just fine.

I don't understand this? From my understanding, macOS blocks all media keys from the app unless explicitly allowed by the user under accessibility.

The reason to keep it disabled by default behind the user toggle is so that the user knows and understands why their media hotkeys are not working.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this? From my understanding, macOS blocks all media keys from the app unless explicitly allowed by the user under accessibility.

This is correct. There should be a way to prompt the user for this though. A lot of apps prompt the user when the app is launched, but Feishin does not trigger this prompt.

I think having this option set to enabled by default is good, because this is what I'd expect from a Music player on macOS, BUT someone needs to figure out how to prompt the user.

Also in the past I had issues with the accessibility permissions after switching builds of Feishin and macOS has difficulty detecting this sometimes. But maybe that was just me and it actually works fine anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't have to prompt user if you don't bind media playback keys. you don't have to bind keys since OS will handle them on it's own same to other music players. If we agree that enabled by default is good on macos I see no point to have option to disable it since it makes things worse: you have to bind to media keys and give app accessibility permissions which it doesn't need and lose convenience of headset media control

const enableMediaSession = store.get('mediaSession', false) as boolean;
const playbackType = store.get('playbackType', PlayerType.WEB) as PlayerType;

Expand Down
10 changes: 4 additions & 6 deletions src/main/features/core/settings/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import { app, dialog, ipcMain, nativeTheme, OpenDialogOptions, safeStorage } fro
import Store from 'electron-store';
import path from 'path';

const getFrame = () => {
const isWindows = process.platform === 'win32';
const isMacOS = process.platform === 'darwin';
const isWindows = process.platform === 'win32';
const isMacOS = process.platform === 'darwin';

const getFrame = () => {
if (isWindows) {
return 'windows';
}
Expand Down Expand Up @@ -36,10 +36,8 @@ export const store = new Store<any>({
enableNeteaseTranslation: false,
global_media_hotkeys: true,
lyrics: ['NetEase', 'lrclib.net'],
mediaSession: false,
mediaSession: isMacOS,
playbackType: 'web',
should_prompt_accessibility: true,
shown_accessibility_warning: false,
window_enable_tray: true,
window_exit_to_tray: false,
window_minimize_to_tray: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { Switch } from '/@/shared/components/switch/switch';
import { PlayerType } from '/@/shared/types/types';

const isLinux = isElectron() ? window.api.utils.isLinux() : false;
const isWindows = isElectron() ? window.api.utils.isWindows() : false;
const isDesktop = isElectron();
const localSettings = isElectron() ? window.api.localSettings : null;

Expand Down Expand Up @@ -56,7 +57,7 @@ export const MediaSessionSettings = memo(() => {
context: 'description',
postProcess: 'sentenceCase',
}),
isHidden: isLinux || !isDesktop,
isHidden: !isWindows,
note: t('common.restartRequired', { postProcess: 'sentenceCase' }),
title: t('setting.mediaSession', { postProcess: 'sentenceCase' }),
},
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/store/settings.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1603,7 +1603,7 @@ const initialState: SettingsState = {
audioDeviceId: undefined,
audioFadeOnStatusChange: true,
filters: [],
mediaSession: false,
mediaSession: utils?.isMacOS() ?? false,
mpvAudioDeviceId: undefined,
mpvExtraParameters: [],
mpvProperties: {
Expand Down