Library navigation controls #953

timrae · 2016-05-22T12:22:23Z

Added three new controls that make it a lot easier to use the library encoder on DJ controllers by allowing the following functionality:

SelectItemKnob allows the library encoder to scroll through sidebar items when the sidebar has focus, as opposed to SelectTrackKnob which only scans through the library and SelectPlaylist which only scans through the sidebar items.
ToggleFocusWidget allows users to toggle the focus between the sidebar and the library.
ChooseItem allows the library encoder push button to either expand a sidebar item if sidebar has focus, or load a track into first stopped deck if library has focus. This is intuitive as it's the same behavior as a double click.

uklotzde · 2016-05-22T13:52:13Z

src/library/libraryview.h

@@ -14,6 +14,7 @@ class LibraryView {
    virtual ~LibraryView() {};

    virtual void onShow() = 0;
+    virtual bool hasFocus() = 0;


Wouldn't it make sense to provide QWidget::hasFocus() as a default implementation instead of declaring the function as pure virtual and let derived classes decide if they really need to override the default implementation? I see a lot of code duplication in derived classes that are overriding this function, always using the same implementation.

Could hasFocus() be const?

I was originally trying to do what you said, but I don't really know how to do that in C++ (I've only just started learning). LibraryView is an interface that doesn't have knowledge of whether or not the classes that implement it are QWidgets or not so we can't really add it here. Where would I put the default implementation?

Could hasFocus() be const?

Yes thanks

Sorry, I didn't have a look at the inheritance hierarchy. Then we should
leave it as it is.

On 05/22/2016 03:58 PM, Tim Rae wrote:

In src/library/libraryview.h
#953 (comment):

@@ -14,6 +14,7 @@ class LibraryView {
virtual ~LibraryView() {};

virtual void onShow() = 0;

virtual bool hasFocus() = 0;

I was originally trying to do what you said, but I don't really know how
to do that in C++ (I've only just started learning). LibraryView is an
interface that doesn't have knowledge of whether or not the classes that
implement it are QWidgets or not so we can't add it here. Where would I
put the default implementation?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/mixxxdj/mixxx/pull/953/files/8491121006d713c19c76f205123f1d3b0ba792d6#r64150309

OK so I'll add const to them all

timrae · 2016-05-22T14:31:56Z

@uklotzde

Thanks for the review, I've made the two changes you requested

rryan · 2016-05-22T14:37:01Z

src/library/librarycontrol.cpp

+        return;
+    }
+    // Select track if a LibraryView object has focus, otherwise select sidebar item
+    if (m_pLibraryWidget->getActiveView()->hasFocus()) {


getActiveView can return null so we should check that the result is not null before dereferencing

timrae · 2016-05-22T14:48:18Z

@rryan

Sorry I had noticed that earlier and had forgotten to fix it! I added a new commit

Be-ing · 2016-05-22T16:03:49Z

Thanks for adding these. How do you propose mapping them? SelectItemKnob for turning a browse encoder, ToggleFocusWidget for pressing the encoder, ChooseItem for shift + press encoder?

IMO the "Knob" should be dropped from "SelectItemKnob" considering this should be mapped to encoders, not knobs.

timrae · 2016-05-22T19:36:57Z

Yeah basically mapped like that, except on the mc4000 there's a separate
button called panel that does the focus change, so would use that instead
of shift + encoder press on my controller.

Re: name. I chose it to be consistent with the existing "SelectTrackKnob"
that everyone maps to the encoder. If everyone thinks it shouldn't have the
"Knob" in the name then how about we deprecate "SelectTrackKnob" and add a
"SelectTrack" control to replace it?
On 23 May 2016 01:03, "Be" notifications@github.com wrote:

How do you propose mapping these? SelectItemKnob for turning a browse
encoder, ToggleFocusWidget for pressing the encoder, ChooseItem for shift +
press encoder?

IMO the "Knob" should be dropped from "SelectItemKnob" considering this
should be mapped to encoders, not knobs.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#953 (comment)

timrae · 2016-05-22T19:41:22Z

To be honest I don't really like the "Select" in the name either as it's
ambiguous with the double click event. Maybe something like "NextPrevTrack"
and "NextPrevItem" would be more appropriate...?
On 23 May 2016 04:36, "Tim Rae" perceptualchaos2@gmail.com wrote:

Yeah basically mapped like that, except on the mc4000 there's a separate
button called panel that does the focus change, so would use that instead
of shift + encoder press on my controller.

Re: name. I chose it to be consistent with the existing "SelectTrackKnob"
that everyone maps to the encoder. If everyone thinks it shouldn't have the
"Knob" in the name then how about we deprecate "SelectTrackKnob" and add a
"SelectTrack" control to replace it?
On 23 May 2016 01:03, "Be" notifications@github.com wrote:

How do you propose mapping these? SelectItemKnob for turning a browse
encoder, ToggleFocusWidget for pressing the encoder, ChooseItem for shift +
press encoder?

IMO the "Knob" should be dropped from "SelectItemKnob" considering this
should be mapped to encoders, not knobs.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#953 (comment)

daschuer · 2016-05-23T07:59:24Z

The change of input focus is hard to get in all skins. I have just filed this bug:
https://bugs.launchpad.net/mixxx/+bug/1584613

I am afraid the original interface will be obsolete soon, since we are working on a 3 column layout, that will be hard to control with it. So we are free to introduce a new namespace, though I am happy with the current names.

IMHO the new interface, should also allow the default keyboard behavior on a controller with:
"Up" SelectPrevItem
"Down" SelectNextItem
"Left" move selection left / collapse
"Right" move selection right / expand
"Enter" ChooseItem
"Tab" ToggleFocusWidget

We have also the issue, that the current interface unlikely to the keyboard does not continue to scroll when holding the up/down button on the controller.

@timrae: Do you have time and fun to add the missing controls?

daschuer · 2016-07-17T11:47:59Z

Fixes: https://bugs.launchpad.net/mixxx/+bug/1603737

The new SelectItemKnob control selects either the next track or the next sidebar item, depending on which widget has focus

The new ToggleFocusWidget toggles the focus between the sidebar and the library by sending a Tab button press to the sidebar widget

The new ChooseItem toggles acts like a double-click, either toggling the expanded state of a sidebar item or loading the selected track into the first stopped deck if a track is selected

timrae · 2017-01-07T08:45:52Z

@daschuer

I've implemented the controls that you requested. I used slightly different names than what we discussed as I think these are more clear. Please re-review the code.

void slotMoveUp(double);
void slotMoveDown(double);
void slotMoveVertical(double);
void slotMoveLeft(double);
void slotMoveRight(double);
void slotMoveHorizontal(double);
void slotMoveFocusForward(double);
void slotMoveFocusBackward(double);
void slotMoveFocus(double);
void slotChooseItem(double v);

The only problem that I've experienced during my testing is that, when mixxx is initially started up, no widget has the focus. I've spent several hours trying to get that working, but the only way I could get it to work is by setting a timer with an arbitrary delay, as in 5a6a90a. The delay is necessary as setFocus() will only be effective when the widget is actually showing.

Does anyone have any better idea to implement this? Also, I'd rather have the main library pane focused instead of the sidebar... Is it supposed to be the widget in wtracktableview.cpp?? Setting focus on that didn't appear to work.

daschuer · 2017-01-07T18:41:25Z

src/library/librarycontrol.h

+    void emitKeyEvent(QKeyEvent&& event);
+
+    // Controls to navigate vertically within currently focussed widget (up/down buttons)
+    ControlPushButton* m_pMoveUp = nullptr;


In this case, the Initialisation here is redundant since they are initialized in the constructor again. It should be removed. This code inlines a delegating constructor, in each user code.

General :
You use here a new C++ feature, which is used in Mixxx here the first time.
"Non-static data member initializers"

There are some pros and cons discussed for this feature:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0

In my real live, I us recently C# and it also supports this feature. It allows to spread class initialisation to the whole file which is from my experience not very readable.
In the current Mixxx code you just need to look at the constructor. If we allow to do it in the class definition we brake this rule.

I am not strictly against this feature, but we should come to a conclusion how we will use it in Mixxx in future.
For example it might be nice to use it for inline classes, or If there is would be only an empty constructor with an initialisation list.

Any thoughts?
@rryan @uklotzde

At my work I use the convention that only the member variables which depend on the input arguments go in the initializer list. If the variable should be initialized with a value which is known at compile time then we always put the value in at the point of declaration in the header file.

This has two advantages:

It requires less code

It is less maintenance work because the order of declaration in the header file in principle should be the same as the order of initialization.

http://stackoverflow.com/questions/1242830/constructor-initialization-list-evaluation-order

Because of this stupid c++ rule it's a good idea to only put the necessary members into the initializer list for your own sanity

I.e the priority order should be:

header file, point of declaration

initializer list

constructor body

In this case to follow that rule I guess we should try to move all of the controller variable initializations to the initialization list. I didn't try it yet though.

OK I added a commit using the rules I mentioned above

There is an other thing, that bugs me more: How to atomize the live time handling of the ControlObjects.

Honestly, modern C++ style teaches to feel sick every time you see a "naked new". I don't really understand how the QT object tree is supposed to work... I guess most developers don't.

You previously mentioned some benchmarks you guys did on "smart pointers" vs. the object tree. Is there any code remaining for this? I often hear people make these kind of claims that "smart pointers are slow" but actually std::unique_ptr has almost zero overhead, and inevitably when we look at the "slow" code they're using a different smart pointer or doing something wrong in the first place.

Having the creation of the control and connecting its signals next to each other improves understandability -- that's why most controls are created this way instead of being in the member initialization list. I think at least for ControlProxys we could move to a pattern where we create them as member variables (instead of allocating in the constructor) letting the no-args constructor run, and then call initialize(ConfigKey) alongside the connection calls. It's more complicated for ControlObject derivatives since we pass a lot of configuration through the constructor args.

I think @daschuer's proposed alternative to is to follow the style of most code in Mixxx and not use non-static data member initializers except for the cases he outlined: "For example it might be nice to use it for inline classes, or If there is would be only an empty constructor with an initialisation list."

That sounds reasonable to me. I like using non-static member initialization for classes/structs that are short-and-sweet (e.g. helper structs with no methods). I share the concern that it makes it harder to be sure you've initialized everything and makes you have to check 2 places (or 2 + n for delegating constructors) to figure out the initial value of the class state.

Having the creation of the control and connecting its signals next to each other improves understandability

How so?

I think @daschuer's proposed alternative is...

Yes I understood the part related to the non-static member initialization, but I was referring to the initializer list. Can you put the rules into simple words? Is there a style guide on the wiki?

The de-facto rule is "initialize every member in the member initializer list". ControlObjects would be the most common exception. I don't think the Qt object tree bit is relevant to this particular discussion since you can easily create objects that are part of the tree with the member initializer list since you can pass "this" as the parent if you choose.

The style guide is here:
http://www.mixxx.org/wiki/doku.php/coding_guidelines
but it does not say anything beyond the Google C++ style guide about member variable initialization.

timrae · 2017-01-07T20:39:46Z

I had a better idea to solve the setFocus() issue I mentioned above, and force pushed a new commit to replace that.

daschuer · 2017-01-08T22:19:11Z

Done: https://www.mixxx.org/wiki/doku.php/coding_guidelines#unique_ptr_stdmove
please review.

timrae · 2017-01-08T23:12:41Z

Done: https://www.mixxx.org/wiki/doku.php/coding_guidelines#unique_ptr_stdmove
please review.

Yes! Prefer to QScopedPointer.

Change "to" to "over". I misread it first to mean "Prefer to use QScopedPointer"

If possible, prefer std::unique_ptr in std:: containers over QSharedPointer in Qt containers.

I think it's confusing to sometimes allow QT smart pointers. IMO if we're going to always prefer unique_ptr over QScopedPointer, we should also prefer shared_ptr over QSharedPointer. In which case ho about changing the text above to:

std::unique_ptr in general cannot be used in QT containers, as these require the copy constructor. There are two ways to solve this: A) Convert the QT container to an std:: container (preferred) or B) Use std::shared_ptr inside a Qt container (only in cases where conversion is not feasible).

Site note

Side note ;)

daschuer · 2017-01-08T23:40:47Z

Typos fixed, thanks.

I think it's confusing to sometimes allow QT smart pointers. IMO if we're going to always prefer unique_ptr over QScopedPointer, we should also prefer shared_ptr over QSharedPointer. In which case ho about changing the text above to:

Is there a benefit of switching to shared_ptr?

IMHO we should prefer Qt classes in our Qt code, if there is no reason for other classes.

Due to the move semantic a std:unique_ptr can be used instead of a QScopedPointer, which clarifies the ownership, but if we really have shared ownership, QSharedPointer will work and fits to the surrounding code.

timrae · 2017-01-08T23:45:15Z

Ok, I don't really have a strong preference... I'm just more accustomed to STL classes, but if you only want unique ptr for the move semantics I guess you could say explicitly: prefer unique ptr over QScoped and prefer QShared over shared ptr

timrae · 2017-01-08T23:48:45Z

Honestly though, it feels a bit like "pissing into the wind" trying to use the QT data structures in a C++11 and beyond world that they clearly weren't designed for.

rryan · 2017-01-09T00:02:58Z

IMHO we should prefer Qt classes in our Qt code, if there is no reason for other classes.

👍 to continuing to prefer Qt classes as the default, unless there's a good reason otherwise (such as here).

One benefit I see is that Qt uses the same implementation on all platforms, while the C++ standard library implementation changes per-platform. So using Qt classes we can make slightly stronger assumptions about the implementation we're getting for a given class.

One small benefit to using QSharedPointer is that you get little integrations with QObjects (the pointer automatically goes null when a QObject is deleted by another means, which is a nice safeguard against bugs).

std::unique_ptr seems strictly better than QScopedPointer and I think we should continue to replace it everywhere.

rryan · 2017-01-09T00:10:28Z

Honestly though, it feels a bit like "pissing into the wind" trying to use the QT data structures in a C++11 and beyond world that they clearly weren't designed for.

Yea, though in Qt5 a lot of containers became movable, and I'm hoping we will get better support for things like emplacement, etc. in the future.

timrae · 2017-01-09T08:21:03Z

@daschuer

Yes. .. and there is no reason to delay this PR for future decisions.

I guess this is ready to merge then?

daschuer · 2017-01-09T23:06:10Z

Yes, Thank you very much! LGTM

sblaisot · 2017-01-10T07:35:29Z

Did you update https://www.mixxx.org/wiki/doku.php/mixxxcontrols#library @timrae ?

timrae · 2017-01-10T07:41:45Z

Not yet, but I will do it within the next few days

…

On 10 Jan. 2017 16:35, "Sebastien BLAISOT" ***@***.***> wrote: Did you update https://www.mixxx.org/wiki/doku.php/mixxxcontrols#library @timrae <https://github.com/timrae> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#953 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4rkJbqi1pFcuoTaq3HuC3SBpRTQsks5rQzTEgaJpZM4Ij-f8> .

timrae · 2017-01-11T08:59:56Z

@sblaisot
Done

Be-ing · 2017-01-19T22:06:57Z

Backwards compatibility with the old [Playlist] COs is broken.

daschuer · 2017-01-20T07:55:07Z

It looks like we break something:
https://bugs.launchpad.net/mixxx/+bug/1657743
@timrae: Can you have a look?

timrae · 2017-01-20T10:13:17Z

Yes I'll try take a look this weekend

…

On 20 Jan. 2017 16:55, "Daniel Schürmann" ***@***.***> wrote: It looks like we break something: https://bugs.launchpad.net/mixxx/+bug/1657743 @timrae <https://github.com/timrae>: Can you have a look? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#953 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4lOO0dnpvSA73GFc7g_eNbhKTs1Kks5rUGhdgaJpZM4Ij-f8> .

timrae · 2017-01-20T23:40:37Z

#1137

Be-ing · 2017-05-14T14:41:13Z

Testing these new Controls in master from the Developer Tools menu in Developer Mode doesn't seem to do anything. Are they dependent on #1117 to work?

timrae · 2017-05-31T00:28:01Z

They shouldn't be, could you please give some simple steps for me to reproduce the issues you are seeing?

uklotzde reviewed May 22, 2016
View reviewed changes

timrae force-pushed the library-navigation-controls branch from 8491121 to 71cc630 Compare May 22, 2016 14:31

timrae force-pushed the library-navigation-controls branch 2 times, most recently from df738e3 to a6e10d5 Compare May 22, 2016 14:34

rryan reviewed May 22, 2016
View reviewed changes

daschuer mentioned this pull request Aug 14, 2016

Library layout redesign #991

Closed

timrae added 6 commits January 7, 2017 10:22

Add new control SelectItemKnob

64e7dd2

The new SelectItemKnob control selects either the next track or the next sidebar item, depending on which widget has focus

Add new control ToggleFocusWidget

28acc2e

The new ToggleFocusWidget toggles the focus between the sidebar and the library by sending a Tab button press to the sidebar widget

Add new control ChooseItem

7d6756c

The new ChooseItem toggles acts like a double-click, either toggling the expanded state of a sidebar item or loading the selected track into the first stopped deck if a track is selected

Replace NULL with nullptr

38e022c

Check for null before dereferencing lib widget activeView

04b04e2

Use modern C++ style

ac2b6c1

timrae force-pushed the library-navigation-controls branch from 74fa469 to bbb536b Compare January 7, 2017 06:48

timrae force-pushed the library-navigation-controls branch 2 times, most recently from 32e5d59 to 5a6a90a Compare January 7, 2017 09:14

Add all generic navigation controls

ac01058

timrae force-pushed the library-navigation-controls branch from 5a6a90a to 89f5cfd Compare January 7, 2017 16:19

daschuer reviewed Jan 7, 2017

View reviewed changes

Set the focus to the sidebar if no widget currently has focus

522a0e4

timrae force-pushed the library-navigation-controls branch from 89f5cfd to 522a0e4 Compare January 7, 2017 20:05

timrae mentioned this pull request Jan 9, 2017

Metaknob soft takeover #1111

Merged

Undeprecate AutoDjAddTop / AutoDjAddBottom

b105407

daschuer merged commit cb88711 into mixxxdj:master Jan 9, 2017

timrae mentioned this pull request Jan 11, 2017

MC4000 update #1102

Merged

timrae deleted the library-navigation-controls branch January 11, 2017 08:59

timrae mentioned this pull request Jan 15, 2017

Replace the ChooseItem control with new GoToItem daschuer/mixxx#12

Merged

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

daschuer mentioned this pull request Jan 24, 2018

focus library on [Library] ChooseItem from sidebar #1493

Closed

Library navigation controls #953

Library navigation controls #953

Conversation

timrae commented May 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timrae May 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timrae commented May 22, 2016

Choose a reason for hiding this comment

timrae commented May 22, 2016

Be-ing commented May 22, 2016 • edited Loading

timrae commented May 22, 2016

timrae commented May 22, 2016

daschuer commented May 23, 2016

daschuer commented Jul 17, 2016

timrae commented Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timrae commented Jan 7, 2017

daschuer commented Jan 8, 2017

timrae commented Jan 8, 2017

daschuer commented Jan 8, 2017 • edited Loading

timrae commented Jan 8, 2017 • edited Loading

timrae commented Jan 8, 2017

rryan commented Jan 9, 2017

rryan commented Jan 9, 2017

timrae commented Jan 9, 2017

daschuer commented Jan 9, 2017

sblaisot commented Jan 10, 2017

timrae commented Jan 10, 2017 via email

timrae commented Jan 11, 2017 • edited Loading

Be-ing commented Jan 19, 2017

daschuer commented Jan 20, 2017

timrae commented Jan 20, 2017 via email

timrae commented Jan 20, 2017

Be-ing commented May 14, 2017 • edited Loading

timrae commented May 31, 2017

timrae commented May 22, 2016 •

edited

Loading

timrae May 22, 2016 •

edited

Loading

Be-ing commented May 22, 2016 •

edited

Loading

timrae commented Jan 7, 2017 •

edited

Loading

daschuer commented Jan 8, 2017 •

edited

Loading

timrae commented Jan 8, 2017 •

edited

Loading

timrae commented Jan 11, 2017 •

edited

Loading

Be-ing commented May 14, 2017 •

edited

Loading