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 missing keyboard shortcuts #2698

Closed
wants to merge 14 commits into from

Conversation

Aiz0
Copy link
Contributor

@Aiz0 Aiz0 commented Oct 10, 2022

Add missing keyboard shortcuts

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

#2138

Description

According to #2138 there seems to be 2 missing shortcuts, 2 shortcuts that are already implemented but users want alternate bindings, and 2 shortcuts don't have any regular key binds, uses Media keys exclusively.

What's implemented

  • Home Jump to beginning of video
  • End Jump to end of video
  • < and > added as alternate shortcuts for controlling video playback rate.
  • Shift + N play next video in playlist or recommendations
  • Shift + P play previous video in playlist.

Shift + P can no longer be used to increase video playback rate

Alternate shortcuts

video playback rate

Should < and > be added as alternate keybinds to speed up / slow down video playback rate?
Implemented

playlist navigation

Youtube supports Shift + N and Shift + P to navigate to next / previous video in playlist.
Freetube already uses P to slow down playback rate.
Should we use some other keys for this?

Implemented

I could also try to add support for the media next playing next suggested video if not playing any playlist.
Implemented

Screenshots

no visible changes.

Testing

Home and end shortcuts

  • Go to a video.
  • Press End to Jump to the end of the video.
  • Press Home to Jump to the beginning of the video.

Playback rate shortcuts

  • Go to a video.
  • < and > to control playback rate

Playlist shortcuts

  • go to playlist video (link)
  • Press Shift + N to play next video in playlist
  • Press Shift + P to play previous video in playlist
    you should also test if autoplay next works as expected.

Next Recommended videos shortcuts

Go to any video with hide recommended videos disabled

  • Press Shift + N to play next recommended video
    you should also test if autoplay next works as expected

Nothing should happen if Hide recommended videos is enabled.

Desktop

  • OS: Arch Linux
  • OS Version: 5.19.13-arch1-1
  • FreeTube version: 0.17.1

Additional context

https://support.google.com/youtube/answer/7631406

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 10, 2022
@Aiz0 Aiz0 changed the title Add missing keyboard shortcuts WIP: Add missing keyboard shortcuts Oct 10, 2022
@efb4f5ff-1298-471a-8973-3d47447115dc

Alternate shortcuts

video playback rate

Should < and > be added as alternate keybinds to speed up / slow down video playback rate?

Yes please, we now have O and P for that but it would be nice if < and > would also do that. This makes it more uniform and familiar with the users of YT.

playlist navigation

Youtube supports Shift + N and Shift + P to navigate to next / previous video in playlist. Freetube already uses P to slow down playback rate. Should we use some other keys for this?

Im not sure if it would really matter because you are pressing SHIFT first. I would like it to behave the same as YT for the same reasons pointed out before.

I could also try to add support for the media next playing next suggested video if not playing any playlist.

That would indeed be nice!

@Aiz0
Copy link
Contributor Author

Aiz0 commented Oct 11, 2022

I've now added the other shortcuts

  • < and > added as alternate shortcuts for controlling video playback rate.

  • Shift + N and Shift + P can now be used navigate to next / previous video in playlist.

  • When not watching a playlist Shift + N will play next recommended video instead.

Shift + P can no longer be used to increase video playback rate

@Aiz0 Aiz0 marked this pull request as ready for review October 11, 2022 02:23
@Aiz0 Aiz0 changed the title WIP: Add missing keyboard shortcuts Add missing keyboard shortcuts Oct 11, 2022
@efb4f5ff-1298-471a-8973-3d47447115dc

Shift + P can no longer be used to increase video playback rate

Shift + P never did that P increased video playback rate. Do you maybe mean P?

@Aiz0
Copy link
Contributor Author

Aiz0 commented Oct 11, 2022

Shift + P never did that P increased video playback rate. Do you maybe mean P?

you could control video playback rate with lowercase and uppercase p
uppercase p is just shift + p or shift + P

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested most except
Home and end shortcuts (MacOS)

@absidue
Copy link
Member

absidue commented Oct 13, 2022

Tested most except Home and end shortcuts (MacOS)

according to this article you can use Fn+Left arrow for Home and Fn+Right arrow for End

@PikachuEXE
Copy link
Collaborator

Still can't get it to work ._.
Might need people with windows to test

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 14, 2022
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Forgot about this somehow.
Prevents a error from appearing in console when using shortcut while not watching a playlist.

I did notice a seperate error appears if on first video.
auto-merge was automatically disabled October 15, 2022 01:34

Head branch was pushed to by a user without write access

Requires you to press Shift when Caps Lock is enabled.

Co-authored-by: ChunkyProgrammer <78101139+ChunkyProgrammer@users.noreply.github.com>
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 22, 2022 15:48
auto-merge was automatically disabled October 22, 2022 21:05

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 22, 2022 21:05
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 22, 2022

Ready for review?

@Aiz0
Copy link
Contributor Author

Aiz0 commented Oct 22, 2022

bug needs fix first.
cant use < > because they use shift to be typed on most layouts

@absidue
Copy link
Member

absidue commented Oct 23, 2022

@Aiz0 You can fix that by not returning at the top when shift is pressed, you only need to check that shift is pressed for specific shortcuts, for others it's fine if shift is pressed or not.

- Alphanumeric keys don't work when shift is held, they still work when capslock is enabled
- By using toLowerCase(), Only one case per key is required instead of one for upper and one for lowecase.
- The check for shift does not simply return anymore. additional switch statements can be added for shifted keys. Having a seperate switch statement is in my opinion better than having checking for shift on each shortcut.
- Keys that require shift to be typed put into seperate switch statement.

lmaooooooooooooooooooooooooooooooooooooooooooooooooooooooo :[ :(
simplefies things and shouldn't cause any issues as far as i'm aware
auto-merge was automatically disabled October 29, 2022 14:49

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 29, 2022 14:50
second switch statement now only includes keys with names longer than a single character
auto-merge was automatically disabled October 29, 2022 15:01

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 29, 2022 15:01
@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Nov 1, 2022
if (event.ctrlKey || document.activeElement.classList.contains('ft-input') || !event.shiftKey) {
return
}
switch (event.key.toUpperCase()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we use switch (event.key.toLowerCase()) { above but toUpperCase here ?_?
No an issue for me though, just curious

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@FreeTubeApp FreeTubeApp deleted a comment from github-actions bot Nov 4, 2022
@Aiz0 Aiz0 closed this Nov 4, 2022
auto-merge was automatically disabled November 4, 2022 21:12

Pull request was closed

@ChunkyProgrammer
Copy link
Member

Did you mean to close this PR? @Aiz0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants