Fix Windows 10 Overlay Next/Prev. Media Key Duplication Issue

[Kytech]

This code change will address the double-skipping bug with the Windows 10 media overlay enabled that is mentioned in several issues.

When the Windows 10 media overlay integration is used, Windows will call the event handler function registered for the media overlay's next/previous buttons whenever any physical hardware next/prev buttons are pressed. I added code to check if overlay integration is turned on. If overlay is turned on, the program will not register a keypress handler using electron's KeyRegisterFn to prevent electron from calling the program's next/prev function in addition to Windows firing it's call to the next/prev function when a media key is pressed. We want only Windows to handle the keypress event with the overlay turned on. If both handle the event, the result is the double-skip bug.

This change resolves the following issues:
Closes #2364
Closes #3293
Closes #3598

This issue deals with the double-skipping bug, but also describes an issue with the play/pause functionality, which is similar to this problem, though it is being caused in a different part of the program. Update: The play/pause issue seems to have been fixed when I pulled in some more recent code from master.

[welcome]

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

• Run npm run lint locally to catch formatting errors earlier.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.
• If you added new translation strings ensure that you have added empty strings for all languages
• If you added new functionality or modified existing functionality please ensure it is tested

[jostrander]

This would indeed fix the double skipping problem on initial startup, but what about if the user unchecks the Windows 10 media button in settings? It would leave them without keyboard shortcuts, right?

[Kytech]

@jostrander If the user unchecks the Windows 10 media buttons in settings, the program will register shortcuts using Electron's key press handler instead of using the overlay to handle them. The if statement on line 15 that wraps the electron key press event handler registration checks to see if the program settings are configured to use the Win10 media keys and if the program is running on Windows 10 specifically. If the Win10 media service is disabled or the user is not on Windows 10, they will still have keyboard shortcuts without the Win10 media keys since it will then make Electron handle the media keys. I ran a test on this myself and verified this. You could also test this if you'd like with the AppVeyor build for this PR to confirm that it is working as intended.

[jostrander]

I'm not using windows at the moment, but I believe you have to restart GPMDP after selecting the media service, correct? If that's the case then the code makes sense.

I was concerned that an 'on-the-fly' change in settings wouldn't be registered because I saw no event listener for that change being modified. However I believe now that the setting is changed, saved, then applied on restart.

[Kytech]

Yes, that is correct. The current behavior for GPMDP's media key settings is that it requires a restart. Since that is the current behavior, I kept it that way. The settings change is applied on restart, as indicated by the settings UI.

[MarshallOfSound]

I've experimented with this before and on some machines (one of mine in particular) the media service does not always emit events when some applications are in focus (games in particular). I'd want to test this extensively again before landing

[Kytech]

I've noticed that the play/pause button has some general issues dispatching events, though that seems to be from a different cause that I plan to investigate a bit myself in another pull request. I originally designed this fix in an older version of GPMDP and it worked perfectly, but after I pulled in the newer code I noticed the play/pause issue. I'm thinking of opening another pull request for that after I have a chance to investigate that issue as well. The skip next/forward seems to be consistent though. Wouldn't inconsistencies with the media service sending events or not be a separate issue? I'm thinking this pull request is to just fix part of the issue since it seems like a bit more of a larger problem.

[xCSxXenon]

Is this just waiting for the play/pause issue to be fixed as well? Really excited to get a fix for the double skip bug!

[Kytech]

@MarshallOfSound I've just updated the code base of my PR to be inline with the 4.7.1 release. After testing once I merged in the updates, The play/pause issue no longer occurs. Track movement and media keys work perfectly, as does the system media transport controls integration when they are fully enabled.

However, I noticed that when I disable the "Show Media Information in System Overlay" option, or disable the system media transport controls option entirely, media keys don't work at all. It sounds like the alternate/original method used to respond to media keys is not re-enabling with the changes I made. This issue sounds a lot like, and could close #3598, though I may have been able to make everything behave more consistently. I suspect the issue here is due to the change in the settings menu from when I originally made this fix, so I probably need to adjust how this fix works with the settings options.

One final note, When I last built this project on my dev machine, I was receiving warnings that the current implementation we are using for integrated system media transport controls (The MediaControl class, which is what BackgroundMediaPlayer uses) is deprecated in favor of using the SystemMediaTransportControls class (Which is used by the new MediaPlayer class that can be configured for background) and that the MediaControl class is not guaranteed to work on versions of Windows beyond 8.1. This may contribute to the reason why it works in many cases, yet doesn't in a fair amount of cases as well. Best option is probably to get this working as a toggle-able option for now and then update the implementation to use SystemMediaTransportControls in a separate pull request. Using the new API could possibly fix the incompatibilities with keyboard emulators due to some changes under the hood.

[Kytech]

@MarshallOfSound, I've just updated my patch to work with the current settings menu configuration. I also noticed that Windows does see a media playback command and a media key press as different events. I noted this when I configured my laptop trackpad (uses the precision drivers and can configure gestures through the Windows settings) to use a three finger side swipe for media playback control options next track and previous track. When I enabled system media transport controls, this worked as expected. However, without them, they did not function. The keybindings as implemented when system media transport controls are off is to look for a specific keyboard keypress of a next or previous key. The default behavior of Windows's media keys when a program is not binding to them is to essentially trigger a media playback command. I noticed the same thing with Windows Media Player when trying to use my trackpad configuration, which was coded before System Media Transport Controls was built as an API for Windows.

I'm thinking that this may be why the system media transport controls may not always work in certain games. They may register hooks to the keypress events, which then prevents that keypress from being forwarded to Windows to generate a system media transport controls event. I could especially see this being the case where the media playback keys are implemented as alternate functions of the Function/F keys on some machines/keyboards, depending on how they implement this behavior and how some games deal with it. I'm planning to test my patch with something like Auto-Hotkey to see how it behaves. It seems like some games likely interfere with anything using the new system media controls, so making this option controllable is still our best option.

I think my main goal for this PR is to get an implementation that is in line with the current "Windows 10" way of doing things so that we're able to handle media playback controls in a way that plays best with the OS without the annoying double-skipping bug that we were experiencing before this small tweak in how media keys are handled with the System Media Controls Setting turned on.

[Kytech]

Another thought as well, @MarshallOfSound, in the settings menu, the "Show Track information in Windows 10 Volume Overlay" settings seems to be redundant since the "Enable the Windows 10 System Media Service" setting essentially has the same effect. The only difference I see is the latter option states that it will only show playback info on the lock screen. However, Windows does not provide a way to do this without also displaying track info in the volume overlay. Should this setting option be removed or is it's intended purpose to do something else? I noted that disabling track info in the volume overlay has the same effect as disabling the Windows System Media Service entirely.

[Kytech]

@MarshallOfSound does the issue with the Windows System Media Transport Controls also occur with programs such as Spotify? I know that Spotify uses a similar integration as the one we have with the Windows System Media Transport Controls, so if the same issue occurs with Spotify and those games you are having issues with GPMDP and the media transport controls, I think we could conclude that the issue is those games instead of our implementation. I also made a few tweaks to the way this fix interfaces with the settings since I found a few unexpected behaviors before I made that change. Perhaps this might help with some of the issues we are experiencing? I also pulled in the latest changes as of late from the time you last looked at this, so perhaps the issue is no longer persisting. Could you take another look at my fixes?

[Kytech]

@MarshallOfSound I do note that you stated in the first issue that I tagged in the original PR that the thought of making the overlay optional, which has been done, would allow us to work around poorly designed programs that interfere with the Windows System Media Transport Controls. I know that Spotify also makes that integration optional as well, so I think we're on the right track concerning having this optional. However, I've noticed from my use of newer hardware that many of the newer hardware's media control features actually dispatch media control actions through the system media transport controls instead of emulating a hardware keypress.

To provide an example, newer windows PCs that support precision trackpads allow for advancing through tracks using gestures on the trackpad. The only way I can get that to work on my surface book 2 (which uses precision trackpad drivers) with GPMDP is to enable the media transport controls. My logitech mouse behaves similarly when I configure it's macros using Logitech's options software. I'm thinking that we should get this fix pushed through as it will appear that media transport controls are entirely broken on newer hardware that dispatches media transport controls through the system media transport controls if it is not fixed. Getting this fix applied should be the most reliable way to have clean media key integration in circumstances for the vast majority of Windows 10 users while the option of switching it off and using global listeners will take care of the other instances where a game may interfere with the functionality. Seeing how Microsoft is pushing for the system media transport controls as the new standard, I think it would be good to incorporate this so that GPMDP plays nice with newer hardware. It seems like we've found a fix that will still have the flexibility to work with all situations by making this a toggle-able option.

I would really like to see this incorporated into the program as I feel like this program has been amazing for my music listening needs and I know a lot of others feel the same way and have experienced the same or similar issues as myself. I know you're also wanting to get a final fix for this issue as well. I too have also seen a few occasional cases where I've had to switch the setting off and on for the system media transport controls, but I get best results that work about 99% of the time for me by using the media transport controls with this fix. I actually get more reliable results on my surface book 2 and even my desktop with the overlay enabled than with it disabled, but the option to switch it off has served as a reliable fix for those cases where I have to work around other programs.

If there are any other cases you would like me to test, I'd be happy to take a look at them. Please let me know what else I can do to get this moved along.