From dc83783669ce45392f1955825e129d299ed8190c Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 23 Nov 2022 17:28:21 +0100 Subject: [PATCH 1/4] Restore wraparound in [Library],MoveVertical This makes navigating key-sorted track lists using a controller much more convenient. Though as mentioned in the comment, this does not match the Up/Down cursor key behavior. Should those also wrap around? --- src/library/librarycontrol.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/library/librarycontrol.cpp b/src/library/librarycontrol.cpp index 264a2b25bfc..6d833d23520 100644 --- a/src/library/librarycontrol.cpp +++ b/src/library/librarycontrol.cpp @@ -635,12 +635,14 @@ void LibraryControl::slotMoveVertical(double v) { return; } case FocusWidget::TracksTable: { - // This wraps around at top/bottom. Doesn't match Up/Down key behaviour - // and may not be desired. - //int i = static_cast(v); - //slotSelectTrack(i); - //return; - break; + // This wraps around at the top/bottom of the tracks list. Doesn't match + // Up/Down key behaviour, but it greatly improves the ergonomics of + // navigating a list of tracks sorted by key from a controller. + // Otherwise moving from 12/C#m/E to 1/G#m/B requires either a serious + // workout or reaching for the mouse/keyboard. + const auto i = static_cast(v); + slotSelectTrack(i); + return; } case FocusWidget::Dialog: { // For navigating dialogs map up/down to Tab/BackTab From 92683097b6d993fe5a8aa9d41229d3a1254587e4 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 23 Nov 2022 18:04:45 +0100 Subject: [PATCH 2/4] Also wrap library list around with cursor movement This matches the new(ly restored) behavior from dc83783669ce45392f1955825e129d299ed8190c by overriding `QTableView`'s `moveCursor()` function. This way this also works as expected with the other common selection hotkeys like Shift+Up/Down and Ctrl+Up/Down -> Space. --- src/widget/wlibrarytableview.cpp | 34 ++++++++++++++++++++++++++++++++ src/widget/wlibrarytableview.h | 2 ++ 2 files changed, 36 insertions(+) diff --git a/src/widget/wlibrarytableview.cpp b/src/widget/wlibrarytableview.cpp index 43e18ea706c..37f858389fa 100644 --- a/src/widget/wlibrarytableview.cpp +++ b/src/widget/wlibrarytableview.cpp @@ -260,3 +260,37 @@ void WLibraryTableView::focusInEvent(QFocusEvent* event) { } } } + +QModelIndex WLibraryTableView::moveCursor(CursorAction cursorAction, + Qt::KeyboardModifiers modifiers) { + // The up and down cursor keys should wrap the list around. This behavior + // also applies to the `[Library],MoveVertical` action that is usually bound + // to the library browse encoder on controllers. Otherwise browsing a + // key-sorted library list requires either a serious workout or the user + // needs to reach for the mouse or keyboard when moving between 12/C#m/E and + // 1/G#m/B. + QAbstractItemModel* pModel = model(); + if (pModel && + (cursorAction == QAbstractItemView::MoveUp || + cursorAction == QAbstractItemView::MoveDown)) { + // This is very similar to `moveSelection()`, except that it doesn't + // actually modify the selection + const int row = currentIndex().row(); + const int column = currentIndex().column(); + if (cursorAction == QAbstractItemView::MoveDown) { + if (row + 1 < pModel->rowCount()) { + return pModel->index(row + 1, column); + } else { + return pModel->index(0, column); + } + } else { + if (row - 1 >= 0) { + return pModel->index(row - 1, column); + } else { + return pModel->index(pModel->rowCount() - 1, column); + } + } + } + + return QTableView::moveCursor(cursorAction, modifiers); +} diff --git a/src/widget/wlibrarytableview.h b/src/widget/wlibrarytableview.h index 4f0c00d176d..394f18e0e15 100644 --- a/src/widget/wlibrarytableview.h +++ b/src/widget/wlibrarytableview.h @@ -59,6 +59,8 @@ class WLibraryTableView : public QTableView, public virtual LibraryView { protected: void focusInEvent(QFocusEvent* event) override; + QModelIndex moveCursor(CursorAction cursorAction, + Qt::KeyboardModifiers modifiers) override; virtual QString getModelStateKey() const = 0; private: From 1f4088fa6aa23e70eb07c05dfdee2387f31ee08d Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 23 Nov 2022 18:07:09 +0100 Subject: [PATCH 3/4] Reuse new cursor key handler in MoveVertical The event handler for `WLibraryTableView` now also wraps around the list when navigating it using the cursor keys. So `[Library],MoveVertical` can revert to just sending Up/Down key events to the `WLibraryTableView`. --- src/library/librarycontrol.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/library/librarycontrol.cpp b/src/library/librarycontrol.cpp index 6d833d23520..6c8d297efa3 100644 --- a/src/library/librarycontrol.cpp +++ b/src/library/librarycontrol.cpp @@ -635,14 +635,12 @@ void LibraryControl::slotMoveVertical(double v) { return; } case FocusWidget::TracksTable: { - // This wraps around at the top/bottom of the tracks list. Doesn't match - // Up/Down key behaviour, but it greatly improves the ergonomics of - // navigating a list of tracks sorted by key from a controller. - // Otherwise moving from 12/C#m/E to 1/G#m/B requires either a serious - // workout or reaching for the mouse/keyboard. - const auto i = static_cast(v); - slotSelectTrack(i); - return; + // `WLibraryTableView`'s cursor movement function has been overridden to + // wrap the selection around at the top/bottom of the tracks list. This + // behavior is thus shared between `[Library],MoveVertical` and Up/Down + // cursor key presses. See `WLibraryTableView::moveCursor()` for an + // explanation on why this is useful. + break; } case FocusWidget::Dialog: { // For navigating dialogs map up/down to Tab/BackTab From 11f5f894992df9de5c506ecb18c43789a3b62b19 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 23 Nov 2022 19:37:20 +0100 Subject: [PATCH 4/4] Handle moving invalid WLibraryTableView cursor This happens when the view has not yet been interacted with. --- src/widget/wlibrarytableview.cpp | 34 ++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/widget/wlibrarytableview.cpp b/src/widget/wlibrarytableview.cpp index 37f858389fa..2e69d8a147b 100644 --- a/src/widget/wlibrarytableview.cpp +++ b/src/widget/wlibrarytableview.cpp @@ -275,17 +275,35 @@ QModelIndex WLibraryTableView::moveCursor(CursorAction cursorAction, cursorAction == QAbstractItemView::MoveDown)) { // This is very similar to `moveSelection()`, except that it doesn't // actually modify the selection - const int row = currentIndex().row(); - const int column = currentIndex().column(); - if (cursorAction == QAbstractItemView::MoveDown) { - if (row + 1 < pModel->rowCount()) { - return pModel->index(row + 1, column); + const QModelIndex current = currentIndex(); + if (current.isValid()) { + const int row = currentIndex().row(); + const int column = currentIndex().column(); + if (cursorAction == QAbstractItemView::MoveDown) { + if (row + 1 < pModel->rowCount()) { + return pModel->index(row + 1, column); + } else { + return pModel->index(0, column); + } } else { - return pModel->index(0, column); + if (row - 1 >= 0) { + return pModel->index(row - 1, column); + } else { + return pModel->index(pModel->rowCount() - 1, column); + } } } else { - if (row - 1 >= 0) { - return pModel->index(row - 1, column); + // Selecting a hidden column doesn't work, so we'll need to find the + // first non-hidden column here + int column = 0; + while (isColumnHidden(column) && column < pModel->columnCount()) { + column++; + } + + // If the cursor does not yet exist (because the view has not yet + // been interacted with) then this selects the first/last row + if (cursorAction == QAbstractItemView::MoveDown) { + return pModel->index(0, column); } else { return pModel->index(pModel->rowCount() - 1, column); }