Skip to content
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.

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

Focus library directly #4369

Merged
merged 2 commits into from
Oct 19, 2021
Merged

Focus library directly #4369

merged 2 commits into from
Oct 19, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Oct 9, 2021

1 Implements an old ToDo in librarycontrol:
focus library directly, without the indirection via sidebar, and without having to assume the library is next/after the sidebar,

2 Adds [Library], focus_widget control which allows mappings with more fine-grained library control.
For example Trax encoder turn or press can now trigger specific actions per context.

The control holds the internal number of the current lib focus widget:

enum class FocusWidget {
    NONE,
    SEARCHBAR,
    SIDEBAR,
    LIBRARY
};

Internally it's updated by [widget]FocusChange([widget] | NONE) signal emitted by each widget's focusInEvent / focusOutEvent.
Valid external updates (from scripts) are 1...n only, verified by each widget's hasFocus() functions.

ToDo

  • remove skin demo

@ronso0 ronso0 added controller mappings changelog This PR should be included in the changelog labels Oct 9, 2021
@ronso0 ronso0 force-pushed the focus-library-directly branch from 069e1f0 to 574e8f2 Compare October 9, 2021 03:52
@uklotzde
Copy link
Contributor

uklotzde commented Oct 9, 2021

This would allow to map dedicated navigation buttons found on some controllers.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 9, 2021

I'm not sure this is a great idea. There is no guarantee that future GUI designs will have all of these widgets which could break controller mappings.

@ronso0
Copy link
Member Author

ronso0 commented Oct 9, 2021

I don't understand the concern. Future GUI designs will still use [group],contrlkey right? So there'll be ways to make use of those COs.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 9, 2021

There is no guarantee there will be a sidebar in future designs, or if it exists, that it would always be showing.

@ronso0
Copy link
Member Author

ronso0 commented Oct 9, 2021

Furtthermore, I think I have some understanding how [Library],libraryViewHasFcocus etc. can be implemented to allow special library behaviour with controllers that have two library control sections like the S4 Mk3.
Pretty much what you were looking for: use those Tracks encoders to trigger GoToItem and load tracks to a specific deck, which IMO can not be solved with a general design for all controllers.
https://mixxx.zulipchat.com/#narrow/stream/247619-UI-.26.20UX.20design/topic/library.20search.20.26.20controller.20mappings/near/254959712

@ronso0
Copy link
Member Author

ronso0 commented Oct 9, 2021

There is no guarantee there will be a sidebar in future designs, or if it exists, that it would always be showing.

Indeed. But sure there'll be some kind of feature bar or alike, like in https://github.com/mixxxdj/mixxx/tree/jmigual-library-redesign.

@Swiftb0y
Copy link
Member

Swiftb0y commented Oct 9, 2021

I'm not sure this is a great idea. There is no guarantee that future GUI designs will have all of these widgets which could break controller mappings.

I disagree. I think this is a very useful addition for lots of mappings that were made for serato. If we for some reason don't have a sidebar (or something comparable) we can just ignore the CO. The mapped button on the hardware does become useless, but it kinda was so already at the beginning when there was no CO.

Rejecting this PR for a hypothetical in the far future leads to stagnation of progress just as much as bikeshedding. We just need to agree/document that we can drop these COs in the future again otherwise this will lead to lots of bikeshedding too.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 9, 2021

The mapped button on the hardware does become useless, but it kinda was so already at the beginning when there was no CO.

That's a good point. It's not that big of a deal if those buttons stop working. In the worst case scenario the user can use their mouse anyway.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 10, 2021

I think this is a very useful addition for lots of mappings that were made for serato.

Can you elaborate? Which specific controllers would this be useful for? It is already confusing how many different ControlObjects there are for navigating the library. What is the benefit of these new ones? Why should they be used instead of the old ones? Should the old ones ever be used?

allow special library behaviour with controllers that have two library control sections like the S4 Mk3.

I don't understand. What does this have to do with the S4 Mk3?

@ronso0
Copy link
Member Author

ronso0 commented Oct 10, 2021

It is already confusing how many different ControlObjects there are for navigating the library

What is confusing you?
[Library],Move... = arrow keys
[Library],MoveFocus.. = Tab key
[Library],GoToItem = a bit like Return

The [Playlist] controls can be used on controllers that have more a Trax knob, or mappings that internally track if you're navigating the sidebar or the table.

The Reloop Terminal Mix series for example has 4 buttons above the Trax push encoder:
View, Crates, Back & Prep
The NI S4 Mk3 also has 4 similiar buttons. Other (Serato) controllers have a View and a Prep button. With the new controls those buttons could now act exactly like're labeled.

As I mentioned a few times already: consider this PR a first step to allow more fine-grained library control.
None of those controls necessarily needs to be used in official mappings, but adds more flexibility for custom mappings.
For example if you only use playlists when performing and thus don't need to access the search bar from the controller, the View button could switch between sidebar and tracks table.

Next step I'm working on: read-only controls for the focus state [Library],XyzHasFocus where Xyz = Libary, Searchbar or Sidebar. With those, pressing a Trax encoder could trigger actions per context, especially with controllers that have two push encoders, thus no need to sacrifice another button to implement MoveFocus controls

  • in the searchbar: go to library, longpress: clear search
  • in the sidebar: go to library, longpress: go to searchbar
  • in the table: load to deck N, longpress: go to searchbar

I hope that finally clarifies my intention and the value of the new controls.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 10, 2021

trigger actions per context

I think this is more complexity than mapping developers should have to think about.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 10, 2021

more flexibility for custom mappings

Again, I am skeptical it is a good idea to make mappings responsible for this. If there are different use cases for the same controller designs, I think those should be handled by preferences in Mixxx, not offloaded to users to edit their mappings if they don't like how it works.

@ronso0 ronso0 marked this pull request as draft October 10, 2021 20:32
@ronso0
Copy link
Member Author

ronso0 commented Oct 10, 2021

made some stupid mistakes I need to iron out.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 10, 2021

For example if you only use playlists when performing and thus don't need to access the search bar from the controller, the View button could switch between sidebar and tracks table.

Why not make a preference option to skip the search bar with MoveHorizontal?

I'm really skeptical that introducing a third way to map library navigation or leaking implementation details of the library to controller mappings are good ideas. What exactly is the problem this PR solves? Is it unfixable by changing the behavior of the existing controls?

With the new controls those buttons could now act exactly like're labeled.

The View button on the S4 Mk3 is designed to maximize the library. I don't understand what that has to do with this PR.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 10, 2021

It seems there are a few common patterns for library navigation on controllers (excluding deck load buttons):

  1. A push encoder (Numark Mixtrack Platinum FX3, Allen & Heath Xone K2)
  2. A push encoder plus 2 buttons (Pioneer DDJ-SX3, DDJ-1000, NI Kontrol S2 Mk3)
  3. A push encoder plus 4 buttons (Denon MC7000, NI Kontrol S4 Mk3, Reloop Terminal Mix)

I think there should be exactly one recommended way to map these common patterns. Currently I cannot figure out what that is nor do I understand how this PR helps.

@ronso0 ronso0 force-pushed the focus-library-directly branch from 574e8f2 to d223291 Compare October 10, 2021 23:11
@ronso0
Copy link
Member Author

ronso0 commented Oct 11, 2021

You have noticed a use case yourself where current controls are not sufficient to have a satisfying experience with the S4 Mk3. Thus you started https://mixxx.zulipchat.com/#narrow/stream/247619-UI-.26.20UX.20design/topic/library.20search.20.26.20controller.20mappings
For sake of completeness

I find it frustrating that after typing in the library search bar, scrolling with [Library], MoveVertical does not scroll through the search results. Instead it scrolls through the history of search queries. Does anyone else find this cumbersome?

The reason you ran into this issue:
Instead of mapping encoder push to [Libary],GoToItem

that would focus the tracks table directly and works fine for regular type 3 devices with 'A push encoder plus 4 buttons'

you want to map [ChannelN],LoadSelectedTrack because the S4 Mk3 (and the Denon MC7000, and CDJs?) is a bit special because they have "A push encoder plus 4 buttons" per deck. (let's call them type 3 B)
The expected behaviour seems to be to navigate the common library with the controls but press the encoder to load the selected track to the respective deck.
Not possible with the current control set.

I propose to add a set_..._focus as well as a read-only xxx_..._focus CO for each library widget.(maybe I even manage to make them in/out COs like the _set_default-suffixed potmeter controls, #3804)
1 a suitable workflow for special type 3 B devices can be created in the scripts
2 anyone can use the COs for any controller to create any workflow

Btw, by now at least 3 devs + me appreciate that.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 11, 2021

I find it frustrating that after typing in the library search bar, scrolling with [Library], MoveVertical does not scroll through the search results. Instead it scrolls through the history of search queries. Does anyone else find this cumbersome?

What does this have to do with this PR? Without this PR I have to press a button to move focus back to the track table. With this PR there is a different button that could be used for that. Either way I have one button to press... so how is this an improvement?

The reason you ran into this issue:
Instead of mapping encoder push to [Libary],GoToItem

That is unrelated. That has no impact on the behavior of [Library], MoveVertical after typing in the search box.

If the reason for this PR is to move focus from the search bar after typing in it, this branch doesn't do anything to change that.

The expected behaviour seems to be to navigate the common library with the controls but press the encoder to load the selected track to the respective deck.
Not possible with the current control set.

I have already done this... what do you mean it's not possible??

@ronso0
Copy link
Member Author

ronso0 commented Oct 11, 2021

TraktorS4mk3.tracksEncoderTurn(...) {
  if (engine.getValue("[Library]", "searchbarHasFocus")) {
    script.triggerControl("[Library]", "focusLibraryView", 50);
  } else if (engine.getValue("[Library]", "libaryViewHasFocus")) {
    engine.setValue("[Library]", "MoveVertical", value - 64);
  }
  ...
}

TraktorS4mk3.tracksEncoderPush(...) {
  if (value) {
    ...
  } else {  // button release
    if (was_longpressed) {
      if (engine.getValue("[Library]", "libaryViewHasFocus")) {
        script.triggerControl("[Library]", "focusSearchbar", 50);
      }
    } else { // quick release
      if (!engine.getValue("[Library]", "searchbarHasFocus")) {
        script.triggerControl("[Library]", "focusSidebar", 50);
      } else if (engine.getValue("[Library]", "sidebarHasFocus")) {
        script.triggerControl("[Library]", "focusLibraryView", 50);
      }
    }
  }
// etc.

The expected behaviour seems to be to navigate the common library with the controls but press the encoder to load the selected track to the respective deck.
Not possible with the current control set.

I have already done this... what do you mean it's not possible??

oh, sorry for the confusion. You were asking for simplicity so I was proposing to do all that with just the encoder turn + push only.
Then you also wouldn't have to the Star and Playlist buttons to MoveFocus... but do something different?
Maybe Star jumps to the search bar, and Playlist switches between sidebar and tracks table?

@Be-ing
Copy link
Contributor

Be-ing commented Oct 11, 2021

do all that with just the encoder turn + push only

I did not agree this was a design goal or desirable. Whether I have to push the encoder or a different button doesn't really matter; I still have to press something either way. I do not think it is a good idea to make the code so complicated for this slightly different behavior that isn't really better than what is already possible.

@ronso0 ronso0 force-pushed the focus-library-directly branch from d223291 to fe2cc41 Compare October 11, 2021 14:25
@Be-ing
Copy link
Contributor

Be-ing commented Oct 11, 2021

Currently only a single button is required to move focus, either [Library], MoveLeft or [Library], MoveRight. I have mapped both on the S4 Mk3 because I have enough buttons to do that. You are proposing to replace that with 3 separate buttons to focus specific widgets... how is replacing 1 or 2 buttons with 3 beneficial?

@github-actions github-actions bot added the skins label Oct 11, 2021
@ronso0
Copy link
Member Author

ronso0 commented Oct 11, 2021

Update:
set ControlPushbuttons
[Library], library_set_focus
[Library], sidebar_set_focus
[Library], searchbar_set_focus

get COs, read-only, bound to focusEvents so they work with mouse and keyboard focus change, too.
[Library], library_has_focus
[Library], sidebar_has_focus
[Library], searchbar_has_focus

This can be tested without controllers with the demo buttons in the lateNight top row.

@Holzhaus
Copy link
Member

Holzhaus commented Oct 15, 2021

focus_widget

Unsure if this is a good name, as we are planning to move away from widgets (and use QML instead) in the future :D But I struggle to come up with a better one, so I guess it's okay. Maybe focused_widget would be better to emphasize that this control can also be read to check what widget has focus.

@ronso0
Copy link
Member Author

ronso0 commented Oct 15, 2021

FWIW QML has nothing to do with QWidget, but the QML docu I read so far uses examples like "MySliderWidget", so I think it doesn't make any difference for users how we name this controls.
Edit I wonder how many users know what a widget is...

@ronso0 ronso0 force-pushed the focus-library-directly branch 2 times, most recently from 8d12415 to 5e29903 Compare October 15, 2021 21:12
@ronso0
Copy link
Member Author

ronso0 commented Oct 15, 2021

@Be-ing I renamed the CO and the internal control.
If this get's a LGTM I have to remove the skin demo, then we can merge.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for you efforts. I have left some comments.

src/library/library_decl.h Outdated Show resolved Hide resolved
src/library/librarycontrol.cpp Outdated Show resolved Hide resolved
src/library/librarycontrol.cpp Outdated Show resolved Hide resolved
src/library/librarycontrol.cpp Show resolved Hide resolved
@ronso0 ronso0 force-pushed the focus-library-directly branch from 5e29903 to 22bb5b2 Compare October 16, 2021 13:38
Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

waiting for @daschuer's approval

@ronso0
Copy link
Member Author

ronso0 commented Oct 19, 2021

@daschuer Anything left to do?

@daschuer
Copy link
Member

I will do a brief manual test.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Works good, thank you.

@daschuer
Copy link
Member

Waiting for the final commit to remove the demo skin.

without the indirection via sidebar, and without assuming the library is next/after the sidebar.
@ronso0 ronso0 force-pushed the focus-library-directly branch from 22bb5b2 to 94f324b Compare October 19, 2021 21:50
@ronso0
Copy link
Member Author

ronso0 commented Oct 19, 2021

Thanks, all done.

@daschuer
Copy link
Member

Thank you.

@daschuer
Copy link
Member

Waiting for CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog controller mappings library skins ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants