Add YouTube style keyboard shortcuts

[hicom150]

refs: #1395

Left/Right arrows - seek back/forward 5 seconds
Up/Down arrows - volume up/down 5%
j or l - seek back/forward 10 seconds
k - play/pause
Shift + > - Increase speed
Shift + < - Decrease speed

[sunshinecool]

Hi! Is this going to get merged anytime soon?

[Borewit]

The YouTube keyboards are looking good to me.

Great idea to add Linux integration tests, but please be so kind to move those in a different PR.

I did test it under Linux and it all does what it supposed to do, however due to small nasty differences lay-out delta's the unit tests are not passing.

[sunshinecool]

@Borewit sure, I will try to add a few tests for this change.

[Borewit]

So please get rid of c246850 & fd93741 in this PR.

[hicom150]

@Borewit I think I have successfully removed the requested commits. Sorry I'm still a newbie 😅

[Borewit]

Thanks @sunshinecool, you doing just fine.

[Borewit]

I would like to go through the changes one more time, no changes rqeuired on PR.

[mathiasvr]

As I mentioned in the issue, I don't really like to have multiple shortcuts for the same actions.
I'm also not a fan of using two different key handlers (electron menu accelerators, and addEventListener).
I find it confusing that Cmd+Alt+Right skips 10s and a single Right skips 5s, and would they ever overlap?

I think we should just update the accelerators for seeking and volume control, and keep the rest.

Thoughts?

[Borewit]

I agree with @mathiasvr for consistency in the code it would be better to have one way to assign keys.

I have in principle no problem with multiple assignents mapped to the same action, as long there is a justification for the existence of each of them. To be compliant with YouTube layout is in my opinion a good justification, but it is indeed a fair question, do we still need the other one?

If you move your shortcuts to the accelerators, please mark them with comment so it is clear they are part of the YouTube like layout key-layout.

Happy with that consession @mathiasvr?

To have all the shortcuts documented somehere would be awesome.

Just a small advice on your next PR, for mostly your own benefit, don't commit changes to your master branch. Keep that one reserver for incoming changes from the original master. Some more info on this kind of conventions can be found here.

Will you create another PR with the work you did on the Linux integration tests?

[mathiasvr]

@Borewit The problem is that the menu accelerators don't support multiple shortcuts, so if we want to support different shortcuts (at the same time), we probably have to do it this way.

I don't know how we are going to decide this though?

[Borewit]

By being pragmatic.

I believe to support the YouTube shortcuts is desired according to @feross the way I read it in the underlying issue. This issue has been marked as a good first contribution, someone took the challenge, I think we should approve or give guidence how he can succeed on his objective.

If it doesn’t fit in the menu accelerators, it makes sense to have the mechanism defined elsewhere. It should not conflict indeed, if so that needs to be addressed.

[Borewit]

Why keyup and not keydown?

[mathiasvr]

I guess I will let @hicom150 decide then.

You can update the menu accelerators for seeking and volume control, I think that's the best solution.

Or if you think we should go with the current solution, I would request the following changes:

• use keydown instead of keyup, i think instant feedback and repeated events is an improvement.
• use window.addEventListener instead of document.addEventListener.
• use e.key instead e.code, currently this PR only works with qwerty keyboard layouts.

[Borewit]

Superseded by #1395 due the fact the underlying repo holding the PR branch is missing.