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 on [Library] ChooseItem from sidebar #1493

Closed
wants to merge 1 commit into from

Conversation

iamcodemaker
Copy link
Contributor

When the browse knob is mapped to [Library] ChooseItem this will cause
the library to be focused when a leaf node in the sideview is chosen.
Focus only changes when a leaf node is selected, otherwise the given
tree node is expanded or collapsed.

When the browse knob is mapped to [Library] ChooseItem this will cause
the library to be focused when a leaf node in the sideview is chosen.
Focus only changes when a leaf node is selected, otherwise the given
tree node is expanded or collapsed.
@daschuer
Copy link
Member

Welcome to Mixxx!

I am glad you picked this up and prevents us from an odd future interface change.

The original PR is here: #953
Than @timrae has made daschuer#12
unfortunately that went to the library redesign branch that fails to go to 2.1 beta.

So the "[Library] ChooseItem" CO is deprecated before release.
I think we should not release it and switch to "GoToItem" now.

@Be-ing: it is used for three mapings did it even work? Will it be a problem to change it now?

@iamcodemaker: would you mind to adopt the "GoToItem" branch to this PR?

@daschuer
Copy link
Member

We need also you permission to merge your branch.
Please sign: https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
Thank you.

@iamcodemaker
Copy link
Contributor Author

Contributor agreement signed. So I should I rebase this on the daschuer:jmigual-library-redesign branch?

@daschuer
Copy link
Member

No, it is more like rebase daschuer#12 onto this branch.
Or likewise adopt the changes to this branch. The pending question is if we can just replace ChooseItem

@Be-ing
Copy link
Contributor

Be-ing commented Jan 24, 2018

@Be-ing: it is used for three mapings did it even work? Will it be a problem to change it now?

I'm not sure if it ever worked. We've had a handful of reports of track selection encoders not working with 2.1 beta and I'm not clear why. I haven't kept up with the changes to the library COs for controllers because I personally prefer to use my laptop's touchpad or touchscreen.

@daschuer
Copy link
Member

Than I think we should use "GoToItem" and drop "ChooseItem".

@Be-ing Be-ing added the library label Jan 24, 2018
@iamcodemaker
Copy link
Contributor Author

I think we need something that will expand expandable items or focus the library when a leaf node is selected. Looking at the code, it looks like GoToItem always focuses the library, which doesn't seem correct.

@daschuer
Copy link
Member

Yes, right. So a combination of both approaches should work best.

In the new library design we will have a third pane with dynamic custom controls. Some have a tree some not.
It looks like to always using return and tab is a bad idea at least in case of trees.

In the 2.1 case this PR solution seems to be sufficient. But it would be better to decide if we need a tab or not inside the target widget.

Do you have an idea how to solve this for the library redesign branch?

@iamcodemaker
Copy link
Contributor Author

I'll take a look at that branch this evening. Where can I find that code?

While looking at master, ideally the decision to change focus would be handled all the way down in each item and it would signal the library to change focus. But the way it is set up now, the item doesn't know if it was activated because the user scrolled past it or because they clicked on it or ChoseItemed it. So there would need to be another way to notify the child that it was chosen and not just browsed over. (This also causes items to be reloaded more than they need to be, once when highlighted another when ChoseItemed)

Adding that is beyond my level of familiarity with the code base (and QT) so I went for a simpler solution with my implementation here.

@daschuer
Copy link
Member

The upcoming 2.2 code lives here:
https://github.com/mixxxdj/mixxx/tree/jmigual-library-redesign

It is Ok for me if we have a solution that just works for 2.1 like your PR and is interface compatible to library redesign.

@iamcodemaker
Copy link
Contributor Author

This change doesn't work properly with the library-redesign branch. I can get the code to merge and build but the leaf node detection doesn't work. Going to work on a fix.

@daschuer
Copy link
Member

We do not need to have the same solution for both branches if the interface is equal.
For the next library branch I can consider three solutions: Send "Enter" with a modifier like "Shift" and handle the behaviour in each widget. This would allow to do this via keyboard as well. Or add a common call, instead of a key. Or a query, if enter should switch focus or not.
We have also considered to use the curser right key, but we want to stick with the default Os key board commands as close as possible.

@daschuer
Copy link
Member

I have searched the web a bit and a good equivalent is probably Firefox Alt+enter for opening a link in a new tab.

@Be-ing Be-ing added this to the 2.1.0 milestone Feb 25, 2018
@daschuer
Copy link
Member

merged to #1560
Thank you!

@daschuer daschuer closed this Mar 25, 2018
@iamcodemaker iamcodemaker deleted the focus branch November 30, 2018 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants