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

Games List: Move Steam-Specific Logic to steamutil #198

Closed sonic2kk closed 1 year ago

sonic2kk commented 1 year ago

Initially the Games List really only managed Steam game stuff. Lutris recently got support for showing a Games List, and in future hopefully there will also be a Heroic games list.

No screenshot here as there should be no user-facing change, just "backend" changes :-)

This PR splits the ProtonDB fetching logic and the Anti-Cheat status logic out into separate functions in steamutil. This is not really faster or anything as far as I can tell (nor does it appear to be any slower), but the hope is to make the games list a little bit cleaner. Instead of having all of this logic in the games list, it's separated out into utility functions.

This also introduces the possibility of fetching ProtonDB and Anti-Cheat status outside of the games list dialog if it's ever needed, though I don't know when this would be used.

There are also some other small tweaks in this file that I noticed and thought would fit in this PR too, but the main goal of this was to break down the logic in the games list to make it a bit more clean and hopefully easier to maintain as this dialog could grow to accommodate more game lists.

Hope this makes sense and please let me know if there are any further changes/improvements that should be made, or if there are any changes made here that should be undone.

Thanks!

DavidoTek commented 1 year ago

I agree it is a good idea to those into separate function. That makes the code a lot cleaner :)

into separate functions in steamutil.

You pointed out this makes the AWACY strings available to other parts of ProtonUp-Qt. This is a valid argument though I'm not sure whether we should put those functions in steamutil as it contains more backend stuff at the moment. Also, they are only used by the game list at the moment.

I also played around a bit with the i18n part. self.tr(some_dynamic_variable) sadly doesn't work (or at least is not intended to work without work-arounds :smile:). Every string needs own self.tr(''). If we move get_steamapp_awacystatus and get_steamdeck_compatibility to the game list class, nothing needs to be changed in the translation files, though the functions are only ment to be accessed by the game list then. Otherwise we would need to change the "context" of the translations in the translations files and use QApplication.instance().translate('<context>', '<text to translate>') in steamutil.

So I personally would put get_steamapp_awacystatus and get_steamdeck_compatibility as methods in the game list class for now. We can change it later if needed. What do you think?

Thanks!

sonic2kk commented 1 year ago

That's a real shame about the i18n stuff, I did wonder how it might work when I was moving the code around. Oh well, thank you for testing it! I don't think putting in a bunch of workarounds for translations is worth it for a change like this, we can leave it as-is :slightly_smiling_face:

So I personally would put get_steamapp_awacystatus and get_steamdeck_compatibility as methods in the game list class for now. We can change it later if needed.

I think this is the best alternative for now. It still separates the code out, plus there are already launcher-specific "helper" methods in this class so to speak, so it still accomplishes the same goal.

And as you brought up maybe steamutil isn't the best place to put this code anyway. If there comes a time when we absolutely need the Steam Deck compatibility/anti-cheat status somewhere else, that can be resolved separately :-)

I'll move those as methods into the game list class :smile:

DavidoTek commented 1 year ago

Okay.

If there comes a time when we absolutely need the Steam Deck compatibility/anti-cheat status somewhere else, that can be resolved separately :-)

Yes, that's a good idea. Maybe something like "ui-stuff.py" :smile:

I'll move those as methods into the game list class smile

I already did that while testing, I can push that to this PR if you want.

sonic2kk commented 1 year ago

Ah, then yup please go ahead and push if you already have it :smile: