search

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

Fix Steam games list regression from #183 #197

Closed sonic2kk closed 1 year ago

sonic2kk commented 1 year ago

I introduced a small regression in #183 that I somehow did not catch. When an appmanifest didn't exist for a SteamApp, the entire app list loading would trigger the exception in get_steam_app_list, meaning a lot of tools were erroneously listed as unused.

I added a check before attempting to load the vdf file to ensure it is actually a valid file.

sonic2kk commented 1 year ago

Actually, it wasn't introduced in that PR... Almost certainly from #183 ? Though I didn't notice this bug back then...

I have been downloading, moving, and uninstalling a bunch of games these last couple of days (new ISP, wanted to test speeds) so I wonder if perhaps this was some semi-edge case that I didn't discover back then that has only reared its head now.

I also noticed an issue with the .get where I forgot to add a fallback if it can't get installdir. It falls back to None now, then we do a check to ensure that the appmanifest path was found to begin with, and then check to ensure the found path exists. This truthy check should also cover blank paths, ensuring that isdir will not fire True in the case of /path/to/steamapps/common existing (which it always should).

sonic2kk commented 1 year ago

Huh, after opening Steam, the unused issue goes away even on main...

Well, I guess this PR may not be needed in the end. Maybe the checks make sense to have anyway, just in case?

DavidoTek commented 1 year ago

Interesting. I didn't notice that behaviour before.

I wonder if perhaps this was some semi-edge case that I didn't discover back then

Maybe the .acf files get created after they are downloaded to the library, so ProtonUp-Qt already detected the game but the .acf file didn't exist yet...

I also noticed an issue with the .get where I forgot to add a fallback if it can't get installdir. It falls back to None now,

.get should return None by default, but it doesn't harm to add it.

This truthy check should also cover blank paths, ensuring that isdir will not fire True in the case of /path/to/steamapps/common existing (which it always should). Maybe the checks make sense to have anyway, just in case?

Yeah, this weird edge case may happen to other as well and blank path may appear if the file is corrupted. So I guess it's good to have.

sonic2kk commented 1 year ago

Maybe the .acf files get created after they are downloaded to the library, so ProtonUp-Qt already detected the game but the .acf file didn't exist yet

I was considering too that this could be the case, it definitely seems like some kind of edge case that shouldn't normally happen. Or maybe it was a hiccup on the Steam side, either way having more robust checking could only be a good thing I think :-)