Revise librarycontrol // FocusWidget #4618

ronso0 · 2022-01-14T01:11:15Z

While testing #4465 I was looking for a way to reliably navigate the track menu with a push encoder, and it turned out my FocusWidget approach in #4369 is complicated and not extendable to cover menus and dialogs.

Here I revise the focus control:

hook up to QApplication's focusChanged and focusWindowChanged signals. These are emitted every time the focused widget changes or when a new window/popup/menu is ready to receive keyboard presses, thus I could remove almost the entire former implementation.
these signals both invoke updateFocusedWidgetControls()
this identifies the currently focused Mixxx widget, QDialog or QMenu, and stores the FocusWidget type, as well as the respective new state of [Library],focused_widget.
this FocusWidget is now used by a few library controls to trigger context-specific actions -- this and LibraryControl::emitKeyEvent is now much simpler, faster and less error-prone I reckon

Updated enum values of [Library],focused_widget:

0  None        r
1  Searchbar   rw
2  Sidebar     rw
3  TracksTable rw  // or a feature root view (WLibraryTextBrowser)
4  ContextMenu r   // any context menu + main menu bar
                   // effect selector popup + AutoDJ transition mode popup
5  Dialog      r   // any file or error dialog, CoverArt, key wheel window, preferences, About
6  Unknown     r   // Unknown skin widget or unknown window

TESTING

with controller:
Every [Library] control should still work as expected.
For testing the menu navigation, manually open a context menu or one in the main menu bar, then use [Library],MoveVertical to move the selection, [Library],GoToItem to activate items and confirm dialogs. Or put https://github.com/ronso0/mixxx/tree/open-track-menu-control on top of this branch.
It's also possible to handle error messages and some compact dialogs with these controls, too.

without controller:
open developer tools, find focused_widget, then click around in the main GUI and watch how the value changes accordingly, see enum above.

ronso0 · 2022-01-15T23:48:21Z

Alright, I tested all controls that were touched by this PR, as well as navigating menus and dialogs.
Everything works as expected 🎉

src/library/librarycontrol.cpp

daschuer · 2022-01-16T13:32:37Z

There seems to be a kind of loop when the Mixxx window receives the focus, resulting in a flashing focus border.
Is this intended?

daschuer · 2022-01-16T13:39:23Z

The rest works nice. Please give a hint when you consider this a ready for merge.

ronso0 · 2022-01-16T14:55:47Z

There seems to be a kind of loop when the Mixxx window receives the focus, resulting in a flashing focus border. Is this intended?

Not intended at all, and I can't reproduce.
I only hook up to the focus signals, not override focusEvents, so I don't know how the flashing is related to my changes (alternating focusIn/focusOut events). Mixxx itself does not set focus itself -- setLibraryFocus(FocusWidget) is only called when you press Esc in the sidebar or in the search box.

Can you describe your scenario and which widget is flashing?

Swiftb0y · 2022-01-16T15:43:56Z

There seems to be a kind of loop when the Mixxx window receives the focus, resulting in a flashing focus border. Is this intended?

fwiw, I can't reproduce this either.

daschuer · 2022-01-16T18:37:40Z

Oh I have just notice that other Qt Apps are affected as well. So this it probably a Qt issue.

ronso0 · 2022-01-19T21:12:13Z

Please let me know when someone starts a code review.
Until then I may still force-push with cosmetic changes / fixups not worth separate commits.

Holzhaus · 2022-01-22T00:19:28Z

src/widget/wtracktableview.h

@@ -35,7 +34,7 @@ class WTrackTableView : public WLibraryTableView {
    bool hasFocus() const override;
    void setFocus() override;
    void keyPressEvent(QKeyEvent* event) override;
-    void loadSelectedTrack() override;
+    void doubleClickSelectedTrack() override;


We shouldn't name signals like that. Double clicking only works computers with a mouse. It does not work on touchscreens (double tapping), it does not work on controllers (long press?).

How about something like triggerSelectedTrackSecondaryAction() or something like that?

Note: This is not a code review, feel free to force-push.

Or is this a non-issue because this signal will never be emitted by using a touchscreen or a controller?

I simply adjusted to the wording in Preferences > Library > Track Double-Click Action so that the connection is clear for developers.

Controller mappings would use [Library], GoToItem

Keyboards: Keyboard: Enter in library is double-click, change Preview shortcuts #4359

I don't remember testing with a touch screen tbh, no idea how one can interact with the library. Depends on the OS and desktop environment IIRC.

suggestion: activateSelectedTrack

yup, perfect!

…mitKeyEvent

Emulated Shift+Tab sent to QApplication::focusWidget() moves the focus, THOUGH it also makes Shift get stuck for this widget. Actually, this wouldn't matter because Shift is released as soon as this widget receives any keyEvent with Qt::NoModifier. Though, [Playlist],selectTrackKnob doesn't use keyEvents, thus using it directly after [Library],MoveFocusBackward would expand the selection... Simply use BackTab key :)

…ontrol

ronso0 · 2022-02-17T15:28:08Z

For testing without controllers you can put abd1520 on top to have some debug output and skin feedback for FocusWidget.

daschuer

LGTM, And works as expected. Thank you.

ronso0 · 2022-02-22T09:10:20Z

Great, I'll add the documentation when I'm back home.

ronso0 · 2022-10-15T11:15:49Z

fixup 4f45dbe
Thanks @JosepMaJAZ for spotting

github-actions bot added library ui labels Jan 14, 2022

ronso0 marked this pull request as draft January 14, 2022 01:13

ronso0 mentioned this pull request Jan 14, 2022

Library: add control to open/close the track menu #4465

Merged

ronso0 force-pushed the focus-widget-library-control branch 8 times, most recently from b1504a2 to 63ee935 Compare January 15, 2022 23:40

ronso0 marked this pull request as ready for review January 15, 2022 23:41

ronso0 force-pushed the focus-widget-library-control branch from 63ee935 to ca34bfe Compare January 16, 2022 00:32

daschuer reviewed Jan 16, 2022

View reviewed changes

src/library/librarycontrol.cpp Outdated Show resolved Hide resolved

ronso0 force-pushed the focus-widget-library-control branch from ca34bfe to 92fff33 Compare January 16, 2022 15:24

ronso0 force-pushed the focus-widget-library-control branch from 5ed1f3b to f87361f Compare January 19, 2022 21:10

ronso0 force-pushed the focus-widget-library-control branch from f87361f to 3037d5d Compare January 22, 2022 00:00

Holzhaus reviewed Jan 22, 2022

View reviewed changes

ronso0 added 3 commits January 23, 2022 00:37

librarycontrol: revise focus control

829cb4b

librarycontrol: use FocusWidget for MoveVertical & GoToItem, revise e…

fdad014

…mitKeyEvent

librarycontrol: consolidate move.. and scroll.. functions

ca3d53c

ronso0 force-pushed the focus-widget-library-control branch from 3037d5d to a60dcad Compare January 22, 2022 23:52

ronso0 added this to the 2.4.0 milestone Jan 26, 2022

ronso0 added 3 commits January 29, 2022 22:17

Library: rename loadSelectedTrack > activateSelectedTrack

f7d4f12

librarycontrol: simplify slotSelectSidebarItem

4be276b

ronso0 force-pushed the focus-widget-library-control branch from a60dcad to 98033e6 Compare January 29, 2022 21:18

Merge remote-tracking branch 'mixxx/main' into focus-widget-library-c…

239127c

…ontrol

daschuer approved these changes Feb 21, 2022

View reviewed changes

daschuer merged commit 4ed8721 into mixxxdj:main Feb 21, 2022

ronso0 deleted the focus-widget-library-control branch February 22, 2022 09:09

ronso0 mentioned this pull request Mar 17, 2022

Update [Library],focused_widget mixxxdj/manual#476

Merged

ronso0 added the changelog This PR should be included in the changelog label Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise librarycontrol // FocusWidget #4618

Revise librarycontrol // FocusWidget #4618

ronso0 commented Jan 14, 2022 •

edited

Loading

ronso0 commented Jan 15, 2022

daschuer commented Jan 16, 2022

daschuer commented Jan 16, 2022

ronso0 commented Jan 16, 2022

Swiftb0y commented Jan 16, 2022

daschuer commented Jan 16, 2022

ronso0 commented Jan 19, 2022

Holzhaus Jan 22, 2022 •

edited

Loading

Holzhaus Jan 22, 2022

ronso0 Jan 22, 2022

ywwg Jan 29, 2022

ronso0 Jan 29, 2022

ronso0 commented Feb 17, 2022

daschuer left a comment

ronso0 commented Feb 22, 2022

ronso0 commented Oct 15, 2022

Revise librarycontrol // FocusWidget #4618

Revise librarycontrol // FocusWidget #4618

Conversation

ronso0 commented Jan 14, 2022 • edited Loading

TESTING

ronso0 commented Jan 15, 2022

daschuer commented Jan 16, 2022

daschuer commented Jan 16, 2022

ronso0 commented Jan 16, 2022

Swiftb0y commented Jan 16, 2022

daschuer commented Jan 16, 2022

ronso0 commented Jan 19, 2022

Holzhaus Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

Holzhaus Jan 22, 2022

Choose a reason for hiding this comment

ronso0 Jan 22, 2022

Choose a reason for hiding this comment

ywwg Jan 29, 2022

Choose a reason for hiding this comment

ronso0 Jan 29, 2022

Choose a reason for hiding this comment

ronso0 commented Feb 17, 2022

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Feb 22, 2022

ronso0 commented Oct 15, 2022

ronso0 commented Jan 14, 2022 •

edited

Loading

Holzhaus Jan 22, 2022 •

edited

Loading