Skip to content

Library navigation controls #953

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
daschuer merged 19 commits into from
Jan 9, 2017
Merged

Library navigation controls #953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
64e7dd2
Add new control SelectItemKnob
timrae May 22, 2016
28acc2e
Add new control ToggleFocusWidget
timrae May 22, 2016
7d6756c
Add new control ChooseItem
timrae May 22, 2016
38e022c
Replace NULL with nullptr
timrae May 22, 2016
04b04e2
Check for null before dereferencing lib widget activeView
timrae May 22, 2016
ac2b6c1
Use modern C++ style
timrae Jan 7, 2017
ac01058
Add all generic navigation controls
timrae Jan 7, 2017
522a0e4
Set the focus to the sidebar if no widget currently has focus
timrae Jan 7, 2017
c4a9772
Revert changes using non-static member initialization
timrae Jan 7, 2017
bd9b492
Convert raw pointers to unique_ptr
timrae Jan 8, 2017
fd7b609
Add controls for scrolling
timrae Jan 8, 2017
4f4878e
Give the focus to the main library panel instead of the sidebar
timrae Jan 8, 2017
ca45153
Fix bug sending multiple key-press events
timrae Jan 8, 2017
8073616
Use EN-US spelling
timrae Jan 8, 2017
af91335
Use <util/memory.h>
timrae Jan 8, 2017
c779781
Use override for inherited functions
timrae Jan 8, 2017
1964523
Rename [Playlist] to [Library]
timrae Jan 8, 2017
81dbb3d
Replace deprecated [Playlist] controls with [Library] controls in wizard
timrae Jan 8, 2017
b105407
Undeprecate AutoDjAddTop / AutoDjAddBottom
timrae Jan 9, 2017
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
71 changes: 44 additions & 27 deletions src/controllers/controlpickermenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,47 +354,64 @@ ControlPickerMenu::ControlPickerMenu(QWidget* pParent)

// Library Controls
QMenu* libraryMenu = addSubmenu(tr("Library"));
addPrefixedControl("[Playlist]", "ToggleSelectedSidebarItem",
tr("Expand/Collapse View"),
tr("Expand/collapse the selected view (library, playlist..)"),
addPrefixedControl("[Library]", "MoveUp",
tr("Move up"),
tr("Equivalent to pressing the UP key on the keyboard"),
m_libraryStr, libraryMenu);

addPrefixedControl("[Playlist]", "SelectPlaylist",
tr("Switch Next/Previous View"),
tr("Switch to the next or previous view (library, playlist..)"),
addPrefixedControl("[Library]", "MoveDown",
tr("Move down"),
tr("Equivalent to pressing the DOWN key on the keyboard"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Library]", "MoveVertical",
tr("Move up/down"),
Copy link
Copy Markdown
Member

@daschuer daschuer Jan 8, 2017

Choose a reason for hiding this comment

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

We can't remove these old ones before we have no intuitive solution for a RMX2 style control buttons.
"[Playlist]", "SelectNextPlaylist" & "[Playlist]", "SelectPrevPlaylist"

We have to keep them forever if we have a controller with two up/down controls for library.
I do not know one, you?

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We can't remove these old ones before we have no intuitive solution for a RMX2 style control buttons

As we previously discussed, my understanding is that currently UP/DOWN maps to slotSelectTrack(), and LEFT/RIGHT maps to slotSelectSidebarItem(). Furthermore, there are two buttons LoadA and LoadB that load the currently selected track into Deck A / Deck B respectively. Is this correct?

If this is correct then the scheme that I previously suggested is perfectly intuitive. The left and right buttons will change the currently focused pane (shift+tab, tab on keyboard), and the up and down buttons will change the selected item within the currently focused pane (up, down buttons on keyboards). This is FAR more intuitive than using left and right to navigate up and down inside the left sidebar IMHO.

Shift+Left, Shift+Right buttons should be mapped to Left, Right keys on the keyboard
Shift + Up, Shift + Down buttons should be mapped to PgUp, PgDown keys on keyboard

If you feel this is unintuitive then please explain yourself in detail. Also keep in mind that though the current mapping may be more efficient in terms of button presses, it's not scalable to more than two panes in the library.

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 8, 2017

Choose a reason for hiding this comment

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

We have to keep them forever if we have a controller with two up/down controls for library.

Do you mean four buttons, 2 x up, 2 x down? I don't know of such a controller, but even if it does exist you can just map the extra buttons as in left and right above. I don't see any big problem with this.

Copy link
Copy Markdown
Member

@daschuer daschuer Jan 8, 2017

Choose a reason for hiding this comment

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

As we previously discussed, my understanding is that currently UP/DOWN maps to slotSelectTrack(), and LEFT/RIGHT maps to slotSelectSidebarItem(). Furthermore, there are two buttons LoadA and LoadB that load the currently selected track into Deck A / Deck B respectively. Is this correct?

Yes. The current mapping is a pain.

But I do not think that a solution that mandatory requires shift is even better. In this case I will stick with the current mapping, Since it is easier to control.

In master:

  • Right/Left to select the playlist
  • Up/Down to select the track
  • LoadA/LoadB to load the track

Your proposal:

  • Up/Down to select the feature
  • [Shift + Right] to open a tree child
  • Up/Down to select a playlist.
  • Right to switch focus to the track table
  • Up/Down to select the track
  • LoadA/LoadB to load the track

I hope you see that there might be users who want to be able to map th old controls.
It is a bit annoying to step over the expanded tree though.

The proposal for the new library layout may look like this:

  • Up/Down to select the feature
  • Right to switch to the feature pane
  • Up/Down to select a playlist.
  • Right to switch focus to the track table
  • Up/Down to select the track
  • LoadA/LoadB to load the track

No Shift would be required. :-D

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But I do not think that a solution that mandatory requires shift is even better. In this case I will stick with the current mapping, Since it is easier to control.

I really don't understand... According to the code in slotSelectSidebarItem(), it does exactly the same as pressing the UP/DOWN buttons on the keyboard while the sidebar has focus. It sounds like you're saying that it's doing some kind of automatic item expansion that I didn't see in the code when I looked.

i.e. AFAIK what you wrote should be changed to:

In master:

Right/Left to select the sidebar item
Press the right button on the keyboard to expand the playlists item in the tree
Right/Left to choose the playlist (now that is has been expanded)
Up/Down to select the track
LoadA/LoadB to load the track

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The proposal for the new library layout may look like this:

It seems we at least both agree that my proposed mapping is more suitable for the new library layout.

No Shift would be required. :-D

Yeah in the case of track selection it's not required, but some features (browse for example) still need it in general, as the left pane is still a QTree.

Copy link
Copy Markdown
Member

@daschuer daschuer Jan 9, 2017

Choose a reason for hiding this comment

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

Press the right button on the keyboard to expand the playlists item in the tree

Yes that bad, Interesting that I have not noticed it :-/

Yeah in the case of track selection it's not required, but some features (browse for example) still need it in general, as the left pane is still a QTree.

The idea is to add a new key, or modified key to the tree that is "open child" when there is a child and "Tab" when there is no child. Do you think that will work?

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes that bad, Interesting that I have not noticed it :-/

So you agree that my proposed new mapping for RMX is in fact more convenient than the old mapping (mouse/keyboard actions no longer required for navigation), and we can safely remove the old deprecated controls from the MIDI learn wizard in order to discourage their use? Mandatory mouse/keyboard action with the current control set was in fact the main motivation for this PR.

The idea is to add a new key, or modified key to the tree that is "open child" when there is a child and "Tab" when there is no child. Do you think that will work?

I prefer to discuss this outside the context of a merge decision for this PR, as I want to work on the new library layout ASAP, and we seem to agree that the new proposed mapping is better in the current library layout, and MUCH better in the new library layout. OK?

In saying that (i.e. even with the understanding that if we were to add a new control it would not be inside this PR):

In the new library design, "show children" is not required for the most common actions due to the new "button bar" feature selection pane. The only feature which I'm aware of it being required for is "Browse" for navigating the filesystem, so let's assume that scenario.

In this scenario, the most common actions will be change pane and move selection. Show children will be a very uncommon action (i.e. limited to browser feature), so I feel that it's more suitable to map it to the shift key. In this case I don't see any real advantage to having the combined button, since one simply releases the shift key when they're ready to change the pane.

I strongly prefer to keep all navigation controls atomic, so that the code is simple and scalable to different designs. Changing the panes and showing/hiding children in a tree are two completely different things. If you still really want this extra control then please add it yourself in a separate PR?

Copy link
Copy Markdown
Member

@daschuer daschuer Jan 9, 2017

Choose a reason for hiding this comment

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

and we can safely remove the old deprecated controls from the MIDI learn wizard in order to discourage their use?

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

But lets try to find a conclusion for th new library layout as well (without delaying this)

In a somehow designed n-Pane library, we have the issue that a put "TAB" / TAB_BACK" binding cycles though all controls. This is required to control the entire GUI without mouse, but it is not that straight forward for a default controller cursor mapping where we are focused on "Load A Track".

TAB it self is handy on th other hand to directly switch between panes. This should be an ideal candiate for the DEON Shift+Back (Panes) button.

If we are looking to a complex example: #991 (comment) ,
and a user wants to load a track from an old history using the keyboard, he has to do this:

  • UP/DOWN for history feature
  • Enter For enable the history list
  • TAB for history list
  • UP/DOWN for selecting the right group of histories
  • RIGHT for expand group
  • UP/DOWN for selecting and enable the desire history in the history track table.
  • TAB -> TAB -> TAB for going to the selected history track table
  • UP/DOWN for selecting the desired track.

The TAB row becomes longer if we consider a button pane for AutoDJ and future features like Notepad or Lyrics or track suggestor.

This leads to a following proposal for the above action using the controller's cursor keys:

  • UP/DOWN for history feature
  • RIGHT for switching to history list and move the focus to it
  • UP/DOWN for selecting the right group of histories
  • RIGHT for expand group
  • UP/DOWN for selecting the desire history
  • RIGHT again for switching the history track table and move the focus to it
  • UP/DOWN for selecting the desired track.

TAB / TAB_BACK could go to Shift + Right / Left
Shift UP/DOWN seams to be a good PgUp PgDown candidate.

Adopted to the DENON controller it is

  • Turn Knob for history feature
  • Push Knob for switching to history list and move the focus to it
  • Turn Knob for selecting the right group of histories
  • Push Knob for expand group
  • Turn Knob for the desired history
  • Push Knob for switching the history track table and move the focus to it
  • Turn Knob for selecting the desired track.

This looks very convenient to me.

It does not map atomic to a keyboard key, but It reuses right, when it would be normally ignored by the widget. So we may consider to back port this behavior to the keyboard.

What do you think?

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@daschuer

This leads to a following proposal for the above action using the controller's cursor keys:

  • RIGHT for switching to history list and move the focus to it
  • Push Knob for switching to history list and move the focus to it

OK, I see you're basically describing a kind of "GoToSelected" control, which would usually be the equivalent of Enter and then Tab on the keyboard, unless the "last" item in the selection hierarchy (e.g. a track) is selected, in which case it's just loaded into the first stopped deck without looping back to the first pane.

I'm happy to have a go at implementing this, but would like to see how it "feels" in both the old and new library layout before committing to the idea, and before deciding whether it should replace "ChooseItem" or be in addition to it, so let's put that into a separate PR.

tr("Move vertically in either direction using a knob, as if pressing UP/DOWN keys"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Library]", "ScrollUp",
tr("Scroll Up"),
tr("Equivalent to pressing the PAGE UP key on the keyboard"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "SelectNextPlaylist",
tr("Switch To Next View"),
tr("Switch to the next view (library, playlist..)"),
addPrefixedControl("[Library]", "ScrollDown",
tr("Scroll Down"),
tr("Equivalent to pressing the PAGE DOWN key on the keyboard"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "SelectPrevPlaylist",
tr("Switch To Previous View"),
tr("Switch to the previous view (library, playlist..)"),
addPrefixedControl("[Library]", "ScrollVertical",
tr("Scroll up/down"),
tr("Scroll vertically in either direction using a knob, as if pressing PGUP/PGDOWN keys"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "SelectTrackKnob",
tr("Scroll To Next/Previous Track"),
tr("Scroll up or down in library/playlist"),
addPrefixedControl("[Library]", "MoveLeft",
tr("Move left"),
tr("Equivalent to pressing the LEFT key on the keyboard"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "SelectNextTrack",
tr("Scroll To Next Track"),
tr("Scroll to next track in library/playlist"),
addPrefixedControl("[Library]", "MoveRight",
tr("Move right"),
tr("Equivalent to pressing the RIGHT key on the keyboard"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "SelectPrevTrack",
tr("Scroll To Previous Track"),
tr("Scroll to previous track in library/playlist"),
addPrefixedControl("[Library]", "MoveHorizontal",
tr("Move left/right"),
Copy link
Copy Markdown
Member

@rryan rryan Jan 8, 2017

Choose a reason for hiding this comment

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

The AutoDJ add to bottom/top controls aren't replaced by this PR right?

Copy link
Copy Markdown
Member

@daschuer daschuer Jan 8, 2017

Choose a reason for hiding this comment

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

You can use ControlDoublePrivate::insertAlias to track them on old and new name.

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 8, 2017

Choose a reason for hiding this comment

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

If I understand correctly, we'll change it to [Library] as well, and we can insert an alias without creating another ControlObject in the library controller?

Copy link
Copy Markdown
Member

@daschuer daschuer Jan 8, 2017

Choose a reason for hiding this comment

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

Yes, but the alias should be created in the library controller.

Copy link
Copy Markdown
Contributor Author

@timrae timrae Jan 9, 2017

Choose a reason for hiding this comment

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

Done

tr("Move horizontall in either direction using a knob, as if pressing LEFT/RIGHT keys"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "LoadSelectedIntoFirstStopped",
tr("Load Track Into Stopped Deck"),
tr("Load selected track into first stopped deck"),
addPrefixedControl("[Library]", "MoveFocusForward",
tr("Move focus to right pane"),
tr("Equivalent to pressing the TAB key on the keyboard"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "AutoDjAddBottom",
addPrefixedControl("[Library]", "MoveFocusBackward",
tr("Move focus to left pane"),
tr("Equivalent to pressing the SHIFT+TAB key on the keyboard"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Library]", "MoveFocus",
tr("Move focus to right/left pane"),
tr("Move focus one pane to right or left using a knob, as if pressing TAB/SHIFT+TAB keys"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Library]", "AutoDjAddBottom",
tr("Add to Auto DJ Queue (bottom)"),
tr("Append the selected track to the Auto DJ Queue"),
m_libraryStr, libraryMenu);
addPrefixedControl("[Playlist]", "AutoDjAddTop",
addPrefixedControl("[Library]", "AutoDjAddTop",
tr("Add to Auto DJ Queue (top)"),
tr("Prepend selected track to the Auto DJ Queue"),
m_libraryStr, libraryMenu);

// Load track (these can be loaded into any channel)
addDeckAndSamplerControl("LoadSelectedTrack",
tr("Load Track"),
tr("Load selected track"), libraryMenu);
Expand Down
4 changes: 4 additions & 0 deletions src/library/autodj/dlgautodj.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,3 +223,7 @@ void DlgAutoDJ::updateSelectionInfo() {
labelSelectionInfo->setEnabled(false);
}
}

bool DlgAutoDJ::hasFocus() const {
return QWidget::hasFocus();
}
15 changes: 8 additions & 7 deletions src/library/autodj/dlgautodj.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@ class DlgAutoDJ : public QWidget, public Ui::DlgAutoDJ, public LibraryView {
Library* pLibrary,
AutoDJProcessor* pProcessor, TrackCollection* pTrackCollection,
KeyboardEventFilter* pKeyboard);
virtual ~DlgAutoDJ();

void onShow();
void onSearch(const QString& text);
void loadSelectedTrack();
void loadSelectedTrackToGroup(QString group, bool play);
void moveSelection(int delta);
~DlgAutoDJ() override;

void onShow() override;
bool hasFocus() const override;
void onSearch(const QString& text) override;
void loadSelectedTrack() override;
void loadSelectedTrackToGroup(QString group, bool play) override;
void moveSelection(int delta) override;

public slots:
void shufflePlaylistButton(bool buttonChecked);
Expand Down
4 changes: 4 additions & 0 deletions src/library/dlganalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ void DlgAnalysis::onShow() {
m_pAnalysisLibraryTableModel->select();
}

bool DlgAnalysis::hasFocus() const {
return QWidget::hasFocus();
}

void DlgAnalysis::onSearch(const QString& text) {
m_pAnalysisLibraryTableModel->search(text);
}
Expand Down
17 changes: 9 additions & 8 deletions src/library/dlganalysis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@ class DlgAnalysis : public QWidget, public Ui::DlgAnalysis, public virtual Libra
DlgAnalysis(QWidget *parent,
UserSettingsPointer pConfig,
TrackCollection* pTrackCollection);
virtual ~DlgAnalysis();
~DlgAnalysis() override;

virtual void onSearch(const QString& text);
virtual void onShow();
virtual void loadSelectedTrack();
virtual void loadSelectedTrackToGroup(QString group, bool play);
virtual void slotSendToAutoDJ();
virtual void slotSendToAutoDJTop();
virtual void moveSelection(int delta);
void onSearch(const QString& text) override;
void onShow() override;
bool hasFocus() const override;
void loadSelectedTrack() override;
void loadSelectedTrackToGroup(QString group, bool play) override;
void slotSendToAutoDJ() override;
void slotSendToAutoDJTop() override;
void moveSelection(int delta) override;
inline const QString currentSearch() {
return m_pAnalysisLibraryTableModel->currentSearch();
}
Expand Down
4 changes: 4 additions & 0 deletions src/library/dlghidden.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,7 @@ void DlgHidden::setTrackTableFont(const QFont& font) {
void DlgHidden::setTrackTableRowHeight(int rowHeight) {
m_pTrackTableView->setTrackTableRowHeight(rowHeight);
}

bool DlgHidden::hasFocus() const {
return QWidget::hasFocus();
}
7 changes: 4 additions & 3 deletions src/library/dlghidden.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ class DlgHidden : public QWidget, public Ui::DlgHidden, public LibraryView {
DlgHidden(QWidget* parent, UserSettingsPointer pConfig,
Library* pLibrary, TrackCollection* pTrackCollection,
KeyboardEventFilter* pKeyboard);
virtual ~DlgHidden();
~DlgHidden() override;

void onShow();
void onSearch(const QString& text);
void onShow() override;
bool hasFocus() const override;
void onSearch(const QString& text) override;

public slots:
void clicked();
Expand Down
4 changes: 4 additions & 0 deletions src/library/dlgmissing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,7 @@ void DlgMissing::setTrackTableFont(const QFont& font) {
void DlgMissing::setTrackTableRowHeight(int rowHeight) {
m_pTrackTableView->setTrackTableRowHeight(rowHeight);
}

bool DlgMissing::hasFocus() const {
return QWidget::hasFocus();
}
7 changes: 4 additions & 3 deletions src/library/dlgmissing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ class DlgMissing : public QWidget, public Ui::DlgMissing, public LibraryView {
DlgMissing(QWidget* parent, UserSettingsPointer pConfig,
Library* pLibrary, TrackCollection* pTrackCollection,
KeyboardEventFilter* pKeyboard);
virtual ~DlgMissing();
~DlgMissing() override;

void onShow();
void onSearch(const QString& text);
void onShow() override;
bool hasFocus() const override;
void onSearch(const QString& text) override;

public slots:
void clicked();
Expand Down
Loading