Update [Library],focused_widget#476
Conversation
ronso0
force-pushed
the
focus-widget-update
branch
from
faeecf3 to
9ee0e0f
Compare
|
Comment addessed, and |
|
merge? |
Swiftb0y
left a comment
Swiftb0y
left a comment
There was a problem hiding this comment.
Sorry I had a thought shortly after approving but forgot to actually submit the review.
| -------- ---------- ------------------------------------------------------------------------------------------------ | ||
| 6 Unknown (widgets that don't fit into any of the above categories) | ||
| ======== ========== ================================================================================================ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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.
2 participants
updated documentation for mixxxdj/mixxx#4618