Replace explicit library home/end key handlers with a moveCursor override #11100
Conversation
robbert-vdh
changed the title
|
Thank you, LGTM It's just I'd prefer to have all keyhandling methods in one class, probably WTrackTableView. |
|
Wouldn't it make more sense to merge |
src/widget/wlibrarytableview.cpp
Outdated
| // By default the home and end keys move to the first and last column rather | ||
| // than the first and last row |
There was a problem hiding this comment.
I had to read that a few times to figure you describe what is overridden, not the new implementation. Let's rephrase it:
| // By default the home and end keys move to the first and last column rather | |
| // than the first and last row | |
| // Make the home and end keys move to the first and last row rather than | |
| // the first and last column (QAbstractItemView default) |
There was a problem hiding this comment.
Sure, I'll change that. Btw, I'm not the only one who does this ha. if you grep for By default you'll get 46 other matches.
I'd prefer merging, but as I wrote in #11090 I'd love to hear an opinion from an 'older' team member on that topic before anyone spends time on this. |
|
Oh and the focus in event handler is also overridden in |
By simply overriding the cursor movement function instead of adding an explicit key event the home and end keys in the library table view now interact as expected with other hotkeys and cursor actions. Shift+End now select all items through the end of the list, and Ctrl+Home jumps to the first item without selecting it so it can be toggled on and off with Space.
robbert-vdh
force-pushed
the
fix/library-home-end-cursors
branch
from
3e309ae to
4ba3a4e
Compare
|
Rebased on top of the changes from the just merged #11090. |
src/widget/wlibrarytableview.cpp
Outdated
| // 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); | ||
| } |
There was a problem hiding this comment.
I think the second comment is actually more relevant for this else branch, so maybe this is more clear?
| // 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); | |
| } | |
| // If the cursor does not yet exist (because the view has not | |
| // yet been interacted with) then this selects the first/last | |
| // row | |
| int row = 0; // MoveDown | |
| if (cursorAction == QAbstractItemView::MoveUp) { | |
| row = pModel->rowCount() - 1; | |
| } | |
| // 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++; | |
| } | |
| return pModel->index(row, column); |
| if (cursorAction == QAbstractItemView::MoveDown) { | ||
| if (row + 1 < pModel->rowCount()) { | ||
| return pModel->index(row + 1, column); | ||
| if (pModel) { |
There was a problem hiding this comment.
Early return if !pModel? Probably a matter of taste, but it reduces the indentation below.
| if (pModel) { | |
| if (!pModel) { | |
| return QTableView::moveCursor(cursorAction, modifiers); | |
| } |
There was a problem hiding this comment.
Yeah I didn't want to repeat the base case. Felt this was similar to something like a Win32 window procedure where you fall back to the default behavior if you didn't return early.
src/widget/wlibrarytableview.cpp
Outdated
| // or the user needs to reach for the mouse or keyboard when moving | ||
| // between 12/C#m/E and 1/G#m/B. This is very similar to | ||
| // `moveSelection()`, except that it doesn't actually modify the | ||
| // selection |
There was a problem hiding this comment.
... only with Ctrl modifier , no?
There was a problem hiding this comment.
No, the function never moves the selection. It just returns a new cursor, which is then used by the AbstractItemView class to do something based on which modifiers are pressed.
There was a problem hiding this comment.
👍 yeah, that's what I mean. Worth noting I'd say
|
I merged this into my personal branch and experienced some issues last night with mixed keyboard and controller navigation but couldn't investigate further at that time:
would not give my the expected selection but less. |
I just tried this and naively moving the cursor with a my controller and then pressing Shift+Home on my keyboard seems to work as expected. I'll see if I can find a situation where it doesn't. |
Oh, sorry, I merged only main with the Up/Down wraparound. |
There was a problem hiding this comment.
This works and looks good.
Thank you.
I have just noticed the inversion of the selection when crossing the list boundaries.
Not sure if this is worth a fix. At least this is unrelated to this PR.
@ronso0 do you want to do more tests or reviews before merge?
|
Yes, I'd like to double-check, though it'll take a few days until I get back to my controller. |
|
@ronso0 Did you find time for the double check? |
|
I didn't explicitly check (tbh I forgot..) but seems I had this already merged into my personal branch and I didn't encounter issues last times I used that live. Thanks @robbert-vdh for this! |
By simply overriding the cursor movement function instead of adding an explicit key event the home and end keys in the library table view now interact as expected with other hotkeys and cursor actions. Shift+End now select all items through the end of the list, and Ctrl+Home jumps to the first item without selecting it so it can be toggled on and off with Space.
Tagging @ronso0 since they suggested it in #11090. Sorry that it took so long (since making this actual change only took a minute heh). Had a busy weekend. Once either of these PRs are merged I'll rebase the other one and refactor the
moveCursor()function a bit.