Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2 #13148

cr7pt0gr4ph7 · 2024-04-21T10:42:12Z

Issue Statement

Right now, one cannot easily use a keyboard shortcut to begin editing a track table cell when keyboard shortcuts are enabled, because the usual F2 key is bound to something else by default the default .kbd.cfg files provided.

Use Case

I am currently setting up my music library, and it is quite annoying that I can't use keyboard shortcuts to both play a track in the preview deck using p (which is only possible when keyboard shortcuts are enabled) and then enter edit mode using the keyboard too (the F2 key is only bound to this function when keyboard shortcuts are disabled).

Proposed Solution (as implemented)

Add a keyboard-triggerable EditItem action (name is up for debate, but matches the existing GoToItem action) that can be remapped using Custom.kbd.cfg. The default keyboard mappings bind EditItem to the r key,
basically because it was the only easily-reachable key not bound yet, but due to being able to rebind it myself, I am open for other suggestions. What I considered so far:

Not providing a default mapping: Kinda bad for discoverability of the feature, but would be fine by me
Binding EditItem to Ctrl+Shift+Return: Makes sense from the viewpoint of consistency (because Ctrl+Enter is already bound to opening the track menu, and Enter is interpreted as "edit this shell" e.g. in Excel and Google Sheets.
Binding EditItem to Ctrl+e: Would work, is easily reachable, but incurs the risk of mistiming the Ctrl press and invoking loop_double (which is mapped to e) instead when editing a lot of tracks.
Binding EditItem to r: The key is easily reachable, not mapped to anything important yet, and has the mnemonic of Rename to remember it by.

Further Work

Adapt the keyboard shortcut table at mixxxdj/manual once the default key binding has been agreed upon.
Making the "Star Rating" column editable by keyboard: I'm already working on this, and have a PR nearly ready, but due to the "special" behavior of the star rating column that allows you to edit it simply by hovering over & clicking a star without focusing the cell first, care has to be taken so that the feature works as expected in all circumstances.
Open a context menu providing the option to clear the playing count when attempting to edit the playing count column.
Possibly adding a toggle option (or separate command) that keeps the "editor mode" active not only when navigating horizontally using Tab/Shift+Tab, but also when navigating vertically using the Up and Down arrow keys.

JoergAtGithub · 2024-04-21T10:45:21Z

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

cr7pt0gr4ph7 · 2024-04-21T11:20:59Z

Welcome at Mixxx! As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

Done 👍 (signed under my real name, i.e. Lukas Waslowski)

ronso0

Thank you for working on this!

I left some notes.
Please also check the CI remarks.

ronso0 · 2024-04-22T01:29:24Z

src/widget/wtracktableview.cpp

+void WTrackTableView::editSelectedItem() {
+    if (state() != EditingState) {
+        QKeyEvent keyEvent(QEvent::Type::KeyPress, Qt::Key_F2, Qt::NoModifier);
+        edit(currentIndex(), EditKeyPressed, &keyEvent);


Why not use the simple method edit(index)?

Thank you for the suggestion. I replaced the keyEvent with nullptr in the new (rebased) version of this PR (because I ended up not needing foitr the future follow-up PR related to StarEditor). The EditKeyPressed flag on the other hand is specified for two reasons:

The edit is triggered because of the user pressing the platform edit key (resp. a replacement for that key), so it makes sense to pass that knowledge along using the API provided for this.

I need the flag in the follow-up PR to distinguish certain user-triggered invocations of edit() from those that are triggered internally by QAbstractItemView.

IIUC you don't need it for this use case. The simple implementation with just the index calls the base function with AllEditTriggers
https://github.com/qt/qtbase/blob/e7362764d4931f255d2377462df8ac7a0d4e7c84/src/widgets/itemviews/qabstractitemview.cpp#L1233-L1240
and that starts editing except the trigger is DoubleClicked or SelectedClicked.
https://github.com/qt/qtbase/blob/e7362764d4931f255d2377462df8ac7a0d4e7c84/src/widgets/itemviews/qabstractitemview.cpp#L2749

The EditKey is already enabled here btw, and IIUC that is captured in keyPressEvent and is not relevant for this new slot editSelectedItem.

mixxx/src/widget/wlibrarytableview.cpp

Line 31 in 67a41d9

setEditTriggers(QAbstractItemView::SelectedClicked|QAbstractItemView::EditKeyPressed);

Tl;dr
IMO we don't need the trigger here and I'm curious about your follow-up ; )
Do you mind sharing a link to your WIP branch?

Apart from that this PR looks good to me btw.

src/proto/httpplugin.proto

…ard shortcut The default platform edit key F2 is often already mapped to other commands when keyboard shortcuts are enabled, so we want to be able to specify a custom key instead to allow full control via keyboards.

All other simple single-key shortcuts are actually taken, and mapping the action to e.g. Ctrl+e would incur the risk of accidentally invoking the loop_double action that is mapped to "e" - especially when you are bulk-editing a lot of tracks.

… in the sidebar

cr7pt0gr4ph7 · 2024-04-24T06:30:59Z

I integrated the review suggestions and fixed the build failures (and have setup the CI pipeline on my fork, so the latter shouldn't happen anymore for future PRs). Thanks for the quick reaction!

ronso0 · 2024-04-24T10:06:08Z

src/widget/wlibrarysidebar.cpp

@@ -176,6 +176,20 @@ void WLibrarySidebar::dropEvent(QDropEvent * event) {
    }
 }

+void WLibrarySidebar::editSelectedItem() {


should better be renameItem() since that's all it does (compared to the table view where we can indeed use "edit" because we don't rename the track, just the metadata item)

isExpanded() is not a good way to check for root nodes, because it returns false for root nodes that are not currently expanded. SidebarModel::renameItem will handle root nodes appropriately anyway.

daschuer

The code looks good. I have not tested yet.
@ronso0 does that fit into our keyboard layout?
In window explorer renaming is on F2

ronso0 · 2024-04-26T08:13:17Z

Yep, we only have Ctrl+R for Recording.

The idea is to have an alternative for F2 because it's mapped to rate_temp_up.

ronso0 · 2024-04-26T08:13:39Z

Haven't tested it either, yet, will do so next week.

ronso0 · 2024-04-26T10:58:21Z

IIUC we now need to query the keyboard config via KeyboardEventFilter in order to show the correct shortcut in the sidebar context menu. I.e. show either F2 (keyboard disabled) or R/custom key (keyboard enabled).

This happens here for playlists

mixxx/src/library/trackset/baseplaylistfeature.cpp

Line 77 in 67a41d9

m_pRenamePlaylistAction->setShortcut(kRenameSidebarItemShortcutKey);

and here for crates

mixxx/src/library/trackset/crate/cratefeature.cpp

Line 71 in 67a41d9

m_pRenameCrateAction->setShortcut(kRenameSidebarItemShortcutKey);

An example is in WMainMenubar or LegacySkinParser

Updating the shortcut when toggling the keyboard is a bit more tricky.
Though, I think both is required for this PR : |

Please also check #13082 (not sure if it conflicts)

cr7pt0gr4ph7 · 2024-04-26T11:17:27Z

@ronso0 Thanks for bringing that to my attention! I'll have a look at the displaying the correct keyboard shortcuts, and will try a local merge with #13082. Based on a quick glance at the latter, this doesn't conflict with my pull request as of now, but might interact with the reload logic.

ronso0 · 2024-04-26T11:55:20Z

Just FYI #13082 hasn't been reviewed, yet, so it's unclear if it'll be merged as it is now and when that'll happen of course.
I.e. I don't recommend to make this PR depend on #13082

ronso0 · 2024-04-26T12:59:37Z

I think, with current main, we need to pass a CoreServices (or KeyboardEventFilter) pointer to the library features that need to access the keyboard cfg, and maybe update the sidebar action shortcuts as required in every call of ...Feature::onRightClickChild().

Connecting the WMainMenuBar::toggleKeyboardShortcuts(bool enabled) signal to the library features is also possible but is certainly even more work.

cr7pt0gr4ph7 · 2024-04-26T13:23:34Z

@ronso0 I've written a quick-and-dirty implementation based on your code, which I will share shortly (as soon as it has built on my machine).

I think the most clean way is to "register" the actions that have to be updated with the KeyboardEventFilter (possibly wrapped behind an abstract KeyboardShortcutManager interface). The library features already receive the KeyboardEventFilter* when LibraryFeature::bindLibraryWidget(WLibrary*, KeyboardEventFilter*) is called,so we can just register the actions with the KeyboardEventFilter then and there.

The code looks quite clean and natural this way. Additions like using a proxy to control lifetimes etc. are also easily possible, but probably not needed anyway. It should probably suffice to use QPointer<QAction> instead of QAction* for storing the references to the actions in KeyboardEventFilter, and to periodically remove actions that have been deleted.

cr7pt0gr4ph7 · 2024-04-26T13:29:03Z

@ronso0 Update: The code can be found here: cr7pt0gr4ph7/mixxx@44a12a8...efc7cd1 (branch is wip-fix-star-rating-merge)
I'll have a look on how to port this to the main branch, if it's forseeable your PR won't be merged anytime soon.

(By the way, I am building Mixxx on my Linux machine inside a VS Code DevContainer, and its sloooooooooow as soon as multiple files are affected. Do you maybe have any tips for speeding the builds up? I am still waiting on the rebuild to finish...

cr7pt0gr4ph7 · 2024-04-26T15:57:07Z

I have looked into the issue of trying to display the correct shortcuts on context menu items etc. Here's my current conclusion:

There are a lot of places in the UI where keyboard shortcuts are available/configurable (e.g. for [Library] AutoDjAddTop) but not displayed in the UI. It would probably be sensible to implement all of those places in the same way.

The implementation for this will most likely look very similar to what @ronso0 has done at Keyboard: automatically reload mapping file and update tooltips #13082. I'd be happy to help on that effort and/or provide a suitable pull request myself. (I actually have a working implementation based on that pull request, which can be found (together with some other stuff) at https://github.com/cr7pt0gr4ph7/mixxx/tree/wip-fix-star-rating-merge).
This PR doesn't really break things any further than they already are, i.e. Mixxx already displays a shortcut for "Rename" that doesn't work when keyboard shortcuts are enabled & F2 is bound to something else.
Implementing the shortcut display logic correctly & cleanly requires passing around parameters to a lot of places and would benefit from a separate discussion, while those modifications are very much orthogonal to the contents of this pull request.

I would therefore argue for keeping this PR self-contained and getting it merged, and deferring the issue of displaying the shortcuts to a separate PR.

ronso0 · 2024-04-27T17:28:13Z

Thank you, that sounds reasonable.

ronso0 · 2024-04-27T17:30:19Z

I am building Mixxx on my Linux machine inside a VS Code DevContainer, and its sloooooooooow as soon as multiple files are affected. Do you maybe have any tips for speeding the builds up?

I have no idea. I'm using Eclipse on Linux with cmake and ccache. With that only changed files (and those units including them) are rebuilt.

ronso0 · 2024-04-29T14:45:03Z

I think the most clean way is to "register" the actions that have to be updated with the KeyboardEventFilter

Yup, like it's done for the menu bar actions in #13082
And that shoudl be straigh forward since we only need to store the KeyboardEventFilter pointer passed around via bindLibraryWidget()

cr7pt0gr4ph7 · 2024-04-30T09:38:31Z

Yup, like it's done for the menu bar actions in #13082 And that shoudl be straigh forward since we only need to store the KeyboardEventFilter pointer passed around via bindLibraryWidget()

See one of my previous comments for a prototype of that 😉 A bit of care has to be taken to drop references to QAction instances that are created temporarily and then destroyed (e.g. when creating context menus) as well as on reloads, if and when we have such code. But besides that, the code is actually pretty simple, provided that #13082 is merged. I'll take a look at reviewing the latter.

cr7pt0gr4ph7 · 2024-05-02T08:21:27Z

@ronso0 Is there anything I can do to help getting this merged? I would love to have this in the official release builds, because it's annoying me in me day-to-day use of Mixxx quite a lot...

ronso0 · 2024-05-04T22:28:16Z

I'll try this soonish.

Regardless when this will be merged, you can run a personal branch with all PRs merged that you need.

cr7pt0gr4ph7 · 2024-05-22T21:13:17Z

I'll try this soonish.

Thank you! I'm still figuring out how this project "ticks", and which way of contributing (small, frequent PRs vs. longer, larger ones) works best. Thanks for the feedback!

Regardless when this will be merged, you can run a personal branch with all PRs merged that you need.

Had a bit of trouble setting it up, because I'm running my production version of Mixxx as a Flatpak application (on an atomic Fedora Silverblue/Bluefin desktop) and the upstream for that only supports 2.4.x and Qt 5 right now... but I've got a CI Flatpak build of my personal staging branch up and running now, if anybody else is interested in copying that setup.
I'm running that build as my main Mixxx instance now, so all contributions should be thoroughly tested on Linux.

I have a few more changesets lined up related to improving keyboard navigation in different areas, quality-of-life improvements for the Auto DJ feature, sound device management and additional hotcue buttons (and probably more stuff in the future), so feel free to tell me in which way you would prefer to receive those.

ronso0 · 2024-05-23T14:04:13Z

I'm still figuring out how this project "ticks", and which way of contributing (small, frequent PRs vs. longer, larger ones) works best.

Yeah, there's no general receipt, developer time is.. fluctuating. Sometimes quick/small PRs get merged rather quickly, sometimes the sit for ages like big/hard-to-review PRs. And there may also been more relevant/pressing WIP that require attention. (like I need to delay this PR for now and try finish my own work first)

github-actions bot added library ui labels Apr 21, 2024

cr7pt0gr4ph7 changed the title ~~Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2~~ (WIP) Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2 Apr 21, 2024

ronso0 reviewed Apr 22, 2024

View reviewed changes

cr7pt0gr4ph7 force-pushed the configurable-edit-item-shortcut branch from 13393f9 to b4f4a3c Compare April 23, 2024 22:10

cr7pt0gr4ph7 added 3 commits April 24, 2024 01:09

LibraryControl: Map the EditItem shortcut to the "Rename" action when…

faf60e6

… in the sidebar

cr7pt0gr4ph7 force-pushed the configurable-edit-item-shortcut branch from b4f4a3c to faf60e6 Compare April 24, 2024 06:26

cr7pt0gr4ph7 changed the title ~~(WIP) Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2~~ Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2 Apr 24, 2024

ronso0 reviewed Apr 24, 2024

View reviewed changes

cr7pt0gr4ph7 added 2 commits April 24, 2024 12:28

WLibrarySidebar: Rename editSelectedItem to renameSelectedItem

0007e5e

WLibrarySidebar: Remove unnecessary condition in renameSelectedItem

0e3787c

isExpanded() is not a good way to check for root nodes, because it returns false for root nodes that are not currently expanded. SidebarModel::renameItem will handle root nodes appropriately anyway.

daschuer approved these changes Apr 26, 2024

View reviewed changes

ronso0 mentioned this pull request May 19, 2024

Keyboard: automatically reload mapping file and update tooltips #13082

Open

cr7pt0gr4ph7 closed this May 21, 2024

cr7pt0gr4ph7 deleted the configurable-edit-item-shortcut branch May 21, 2024 19:15

cr7pt0gr4ph7 restored the configurable-edit-item-shortcut branch May 21, 2024 19:16

cr7pt0gr4ph7 reopened this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2 #13148

Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2 #13148

cr7pt0gr4ph7 commented Apr 21, 2024

JoergAtGithub commented Apr 21, 2024

cr7pt0gr4ph7 commented Apr 21, 2024 •

edited

ronso0 left a comment

ronso0 Apr 22, 2024

cr7pt0gr4ph7 Apr 23, 2024 •

edited

ronso0 Apr 24, 2024

ronso0 Apr 24, 2024

cr7pt0gr4ph7 commented Apr 24, 2024

ronso0 Apr 24, 2024

daschuer left a comment

ronso0 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

cr7pt0gr4ph7 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

cr7pt0gr4ph7 commented Apr 26, 2024

cr7pt0gr4ph7 commented Apr 26, 2024 •

edited

cr7pt0gr4ph7 commented Apr 26, 2024 •

edited

ronso0 commented Apr 27, 2024

ronso0 commented Apr 27, 2024

ronso0 commented Apr 29, 2024 •

edited

cr7pt0gr4ph7 commented Apr 30, 2024

cr7pt0gr4ph7 commented May 2, 2024

ronso0 commented May 4, 2024

cr7pt0gr4ph7 commented May 22, 2024

ronso0 commented May 23, 2024

Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2 #13148

Are you sure you want to change the base?

Usability: Add a rebindable keyboard shortcut for editing items as a replacement for F2 #13148

Conversation

cr7pt0gr4ph7 commented Apr 21, 2024

Issue Statement

Use Case

Proposed Solution (as implemented)

Further Work

JoergAtGithub commented Apr 21, 2024

cr7pt0gr4ph7 commented Apr 21, 2024 • edited

ronso0 left a comment

Choose a reason for hiding this comment

ronso0 Apr 22, 2024

Choose a reason for hiding this comment

cr7pt0gr4ph7 Apr 23, 2024 • edited

Choose a reason for hiding this comment

ronso0 Apr 24, 2024

Choose a reason for hiding this comment

ronso0 Apr 24, 2024

Choose a reason for hiding this comment

cr7pt0gr4ph7 commented Apr 24, 2024

ronso0 Apr 24, 2024

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

cr7pt0gr4ph7 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

ronso0 commented Apr 26, 2024

cr7pt0gr4ph7 commented Apr 26, 2024

cr7pt0gr4ph7 commented Apr 26, 2024 • edited

cr7pt0gr4ph7 commented Apr 26, 2024 • edited

ronso0 commented Apr 27, 2024

ronso0 commented Apr 27, 2024

ronso0 commented Apr 29, 2024 • edited

cr7pt0gr4ph7 commented Apr 30, 2024

cr7pt0gr4ph7 commented May 2, 2024

ronso0 commented May 4, 2024

cr7pt0gr4ph7 commented May 22, 2024

ronso0 commented May 23, 2024

cr7pt0gr4ph7 commented Apr 21, 2024 •

edited

cr7pt0gr4ph7 Apr 23, 2024 •

edited

cr7pt0gr4ph7 commented Apr 26, 2024 •

edited

cr7pt0gr4ph7 commented Apr 26, 2024 •

edited

ronso0 commented Apr 29, 2024 •

edited