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.16k stars 39 forks source link

Lutris: Add some util functions for conditional game list parsing #364

Closed sonic2kk closed 3 months ago

sonic2kk commented 3 months ago

Some readability improvements that may help extending these checks later for future work, hopefully not too premature! :smile:

When I was digging into #363 I noticed that some of the conditional logic was a bit long and seemingly arbitrary. To help with readability I made some generic utility functions that should explain better than the checks we were doing before.

For #363 we may need to extend the conditional logic i.e. in pupgui2ctinfodialog.py, we may need to add a check for whether a given runtime is in use. This would get pretty long, but with this format, we could make is_lutris_game_using_runtime('dxvk', game.dxvk_version) (or something along these lines, idk how we would want to do this just yet, and not too important for this PR :-) ).

The first function added is to check what runner a LutrisGame is using, with not None and length sanity checks. We were already doing this for the Games List

(https://github.com/DavidoTek/ProtonUp-Qt/blob/6cf1def3bf1ee9775b2639ed41aad1ff96ae2afa/pupgui2/pupgui2gamelistdialog.py#L178)

But this function now means anytime we check the runner name, we perform these checks.

The 2nd function is more specific, and it checks if a LutrisGame is using Wine as its runner, with an optional parameter to check if it's using a specific Wine version. I figured there's no harm in making this function a bit more flexible as it was pretty straightforward to extend.

This could make our checks a lot cleaner should they be extended further. At least in my opinion, if is_lutris_game_using_wine(game, self.ctool.displayname) or is_lutris_game_using_runtime(game, ...) is a lot cleaner than extending our existing check.

sonic2kk commented 3 months ago

There are some typing issues in the ctinfo dialog file, but I think we can clean this up as part of a general sweep of typing changes, once we have Python 3.10 (#359).

sonic2kk commented 3 months ago

I'm wondering if these would be better as methods on the LutrisGame class itself, since we have to pass it a LutrisGame anyway...

DavidoTek commented 3 months ago

Thanks! Looks good.

Makes sense to create utility functions for that, especially if we also need is_lutris_game_using_runtime('dxvk', game.dxvk_version) later.

I noticed that the sanity checks also catch situations where the game.runner is None. Can't remember if that is the case anywhere. If yes, we might want to add a type hint like runner: Union[str, None] to datastructures.py#LutrisGame in the future.

The 2nd function is more specific, and it checks if a LutrisGame is using Wine as its runner, with an optional parameter to check if it's using a specific Wine version. I figured there's no harm in making this function a bit more flexible as it was pretty straightforward to extend. This could make our checks a lot cleaner should they be extended further. At least in my opinion, if is_lutris_game_using_wine(game, self.ctool.displayname) or is_lutris_game_using_runtime(game, ...) is a lot cleaner than extending our existing check.

Yes, that's definitively worth adding the utility functions for making the other code cleaner.

(I just noticed that these functions are a great example for something that could use unit tests https://github.com/DavidoTek/ProtonUp-Qt/issues/347. Haven't gotten around to look into that yet...)

I'm wondering if these would be better as methods on the LutrisGame class itself, since we have to pass it a LutrisGame anyway...

I feel like that it is fine as you did for now. I wonder if at some point we might actually want to move the data structures into the launcher specific files (i.e., move LutrisGame to lutrisutil.py). We could then move the functions into LutrisGame and just rename the game parameter into self.