Skip to content

Update [Library],focused_widget #476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Swiftb0y merged 2 commits into from
May 23, 2022
Merged

Update [Library],focused_widget #476

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1afe0c6
Update [Library],focused_widget
ronso0 Mar 17, 2022
9ee0e0f
Update [Library],GoToItem
ronso0 Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions source/chapters/appendix/mixxx_controls.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3435,21 +3435,27 @@ Note that :mixxx:coref:`[Library],MoveUp` and other Move and Scroll controls emu
tracks table has focus, pressing a button loads the selected track to a specific deck, while the same
button would clear the search if the search bar is focused.

Note: This control is useful only if the Mixxx main window has keyboard focus, otherwise it always returns 0.
Note: This control is useful only if a Mixxx window has keyboard focus, otherwise it always returns 0.

:range:
======== ================================
Value Widget
======== ================================
0 none (not writeable)
-------- --------------------------------
1 Search bar
-------- --------------------------------
2 Tree view
-------- --------------------------------
3 Tracks table
======== ================================
:feedback: Currently focused pane changes
======== ========== ================================================================================================
Value writeable Widget
======== ========== ================================================================================================
0 none
-------- ---------- ------------------------------------------------------------------------------------------------
1 X Search bar
-------- ---------- ------------------------------------------------------------------------------------------------
2 X Tree view
-------- ---------- ------------------------------------------------------------------------------------------------
3 X Tracks table or root views of library features
-------- ---------- ------------------------------------------------------------------------------------------------
4 Context menu (menus of library widgets or other editable widgets, or main menu bar)
-------- ---------- ------------------------------------------------------------------------------------------------
5 Dialog (any confirmation or error popup, preferences, track properties or cover art window)
-------- ---------- ------------------------------------------------------------------------------------------------
6 Unknown (widgets that don't fit into any of the above categories)
======== ========== ================================================================================================
Comment on lines +3455 to +3457
Copy link
Copy Markdown
Member

@Swiftb0y Swiftb0y Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more general thought: I'm not sure if its a good design choice to put this last. I think putting it second (Value 1) would make more sense, or maybe as 0 (as in NULL).

Copy link
Copy Markdown
Member Author

@ronso0 ronso0 Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is 0 is null/none (self-explaining), and then there is the 1-3 block with writeable widgets from the previous approach. Breaking up that most meaningful sequence of 0-4 doesn't make much sense to me tbh.

Copy link
Copy Markdown
Member

@Swiftb0y Swiftb0y Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, assuming we add more widgets, they will have to be added after the Unknown category/slot. Which also does not make much sense semantically. If you think that having 0-4 grouped together makes sense, then I'd propose to shift them up and make Unknown the zero slot.

Copy link
Copy Markdown
Member

@Swiftb0y Swiftb0y Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having Unknown as 0 also makes sense from the POV that its like the nullptr when we simply need an implicit fallback value...

To be honest, I'm not super fixated on this issue I just thought I'd mention it. Its fine if you reject it when you don't see the benefit. Saves a little bit of work too.

Copy link
Copy Markdown
Member Author

@ronso0 ronso0 Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for your review then.
I just don't see use cases for more concrete FocusWidget enum items:

  • there are no (focusable) widgets that need to be considered in scripts, all have dedicated controls already (e.g. spinboxes and effect selectors)
  • I doubt we add more focusable widgets to the current skin system
  • for a future multi-pane library the enum concept is not suitable and we'd need to rework the focus concept anyway

Copy link
Copy Markdown
Member Author

@ronso0 ronso0 May 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔔 ping @Swiftb0y ?

Copy link
Copy Markdown
Member

@Swiftb0y Swiftb0y May 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for neglecting this. Thanks for the ping. I don't have any more arguments to convince you. Though I don't consider "we won't change this anyways" a very foresightful perspective.
I won't die on this hill, and the current state is already implemented, so lets just keep it as is.

Copy link
Copy Markdown
Member Author

@ronso0 ronso0 May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I doubt we add more focusable widgets to the current skin system
  • for a future multi-pane library the enum concept is not suitable and we'd need to rework the focus concept anyway

Sorry if this sounds arrogant or ignorant, it's just that I don't see that anyone would invest more time into significant additions the current skin engine or work on a multi-pane library.

Also, please take a look at the getFocusWidget implementation https://github.com/mixxxdj/mixxx/blob/main/src/library/librarycontrol.cpp#L766. We already fallback to None if no Mixxx window has focus or if the main window has no focused widget, and None is considered in all library navigation slots.

[Libary], focused_widget is in main only and that is still alpha, so if you think we should split unknown into known but unaddressed widgets and actually unknown widgets, and change the enum order to put yet unaddressed widgets after the important library widgets... we could still do that.

Anyway, this PR just documents the current implementation so let's merge it.

:feedback: Currently focused widget changes

.. versionadded:: 2.4.0

Expand All @@ -3473,6 +3479,8 @@ Note that :mixxx:coref:`[Library],MoveUp` and other Move and Scroll controls emu
------------------------ -------------------------------------------------------------------------------------------------------------
**Tracks table** Performs the action selected in :menuselection:`Preferences --> Library --> Track Double-Click Action` (default is "Load selected track"). Also see :menuselection:`Preferences --> Decks --> Playing track protection`
------------------------ -------------------------------------------------------------------------------------------------------------
**Context menus** presses :kbd:`Enter`
------------------------ -------------------------------------------------------------------------------------------------------------
**Dialogs / popups** presses :kbd:`Enter`. Note: the :mixxx:coref:`Move.. <[Library],MoveUp>` controls allow to move button focus.
======================== =============================================================================================================

Expand Down