Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add YouTube style hotkeys. Closes #1395 #1579

Merged
merged 2 commits into from
May 6, 2019

Conversation

dsernst
Copy link
Contributor

@dsernst dsernst commented May 5, 2019

This is a continuation of PR #1443, which appears abandoned.

Props to @hicom150 for starting that. I would have added commits on top of that, to preserve git history etc, but the underlying branch looks deleted 😕

This PR takes into account the code changes requested by @mathiasvr in #1443 (comment)

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.

All of these have been included.

👍 This PR has been tested to work locally on macOS 10.14.4.


This closes the original feature request by @feross in #1395.

@dsernst
Copy link
Contributor Author

dsernst commented May 5, 2019

There was also discussion in that PR about taking a different approach: replacing the existing hotkeys (in Menu > Playback) instead of adding these new ones

image

So I also made a branch to take this other approach:

master...dsernst:playback-hotkeys

image

👍 This commit was tested to work locally on macOS 10.14.4.


Happy to send this in as a PR instead.
I have no strong preference either way — I only want to use the new hotkeys.

Hope this won't create a bikeshed... 🙃

Copy link
Member

@Borewit Borewit left a comment

Choose a reason for hiding this comment

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

Thanks a lot for picking up this effort and the hard work @dsernst!

I think it is about time to deliver this YouTube shortcuts to the user.

I don't use the shortcuts to ofton, so I am not the best user to test, but it looks good to me, shortcuts seem to work.

Copy link
Contributor

@mathiasvr mathiasvr left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up and addressing all my annoying comments! 😄

I still prefer the solution on your second branch with youtube style shortcuts only.
I think the menu looks cleaner and allows new users to quickly learn the IMO preferred shortcuts.

However, to avoid more stalling I will accept this and let another person decide whether to merge this or the other (better) option. 😏

Copy link
Member

@alxhotel alxhotel left a comment

Choose a reason for hiding this comment

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

Looks great. I'm just missing the key F to go fullscreen :)

@dsernst
Copy link
Contributor Author

dsernst commented May 6, 2019

@alxhotel:
Looks great. I'm just missing the key F to go fullscreen :)

Added (07816f7). Works on local macOS 10.14.4 testing.

@dsernst
Copy link
Contributor Author

dsernst commented May 6, 2019

@mathiasvr:

Thanks for picking this up and addressing all my annoying comments! 😄

I still prefer the solution on your second branch with youtube style shortcuts only.
I think the menu looks cleaner and allows new users to quickly learn the IMO preferred shortcuts.

However, to avoid more stalling I will accept this and let another person decide whether to merge this or the other (better) option. 😏

Yes, I can see pros & cons each way.

My recommendation for productive forward momentum (similar to your PR approval) is to merge these in as additional hotkeys for now, so they can immediately start being used, albeit undocumented.

And then I will make a new PR (like master...dsernst:playback-hotkeys), to sort out whether to remove the old hotkeys and show these new ones in the Menus. That seems like a bit more of a drawn out conversation, but doesn't need to block getting these new ones in.

@Borewit
Copy link
Member

Borewit commented May 6, 2019

Let's indeed keep the ball roling guys.

@Borewit Borewit merged commit 8b0b5f3 into webtorrent:master May 6, 2019
dsernst added a commit to dsernst/webtorrent-desktop that referenced this pull request May 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
@dsernst dsernst deleted the playback-hotkeys-hicom150 branch July 31, 2020 02:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants