vixalien / decibels

Play audio files
https://gitlab.gnome.org/vixalien/decibels
GNU General Public License v3.0
52 stars 17 forks source link

Add basic keyboard controls #48

Closed BlobCodes closed 9 months ago

BlobCodes commented 9 months ago

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:

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.

BlobCodes commented 9 months ago

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 commented 9 months ago

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

You can add this improvement. Thanks

BlobCodes commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Let me know when ready for review

It's ready now

BlobCodes commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Ah, I see. I didn't know rebase and merge rewrote the commit SHAs (I know, I'm dumb and I hate git.) Thanks for enlightening me. I will now do the regular merge from now on.

On Thu, 21 Dec 2023, 02:23 David Keller, @.***> wrote:

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 https://github.com/vixalien/decibels/commits/main/ 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

— Reply to this email directly, view it on GitHub https://github.com/vixalien/decibels/pull/48#issuecomment-1865309231, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJB5FCKIYOILKRWCGI4TMM3YKN6QHAVCNFSM6AAAAABAXZLRN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRVGMYDSMRTGE . You are receiving this because you commented.Message ID: @.***>

vixalien commented 9 months ago

I believe this branch needs a rebase

BlobCodes commented 9 months ago

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 commented 9 months ago

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

image

vixalien commented 9 months ago

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