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 common keyboard controls #947

Merged
merged 21 commits into from
Sep 22, 2024
Merged

Conversation

Haaruun-I
Copy link
Contributor

@Haaruun-I Haaruun-I commented Jul 26, 2024

A few issues have been asking for this, and the other PR is incomplete, so I thought I would just do it.

The new keybinds are:

  • Up/Down for volume controls
  • +/- for player speed (and -/= so you dont need to hold shift)
  • the bracket keys [] and {} to skip ahead in chapters, along with Page Up and Down,

The left/right arrow key controls have been moved so focusing on the main page should work to seek. As a side effect of that is the skip/rewind buttons in the playerbar are slightly more responsive

The search page cant type the brackets or plus minus keys without the current keybinds overriding them, but I'm looking for a fix to that now

EDIT: Search works as expected now

@Haaruun-I
Copy link
Contributor Author

It should probably be put on the DEVELOPMENT.md that this project is linted with ruff, didnt realize that until it errored for the first time

@Haaruun-I Haaruun-I marked this pull request as draft July 28, 2024 05:47
@Haaruun-I Haaruun-I marked this pull request as ready for review July 28, 2024 06:29
@geigi
Copy link
Owner

geigi commented Jul 29, 2024

Thanks for working on this :)

@rdbende
Copy link
Collaborator

rdbende commented Aug 10, 2024

There are a couple of issues with the current implementation as I see it. The first is actually an accessibility issue.
Since common keys normally used for keyboard are all bound to a playback control action (except Tab), it's quite hard to navigate the Cozy's UI using the keyboard. This is due to the fact that all shortcuts are bound on the application object, thus they are also active in the preferences window for example. I think since these are not actual key combinations, only hotkeys, they shouldn't be bound on the application level, instead on the main navigation view widget.

My other issue is that due to how the create_action is implemented on the CozyUI class, that same class needs to have all the callback methods, even though they belong to other widgets. I think this issue would be solved though, if those actions were bound on the navigation view class.

@rdbende rdbende linked an issue Aug 10, 2024 that may be closed by this pull request
@rdbende
Copy link
Collaborator

rdbende commented Aug 12, 2024

Since common keys normally used for keyboard are all bound to a playback control action (except Tab), it's quite hard to navigate the Cozy's UI using the keyboard.

It was very inconsistent otherwise, so I left them overridden on the main view, but disable these shortcuts in dialogs.

@rdbende
Copy link
Collaborator

rdbende commented Aug 12, 2024

Now about the actual keyboard shortcuts:
As multiple people mentioned in the issues already, VLC is a good example to follow here, so for example, I would use "=" to reset the speed instead of increasing it.
I'd also like to drop shortcuts that are awkward to use with non-English keyboards, like the bracket (AltGr+F for me for example), and instead add Ctrl+arrows for those who don't have Page Up and Down keys. It's also a good idea to use the keypad buttons, as they are sometimes more accessible.
Here is what I think would work:

  • +/- to increase/decrease playback speed (along with the ones on the keypad)
  • = to reset speed
  • PgUp/PgDown and Ctrl+Left/Right for skipping chapters

In the future I would also explore adding more modifiers to the arrow keys for forward/rewind (Shift+arrow, Alt+arrow), as I think that would be more useful than being able to set a custom interval in the preferences.

Mark the primary menu as primary, so we get the F10 shortcut for free.
Also remove the Enter key shortcut from the BookCard's FlowBoxChild,
since it's no longer activatable, and instead the button inside it
responds to Return
Copy link
Collaborator

@rdbende rdbende left a comment

Choose a reason for hiding this comment

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

I think this is good to go now

@Haaruun-I
Copy link
Contributor Author

I agree with most of the changes made, although I dont think its a good idea to copy VLCs playback speed behavior.
As it is now, you need to hold shift to increase the speed, but cant when decreasing it, which makes it frustrating to adjust it, Keeping the controlls +/= -/_ makes it far less annoying to use those hotkeys.

I agree that a speed_reset button is a good idea, maybe it could be moved to the 0 key?

The keyboard behavior in the rest of the app is very inconsistent and definitely needs some more work though.

@rdbende
Copy link
Collaborator

rdbende commented Aug 14, 2024

I see your point, however, I'd prefer assigning these shortcuts based on semantics, and not on convenience of access.
An annoying shortcut is usually relative. For example, take a look at the German keyboard layout, where neither - and + requires shift:
image

The other example is my keyboard, where the - is down on the bottom, + is Shift+3, = is Shift+7, and zero is on the other end of the keyboard. Let me tell you, it's it's quite annoying to use any keyboard shortcuts on this layout.
image

I'll do some more research to see, how other apps do this, but I'd like to avoid adding shortcuts, that are convenient to use on English, and similar keyboards, but doesn't make sense, and/or are inconvenient on any other layout.

@rdbende
Copy link
Collaborator

rdbende commented Aug 14, 2024

We could assign < and > in addition. How do you feel about that?

@Haaruun-I
Copy link
Contributor Author

How about Shift or Cntrl +Up and Down? The up/down arrow keys are only used for volume now, and they dont move on most keyboard layouts (I hope). Changing the speed doesn't happen often enough that the modifier key is a problem, and atleast its consistent.

@rdbende rdbende merged commit e5190e1 into geigi:master Sep 22, 2024
4 checks passed
@rdbende
Copy link
Collaborator

rdbende commented Sep 22, 2024

Thanks, @Haaruun-I!

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