Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Add basic keyboard controls #48

Merged
merged 8 commits into from
Dec 21, 2023
Merged

Add basic keyboard controls #48

merged 8 commits into from
Dec 21, 2023

Conversation

BlobCodes
Copy link
Contributor

@BlobCodes BlobCodes commented Dec 16, 2023

This PR adds keyboard controls on the waveform element, which is focused by default.
This means that one can now simply navigate the song with the keyboard without first tab-ing to the GtkScale.

New keyboard controls:

  • Press space to pause/play
  • Press arrow_left/arrow_right to jump backwards/forwards 10 seconds

These controls have been added to the help page.
Translations for these keys probably need to be added, but I have no idea how to do that.

More precise controls could be added in the future, but I wanted to keep this PR small.

The GtkScale and the three buttons in the lower middle (back 10s, pause, skip 10s) are now not focusable anymore because this should be way more intuitive.

data/gtk/help-overlay.ui Show resolved Hide resolved
data/player.ui Outdated Show resolved Hide resolved
data/player.ui Outdated Show resolved Hide resolved
src/player.ts Outdated Show resolved Hide resolved
@BlobCodes
Copy link
Contributor Author

BlobCodes commented Dec 20, 2023

One possible improvement to the keyboard controls could be to set the "focus-on-click" property to false on the buttons and slider in the bottom.

Currently, when clicking the pause button, it is automatically focussed by gtk.
In this state, the player controls don't work.

Otherwise, the flatpak build with the rebased PR seems to work fine.

Should I first add this improvement or should this PR just stay as-is?

@vixalien
Copy link
Owner

Should I first add this improvement or should this PR just stay as-is?

You can add this improvement. Thanks

build-aux/flatpak/com.vixalien.decibels.json Outdated Show resolved Hide resolved
data/player.ui Outdated Show resolved Hide resolved
@BlobCodes
Copy link
Contributor Author

Setting the "focus-on-click" property did not work, that was not possible.
Now the EventController was simply moved to the root of the player widget.

This means that the left/right arrow keys and space will always be captured inside the player view, inside the main window.
It is still possible to use everything via the keyboard using TAB and RETURN.

@BlobCodes
Copy link
Contributor Author

By the way, what method do you use for merging PRs?

The commits from my previous PRs are all marked as unverified although the commits should be signed.
Also, I think those mistakenly pushed changes are not really useful to have in the repo - maybe squash the PR on merge?

@vixalien
Copy link
Owner

By the way, what method do you use for merging PRs?

I usually revise on merge

The commits from my previous PRs are all marked as unverified although the commits should be signed.

I just saw that too. It's weird because it's the first time it's ever happened. Did you remove your keys or something? If not, GitHub might have messed up (probably a bit flip or something 😅) and I'm not in the mood of fixing that so maybe we might need to ignore it?

Also, I think those mistakenly pushed changes are not really useful to have in the repo - maybe squash the PR on merge?

No worries. I can squash.

Let me know when ready for review

@BlobCodes
Copy link
Contributor Author

Let me know when ready for review

It's ready now

@BlobCodes
Copy link
Contributor Author

Did you remove your keys or something?

I didn't change anything.
The commits from #45 are marked as verified on the branch it was merged from:
https://github.com/BlobCodes/decibels/commits/main/

@BlobCodes
Copy link
Contributor Author

It's weird because it's the first time it's ever happened

I have the paranoid mode activated on github where all unsigned commits associated with me are explicitly specified as unsigned.

All code coming from PRs in the commit history are also unsigned, but nobody associated with the PR had this mode activated.

Using "rebase on merge" (I think this is what you meant, I couldn't find "revise on merge") is generally not compatible with signed commits:
https://stackoverflow.com/questions/62950018/verified-signatures-are-gone-after-i-pressed-rebase-and-merge

@vixalien
Copy link
Owner

vixalien commented Dec 21, 2023 via email

@vixalien
Copy link
Owner

I believe this branch needs a rebase

@BlobCodes
Copy link
Contributor Author

No, there are no new commits that this branch doesn't have.

GitHub even says that there are no conflicts (it can do everything automatically).

Bildschirmfoto vom 2023-12-21 14-25-28

@vixalien
Copy link
Owner

Not sure what's wrong here for me. It shows this

image

@vixalien vixalien merged commit 6a14c06 into vixalien:main Dec 21, 2023
2 checks passed
@vixalien
Copy link
Owner

Ah that's right it can't be rebased but it can be squashed as per your recommendation. Thanks!

@BlobCodes BlobCodes deleted the feature/keyboard-seek branch December 21, 2023 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants