DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.22k stars 40 forks source link

Allow Sorting in Game List Dialog #180

Closed sonic2kk closed 1 year ago

sonic2kk commented 1 year ago

The goal of this PR is to allow sorting in the Games List dialog, as a little bit of UIX-sugar and to match the behaviour of other QTableWidgets :smile:

Currently, I have implemented sorting for the Game Name, Compatibility Tool (I'll come back to this), Deck Compatibility, and Anti-Cheat status.

Deck Compatibility Rating Sorting

Anti-Cheat Rating Sorting

Going back to the compat tool sorting, it works, but if you change something from say steamlinuxruntime to GE-Proton-blah, the sorting will still be based on the first selected value, so this changed option will appear out-of-order even if you re-sort. In my mind, desirable behaviour here is to update the sort order but to do it after the user selects a new value and then re-sorts. I'll look into what's up with this, the data for that QTableWidgetItem probably needs updated or something after selection (or perhaps there is a signal when the column sort is selected that should check for and update this on sort, not sure yet).

I tested setting the compatibility tool when the columns were sorted and applying the change, and the compatibility tool seems to be correctly updated for the corresponding game even after sorted.

Also, sorting does not yet seem to work for ProtonDB rating. There is an issue with revealing the ProtonDB status on "sorted" columns. If you sort by, say, Game Name, and then "click" for the ProtonDB rating, the rating will be revealed for another game that could be way down the list of installed games. For example, sorting by name and clicking to reveal the rating for "Yakuza 0" will instead reveal the rating beside "Cuphead". I'm not sure yet which game it's actually revealing the rating for.

There is also a minor segfaulting issue I have noticed but been unable to narrow down, it seems to happen randomly when interacting with the Games List dialog after sorting. However it only happens when I'm using the Wayland backend for PySide6. Using QT_QPA_PLATFORM=xcb python3 -m pupgui2 does not have any segfaulting when sorting, perhaps this is just a Qt bug?


TODO

sonic2kk commented 1 year ago

Okay, I've made good progress with the issues I described. Here are the two remaining issues:

  1. The Compat Tool dropdown sorts whenever the dropdown value changes instead of what I initially had in mind, which was to only update on the column heading sort. Probably this can be fixed by attaching a signal to a click for the column header itself - not sure exactly how to do it but it shouldn't be too difficult. Though if the current behaviour is preferred we can keep that too.
  2. The QTableWidgetItem for the ProtonDB rating overlaps with the QLabel. We keep the QLabel because it has the stylesheet + tooltip and I would prefer to keep it, while having the QTableWidgetItem be only for storing the data needed to sort. I can hide the QTableWidgetItem by settings its type to UserRole, but then that breaks sorting :frowning_face: Still investigating a way to resolve this.
sonic2kk commented 1 year ago

I "fixed" the rating overlap by replacing the QLabel that displays the text with the QTableWidgetItem, and set its colour that way.

Unfortunately this introduced a problem whereby when sorting by ProtonDB rating, since we update the data value on the button click, the order of the items immediately changes. So if you click an item on the 5th row and it has a rating of "platinum", it will go to the top. Maybe, like with the compat tool dropdowns, this is more desirable (since the user has t o be sorting by this column in the first place).

I am not sure what the best go-forward is, but we can discuss and tackle the remaining issues. The PR is in a better state now than when I first opened it and I'm happy about that at least :-)

DavidoTek commented 1 year ago

The goal of this PR is to allow sorting in the Games List dialog, as a little bit of UIX-sugar and to match the behaviour of other QTableWidgets 😄

Great!

There is also a minor segfaulting [...] randomly when interacting with the Games List dialog after sorting [...] using the Wayland backend for PySide6 [...] perhaps this is just a Qt bug?

As far as I can tell, the only place where it changes something is in line 205: self.ui.tableGames.item(self.ui.tableGames.currentRow(), 1).setData(Qt.DisplayRole, ctool_name). This should not cause any issue as currentRow() should always be a valid row. Maybe the re-rendering in Wayland causes some sort of problem. Could it cause item(self.ui.tableGames.currentRow(), 1) to be a nullptr for a short duration? Not sure about that...

The Compat Tool dropdown sorts whenever the dropdown value changes instead of what I initially had in mind, which was to only update on the column heading sort So if you click an item on the 5th row and it has a rating of "platinum", it will go to the top

On the one hand, the user then has to search for it again, but on the other hand the user will expect something to change and will known what compatibility tool it is using anyway. At least for me it would be ok though I'm open on this.

Probably this can be fixed by attaching a signal to a click for the column header itself

QTableWidget.horizontalHeaderItem(column: int) will return the header as a QTableWidgetItem. Attachting a Signal to cellClicked could work that sets the DisplayRole data for all items to it's text. We would need to find an efficient way to do this.

Thanks :tada:

sonic2kk commented 1 year ago

Maybe the re-rendering in Wayland causes some sort of problem

This could be the case, there are a handful of rendering issues with PySide 6 and Wayland - Plasma Wayland at least. For example, opening dialogues will not display a background on scaled displays until you move it to an unscaled display and back again. I am not pinning these at all on PUPQT, they are almost certainly issues with either Plasma or PySide/Qt6. I'm bringing this up as because there are a couple of other issues on Wayland that don't occur on X11 (from my tests anyway) it's very possible that the segfaulting issue is related to Wayland.

These problems probably won't occur with the AppImage because I don't think AppImage in general has Wayland support, so it should always fall back to Xwayland. For Flatpak, it may not occur because it uses Flatpak'd dependencies which may be different from system libraries, although having said that the dialog issue is present with Flatpaks.

At least for me it would be ok though I'm open on this.

For now we could stick with the current behaviour and see if a user request that it be changed. The signal approach discussed seems like it's somewhat going against the "intended behaviour".

Some users (e.g., me 😅) may have quite a lot of games installed, and having them jump around the UI may be annoying. Having said that, a user who has sorted may expect the entries to update in real-time anyway. I think it would be overkill to have a toggle for this as well, so maybe we're better off leaving it as-is? :-)

DavidoTek commented 1 year ago

This could be the case, there are a handful of rendering issues with PySide 6 and Wayland - Plasma Wayland at least.

I was not able to replicate the issue with PySide 6.4.2 on GNOME Wayland. At least not with 10 games installed.

couple of other issues on Wayland that don't occur on X11 [...] opening dialogues will not display a background on scaled displays until you move it to an unscaled display

Interesting. I also remember encountering issues on Wayland, at least with older versions of Qt. The background issue sounds related to Plasma...

I don't think AppImage in general has Wayland support

It actually does. It uses an older version of Qt/PySide though and will use X11 by default in most cases.

, it seems to happen randomly when interacting with the Games List dialog after sorting

As it will only happen on some setups and only when a long list of games is sorted by compat tool, we can leave it as is I guess. If it causes any further issues in the future/won't be fixed by a new version of Qt/Plasma, we can reinvestigate the issue then.

Some users (e.g., me) may have quite a lot of games installed, and having them jump around the UI may be annoying [...] so maybe we're better off leaving it as-is? :-)

I see

sonic2kk commented 1 year ago

Oops, i noticed a small change that I somehow lost in my development, sorry for not catching it until now. pdb_item.setData(Qt.UserRole, pdb_tier) should be pdb_item.setData(Qt.DisplayRole, pdb_tier). I have it fixed on a branch with a couple of other minor changes to the Games List that I missed in this PR (the cells are editable now, it needs the NoEditTriggers field set in QtDesigner).

I'll get a PR up with these misc fixes shortly :computer: