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

GameList: Have home/end keys move to first/last row #10683

Merged
merged 2 commits into from May 24, 2022

Conversation

Pokechu22
Copy link
Contributor

For whatever reason, QTableView always has home/end move to the first/last column (though it also moves to the first/last row if control is held), even with QAbstractItemView::SelectRows and even though the documentation says it should move to the top-left or bottom-right element. This makes home/end useless for the list view in the game list. This PR overrides it to treat it as if control is held when using home or end.

Note that the list view's home button handler always goes to the first or last item when home or end is pressed, even without this change.

Copy link
Contributor

@Dentomologist Dentomologist left a comment

Choose a reason for hiding this comment

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

There's a preexisting bug that this PR makes more visible.

If you haven't clicked anywhere in the GameList in the current Dolphin session and your first click is in the GameList background (i.e. below the last game), if you then press End before any other selection-related key it will select the first entry instead of the last one. Subsequent End presses work correctly, even if you click outside the gamelist and try to repeat the process.

This bug is easy to work around but it interferes with discoverability; if the user hits End and the wrong item is selected there's a fair chance they'll assume it doesn't work instead of trying it again.

Fixing this bug might be the job of another PR, and I'm not sure what the best approach to dealing with it would be, but I wanted to bring it up here in case you thought of something simple enough to include in this PR. If not, I'll go ahead and approve this and deal with it later.

@Pokechu22
Copy link
Contributor Author

That also affects pressing e.g. page down... which is kinda silly.

The relevant Qt code is this, I think: https://github.com/qt/qtbase/blob/067b53864112c084587fa9a507eb4bde3d50a6e1/src/widgets/itemviews/qtableview.cpp#L1722-L1733. It seems to be possible to work around this like this:

  m_list->selectRow(0);
  m_list->clearSelection();

@Pokechu22
Copy link
Contributor Author

Fixed. The fix doesn't work if the gamelist is initially populated with no games, and then games are added later, but that's a rare situation that I don't think is worth handling.

@Dentomologist
Copy link
Contributor

Dentomologist commented May 22, 2022

That fix makes End work, but now Up/Down/Page Up/Page Down won't select any game after the first (even after multiple presses) until after selecting a game with Home/End/click.

@Pokechu22
Copy link
Contributor Author

I can't reproduce; up/down work fine after clicking once (either on a game or in the background). Can you give more detailed reproduction steps (from immediately after starting Dolphin)?

@Dentomologist
Copy link
Contributor

Dentomologist commented May 22, 2022

Start Dolphin
Click on GameList background
Hit Up arrow/Down arrow/Page Up repeatedly (nothing happens)
Press Page Down (first game is selected)
Press any or all of Up arrow/Down arrow/Page Up/Page Down repeatedly (nothing happens)
Optional: Click on Background (goto previous background click)
Click on any game, or press Home/End (game is selected)
Press Up arrow/Down arrow/Page Up/Page Down (they work correctly)

Windows 10, Qt 6.3.

@Pokechu22
Copy link
Contributor Author

Do you only have one game in your gamelist? I'm able to reproduce the behavior with up and page-up, but down and page-down work properly for me. (The behavior when pressing up also occurs on master if you click the background, press up, then click the background again and press up.)

Have you run git pull --recurse-submodules? Perhaps you're in an inconsistent state after the previous change.

@Dentomologist
Copy link
Contributor

I have 12 games, which is a bit less than a full window at the smallest I can vertically resize the window.

I just did the git command and it pulled some stuff, but after rebuilding I saw the same behavior.

On master the first U/D/PU/PD I do selects the first game. Thereafter U/PU will move the cursor appropriately from the last selected game (whether or not I've clicked on the background), with the caveat that nothing happens if the selected game was the first one. In particular if you have the first game selected, click the background, then press U/PU nothing will be selected.

Likewise, D/PD does nothing when you have the last game selected.

@Pokechu22
Copy link
Contributor Author

Yeah, the behavior you're seeing with page up/page down on master is the same that I'm seeing here with my changes - so I'm not sure what's causing your behavior.

Could you try deleting the lines I added in 0a0303d and seeing if the issue still happens after doing that? That would confirm whether the workaround is actually responsible, or if something else has gone wrong (on your end?).

@Dentomologist
Copy link
Contributor

  • With both sets of lines from that commit commented out, the first action always selects the first game, everything else works normally except for the "U/PU at the top is ignored, etc." behavior that's also on master.
  • If I uncomment the second part (where it sets the index with the QPoint), as far as I can tell everything behaves exactly the same.
  • If I instead uncomment the first part that's when everything breaks.

@Dentomologist
Copy link
Contributor

Aha! SelectRow does the trick, but it only works if Column 0 (Platform) is visible. Hide that in your GameList and you should see the behavior I was talking about.

@Pokechu22
Copy link
Contributor Author

Ah, with the platform column hidden, I can reproduce too. I think I've fixed it by moving the workaround to after the UpdateColumnVisibility call (which also required the connect calls to be moved down).

That probably should be reported to Qt, but I'm not going to do it currently.

@Pokechu22
Copy link
Contributor Author

Note that the same workaround for home/end could also be applied to the breakpoint and watch windows, but I don't use those often enough to know if it really matters, and that could be done in a later PR if needed.

@Tilka Tilka merged commit b6ad5c5 into dolphin-emu:master May 24, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants