streamlink / streamlink-twitch-gui

A multi platform Twitch.tv browser for Streamlink
https://streamlink.github.io/streamlink-twitch-gui/
MIT License
2.65k stars 200 forks source link

Enable app native key binding for router component refresh on macOS #985

Closed templateK closed 10 months ago

templateK commented 10 months ago

Checklist

Description

Currently, refresh rebinding on macOS must set through system settings. One downside of this is that users cannot set it with single character such as ascii a. The system shortcut setting requires at least one additional modifier when using ascii.

I've looked around the master branch and found this. https://github.com/streamlink/streamlink-twitch-gui/blob/2c2fc161132bbc8ca4f821595b2b536d3bfb2ca4/src/app/ui/components/main-menu/component.js#L33-L37

Is there any reason why app native key binding is blocked on macOS? Once I've commented out line 35, the refresh works without any problem in my usage. Also, I still could rebind through system settings after that change.

bastimeyer commented 10 months ago

Is there any reason why app native key binding is blocked on macOS?

I had a quick look and it seems like it's a remnant of the previous implementation, where hotkeys were not customizable.

Customizable hotkeys were added in aaf60cd96aa6e6083c7dc67a7b63eb4b3bf89f92.

If you take a look at the git-blame of the MainMenuComponent from the commit prior to that, you can see that the refresh callback was unchanged and just got moved when the customizable hotkeys were implemented:

The if ( isDarwin ) { return; } block was added to the MainMenuComponent's refresh callback, because the macOS system menubar always gets added with the static CMD+r hotkey (due to convention), same as the CMD+, for opening the settings menu, so back then it was deemed redundant having the CTRL+r refresh hotkey as well.


Let's remove the isDarwin check.

Since you already had a look at the code, do you want to open a quick PR which removes the check and the comment above? This won't affect any tests, so it's just these two lines. If not, then I can apply the changes myself later.

templateK commented 10 months ago

Thanks for detailed answer. Let me know if there's anything needs to change on pr.