DavidoTek
commented
1 month ago Thanks!
A small benefit to this method is that if later on we decide we want other criteria, for example if Lutris started displaying Bottles or Heroic games for some reason, we can exclude those with an additional runner check
That would be a good addition if Lutris adds that. Good you kept that in mind! :smile:
installed_atHowever in case this is ever missing, it is a less important factor in determining if a game is valid, so I kept it out of is_valid_lutris_gameslist_game and instead did a slight refactor of our logic for setting this.
Makes sense. Good, we should be safe then in case that ever happens.
We recently updated our base Python version to 3.10, meaning we can finally start to use the type hinting in Python3.10 without all the imports or capitalizations (https://docs.python.org/3.10/library/typing.html).
Okay. It's probably the simplest to introduce such changes gradually as you did. There shouldn't be anyone left running older versions of Python anymore now that the AppImage is updated. Arch/AUR is running the latest Python version anyway and I doubt any Pacstall installations are affected either.
@DavidoTek Note to myself: Remember to include a note about the updated Python requirement in the release notes.
In my testing this isn't missing any games in the Lutris Games List, all the games I would expect to be there are present Let me know if you have any concerns about the implementation here or if we need to do any additional work in the is_valid_lutris_gameslist_game check. I know we had some discussion in #397 already but sometimes it's easier to look at an implementation and realise there are problems 😅
Looks good. I tested it with my (small) Lutris game library and everthing was displayed as expected. The code looks good as well.
Off topic: I'm wondering, did Lutris Flatpak change it's config location? I got an error and noticed that my lutris config is not stored in config but in data. Hmm.
$ python3 -m pupgui2
...
FileNotFoundError: [Errno 2] Datei oder Verzeichnis nicht gefunden: '/home/david/.var/app/net.lutris.Lutris/config/lutris/games'
$ ls /home/david/.var/app/net.lutris.Lutris/config/lutris/games
ls: cannot access '/home/david/.var/app/net.lutris.Lutris/config/lutris/games': No such file or directory.
$ ls /home/david/.var/app/net.lutris.Lutris/data/lutris/games
grand-theft-auto-v-rockstar-games-launc-1706250034.yml
...Strange. According to lutris/config.py and lutris/settings.py, it uses user_config_dir which is /home/david/.var/app/net.lutris.Lutris/config/lutris on my system.
sonic2kk
Fixes #397.
Overview
This PR fixes a crash in the Lutris Games List if some games have data as
Nonethat we expect to be defined.I covered this a lot in #397 but to reiterate briefly: When loading the games list, we get our list of
LutrisGames withlutrisutil#get_lutris_game_list. We filter out Steam games from this list. Then we extract data from eachLutrisGameand display it on the UI; the name, runner, install directory, and installation time, as well as some other data for tooltips.However some games can be missing this information. Lutris stores information about its games in an sqlite3 database,
pga.db, which we query. Some games in this DB may be missing a lot of information though. The example I encountered this with was by syncing my Lutris Steam list. It stored information about all my Steam games including ones that weren't installed, and the ones not installed had lots of empty information including an empty runner.This missing runner meant the check we do to skip Lutris games gotten from Steam with
lutrisutil#is_lutris_game_using_runnerwere not catching these games. Since the runner wasNoneand not'steam'then it was missing this.Solution
The fix I ended up going with was to create a method,
PupguiGameListDialog#is_valid_lutris_gameslist_game. We then use this in our list comprehension when storing our list of Lutris games.Stricter checking with
is_valid_lutris_gameslist_gameThe purpose of this function is to check if a
LutrisGameis valid to be displayed on the Games List. In 99.9999% of cases, this is essentially just ensuring a game is installed, as an entry in Lutris' DB does not guarantee that a game is installed. We have to try an infer this (this is even a problem in Lutris itself, where many a missing game may not always be marked as missing, and will simply fail to launch). Lutris does have a column in its DB to track if a game is installed but this is not very reliable from what I have observed from querying, and from what I recall in the past, which is why we don't simply check this.So now we have
is_valid_lutris_gameslist_gamethat will essentially perform a stricter check to make sure games we want to display on the Games List have all the data we need. Instead of working around withNonechecks all over the place, we can assume early when generating theself.gameslist, that if a game is missing arunnerand aninstall_dir, and if the runner is'steam', then we don't want to display it on the games list.A small benefit to this method is that if later on we decide we want other criteria, for example if Lutris started displaying Bottles or Heroic games for some reason, we can exclude those with an additional runner check. This keeps the list comprehension cleaner and also using this method name should make it a bit more obvious why the conditional check is in place.
Safer Logic for Parsing
LutrisGame#installed_atDespite what I said above, I decided that a safer check for
installed_atwasn't out of the question. Based on my extensive querying of my own Lutris DB in https://github.com/DavidoTek/ProtonUp-Qt/issues/397#issuecomment-2116195449, a game that is installed and valid for our purposes (in other words, a game that passesis_valid_lutris_gameslist_game) should always haveinstalled_at. However in case this is ever missing, it is a less important factor in determining if a game is valid, so I kept it out ofis_valid_lutris_gameslist_gameand instead did a slight refactor of our logic for setting this.Instead of setting all the data when setting up the
QTableWidgetItem, we instead store the data in variables. Then we set the variables to different values based on whetherinstalled_atis Truthy (notNoneand not an empty string). If we have aninstalled_atthen we set it up as before. Otherwise, we set the text toself.tr('Unknown'), set the tooltip to "Install Date is Unknown", and then set the data to0. The data is used for sorting to know how to sort by a given column, so we simply set to0. here.After this check we set up the
QTableWidgetItemas before but we give it the variables defined above. This keeps our conditional logic out of the widget item setup at the expense of some extra lines.Minor Type Hinting Refactor
We recently updated our base Python version to 3.10, meaning we can finally start to use the type hinting in Python3.10 without all the imports or capitalizations (https://docs.python.org/3.10/library/typing.html). Just because I was working in this file I decided to update our type hintings in
pupgui2gamelistdialog.pyto use things likelistandtupleinstead ofListandTuple, meaning we can also remove those imports as well.We couldn't replace the
Callabletype hint, but you win some, you lose some :smile:In my testing this isn't missing any games in the Lutris Games List, all the games I would expect to be there are present (there are games I know is missing, but they were displayed before, and Lutris also does not display them as missing, but the files do not exist anymore).
Let me know if you have any concerns about the implementation here or if we need to do any additional work in the
is_valid_lutris_gameslist_gamecheck. I know we had some discussion in #397 already but sometimes it's easier to look at an implementation and realise there are problems :sweat_smile:Thanks!