Global shortcut for play/pause action using spacebar key
ClosedPublic

Authored by ashwind on Sat, Jan 5, 5:58 PM.

Details

Summary

Assign SpaceBar key as the global shortcut for play/pause action. Most of the media player out there already use spacebar as the global shortcut for play/pause action. So, it would be good to do the same for Elisa.
Modified ElisaMainWindow.qml to add the shortcut component.
Modified MediaPlayerControl.qml to prevent elements from getting focus as it causes interference with the shortcut keypressed events.

BUG: 392989
FIXED-IN: elisa 0.3.80

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ashwind requested review of this revision.Sat, Jan 5, 5:58 PM
ashwind created this revision.
ngraham added a subscriber: ngraham.

Thanks for the patch! There are a lot of unintentional trailing whitespace changes here. Do you think you can clean those up?

ashwind updated this revision to Diff 48774.Sun, Jan 6, 1:15 AM

Well I cleared up the trailing whitespaces.

ngraham accepted this revision.Sun, Jan 6, 3:54 AM

Thanks!

This works great, I love it. It's up to the Elisa devs of course, but this gets a thumbs up from me.

This revision is now accepted and ready to land.Sun, Jan 6, 3:54 AM

Thank you Sir 😊

mgallien requested changes to this revision.Thu, Jan 10, 9:40 PM
mgallien added a subscriber: mgallien.

Thanks for your patch.

Could you please rebase it on top of your previous patch or use arcanist (https://secure.phabricator.com/book/phabricator/article/arcanist/) to submit them ?

The current patch did not apply correctly when trying to do so with "arc patch D17994" command.

Can you also add the ability to configure it like you did in D18017 ?

This revision now requires changes to proceed.Thu, Jan 10, 9:40 PM

Ok, I will look into it.

ashwind updated this revision to Diff 49215.Fri, Jan 11, 8:20 AM

Made the shortcut configurable. I actually did a "git pull" and updated the feature out from a fresh repo. It should work now.

I used archanist to apply the patch. It is working fine for me.

shubham added a subscriber: shubham.EditedFri, Jan 11, 8:57 AM

I used archanist to apply the patch. It is working fine for me.

You should probably also update the diff using it, because it has context. Otherwise you can do git diff -U9999 > foo.diff to make a diff file with context in it.

Ok i will start using archanist ASAP

ashwind updated this revision to Diff 49220.Fri, Jan 11, 9:20 AM

Added context to the diff. Thanks @shubham.

shubham removed a subscriber: shubham.Fri, Jan 11, 9:24 AM

I notice that after navigating to an element using the tab key, hitting the spacebar does correctly trigger that element rather than doing play/pause, but then the just-triggered element is no longer selected afterwards. We should probably preserve that.

maybe i need to do the job using event filter instead of the usual shortcut mechanism used by Elisa

@ngraham can you please specify on which element did you navigate to. I tried to replicate it, it does as you mentioned, on the navigation action bar. But not on the elements outside it. And the purpose of the patch was to perform playpause action irrespective of focus. So probably blocking all the tab events of navigation action bar should do the job.

For example the mute/unmute button.

We don't want to break keyboard navigation via tab though. Perhaps when tab-navigating, items should be triggered with the return key?

Essentially, here's when I had in mind:

  • When using the mouse, items become focused and unfocused, but the space bar should always play/pause
  • When navigating the app using the keyboard via the tab key, you should still be able to trigger the currently focused item in some manner

In other words, Elisa should make a distinction between items deliberately focused using the tab key and items focused "by accident" just in the course of using the app with the mouse.

Does that make sense?

Yes, but elisa doesn't click the elements under focus even through the return key.

So maybe it should, or maybe using the space bar should continue to click the selected element when that element was selected using the tab key.

Ok lets look into it. It might be done through an ugly hack using event filters, but i have to research up a bit.

ashwind updated this revision to Diff 49327.Sat, Jan 12, 1:52 PM

I have updated the diff. Elements focused using tab key will be triggered by Return Key. SpaceBar is assigned global shortcut for play-pause action and won't trigger elements focussed using tab key. Should work for most part.

shubham added inline comments.Mon, Jan 14, 12:55 PM
src/elisaapplication.cpp
422

Inside curly braces

433

Move this curly brace to previous line

436

Extra empty line

ngraham accepted this revision.Mon, Jan 14, 3:49 PM

Nice!

mgallien requested changes to this revision.Mon, Jan 14, 4:22 PM
mgallien added a subscriber: shubham.

Thanks for your work. It works really well.

I have one question. Do you have an idea why the return key needs a special handling whereas QWidget based application seem to work fine ?

I have one more issue. It could be really nice if you could deactivate the event filter when the shortcut is assigned something different from space key.

Please fix also the issues reported by @shubham .

This revision now requires changes to proceed.Mon, Jan 14, 4:22 PM

Ok i will do as you have mentioned. I had to handle return keys separately as they dont trigger actions.

I had to handle return keys separately as they dont trigger actions.

Maybe this is something we should fix at the QQC2-desktop-style level so all QQC/QML/Kirigami apps can get it too?

maybe then we have to take a peek at it.

ashwind updated this revision to Diff 49531.Tue, Jan 15, 2:14 PM
ashwind edited the summary of this revision. (Show Details)
deactivate event filter when SPACE is not assigned as shortcut

I had to handle return keys separately as they dont trigger actions.

Maybe this is something we should fix at the QQC2-desktop-style level so all QQC/QML/Kirigami apps can get it too?

I will look into that but do not want to block this review because of that.

mgallien accepted this revision.Tue, Jan 15, 4:21 PM

Thanks for your work.

This revision is now accepted and ready to land.Tue, Jan 15, 4:21 PM
This revision was automatically updated to reflect the committed changes.