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

Heroic: Add Info Dialog (GOG, side-loaded, Legendary) #193

Closed sonic2kk closed 1 year ago

sonic2kk commented 1 year ago

Some work is a prerequisite for #168.

Implements support for showing an Info Dialog for Heroic Games compatibility tools :partying_face: To faciliate this, this also implements a HeroicGame datastructure, some associated utility functions and some other misc work around this.

image

Overview

This is heavily work-in-progress for the moment and has only seen light testing.

Implementation

Performance

In my tests, performance seems pretty good, though I only have 176 games (175 from GOG, 1 side-loaded).

Oddities

One other thing I would like to note is about the is_installed field in the library.json. This is set correctly for my side-loaded game, but not for my GOG game. It's installed, and Heroic can see that it's installed, and as well as this it has an entry in the gog_store/installed.json file, but it's still set to false for me.

For this reason I created a utility function is_gog_game_installed that cross-references with the installed.json file to check if the app name for the game is in that file.

Future

When implementing this I tried to keep extensibility and maintenance in mind, for future runners that Heroic might add (if they add Amazon Games etc). It should be mostly straightforward to add support for this, assuming they keep the library.json formats the same, which I would assume they would :sweat_smile:

Getting games from a new runner should really only be a case of adding its path to the store_paths list in get_heroic_game_list.

Improvements

At the very least I would like to include support for showing games using the Epic/Legendary(?) runner in this PR, but I do not use the Epic Games Store so I don't know how Heroic stores data about it - And it doesn't appear to store that data unless you link an Epic Games account, which I don't have.

Also, any type hinting suggestions/improvements are welcome, as well as thoughts on implementing docstrings.


Very open to feedback on this, especially around the information stored for HeroicGames. I'm not sure if there is more we want to store, or if we're storing too much, so please feel free to give suggestion here :-) I tried to keep the rest of the code as concise as I could.

I don't want to submit something half-baked so if there is any further work that should be done here, or if you would prefer this PR to (eventually) capture all of #168 that's fine too. I'd like to fully implement #168 but I thought it better to spread it out across two PRs: One for the ctinfo dialog (as it would include creating the HeroicGame datastructure-related groundwork and getting a proof-of-concept of using that data in a table) and one for the Games List.

Further testing is welcome, as well as help on implementing Epic Games support.

Thanks! :-)

sonic2kk commented 1 year ago

I tested one build of GE-Proton from the Heroic Proton Flatpak list and it worked. Of course more testing is welcome here too :-)

As well as this, I added some logic to set the ct.no_games for Heroic. This isn't present for Lutris but this could be looked into for a separate PR, I just thought it would be a nice addition for Heroic to have. Full parity can come to all install dirs with time 😄

DavidoTek commented 1 year ago

No Epic Games integration - Not sure where the library.json file is (if it exists), what it looks like, etc

As far as I can tell, Heroic doesn't keep track of installed games, but rather uses legendary for that. It stores information about installed games with a different structure in ~/.var/app/com.heroicgameslauncher.hgl/config/legendary/installed.json. The app id correspondents with the name of the json file in GamesConfig.

No Games List integration - It could go in a separate PR,

Yes, no rushing. Adding the basic functionality is already great!

It's installed, and Heroic can see that it's installed, and as well as this it has an entry in the gog_store/installed.json file, but it's still set to false for me.

Well that is strange. Maybe a problem with Heroic? Still good you added the workaround.

When implementing this I tried to keep extensibility and maintenance in mind [...] Getting games from a new runner should really only be a case of adding its path to the store_paths list in get_heroic_game_list.

That's great! Yeah I guess they will use the same format for all stores, at least I hope so.

type hinting suggestions/improvements are welcome, as well as thoughts on implementing docstrings.

Looks good so far. There are some places where no parameter type is specified, they are mostly strings. If the parameter isn't self explanatory (e.g. it's not clear whether a path, name or object is expected), a short description what is expected and what the function does can be added.


Thanks! :tada:

sonic2kk commented 1 year ago

As far as I can tell, Heroic doesn't keep track of installed games, but rather uses legendary for that. It stores information about installed games with a different structure in ~/.var/app/com.heroicgameslauncher.hgl/config/legendary/installed.json. The app id correspondents with the name of the json file in GamesConfig

Ah okay. I'm not sure if we want a separate datastructure for LegendaryGame or anything, or if we should have a separate function (probably called from get_heroic_game_list) to build a list of HeroicGames from the Legendary json structure. I'll take a look at how to implement this, hopefully it won't be too complex :-) Thanks!

Maybe a problem with Heroic?

I think so, I haven't tested on other machines so I'm not sure if this is something specific to this PC somehow. I tried a couple of other GOG games ("Dungeon Keeper Gold" and "DOOM I Enhanced") and they were not marked as installed either. So all 3 I have tested are not marked properly.

Another Heroic oddity that might mildly impact usage: I noticed another Heroic "oddity" that applies to GOG and sideloaded games. During install, selecting tools installed to `~/.var/app//config/heroic/tools/wine` or `tools/proton` (both manually and with ProtonUp-Qt) will cause Heroic to show this: ![image](https://user-images.githubusercontent.com/7917345/220186322-29a2df3f-c7f3-4bc1-b095-fae093b9a950.png) Games won't start when this text shows up, and as a side-effect these games won't show up in the `ctinfodialog` in ProtonUp-Qt. I have to select a different Wine and then re-select this tool to get the warning goes away and for the the game to run. After this the `ctinfodialog` always works. This has to be done with *every game* set to use *any* tool in these folders, installed with or without ProtonUp-Qt. My guess is that the Wine path is not properly written out to the Heroic config files and that's why _ProtonUp-Qt_ doesn't see it, as the implementation I did should be correct as it works when Heroic does not erroneously show this warning text. I don't think there's a good way to work around this one, and I think this is a Heroic bug as it happens when installing the tools manually.

The next step now is implementing Legendary support. I'll see if I can "mimic" a Legendary installed.json based on the example in #168, and use that to test out reading from. But as I don't have any "real" games to test it's definitely possible there could be some teething bugs :-)

sonic2kk commented 1 year ago

Did an initial attempt at implementing Legendary game fetching based on the example installed.json. It's possible that "real" installed games may have other quirks but it works for now :-)

I also just assume that Legendary game configs are stored alongside GOG and sideloaded configs in Heroic/GamesConfig/<app_name>.json, as the base "Legendary" folder didn't have any config folder. If it doesn't we may need to adjust get_game_config to fetch (and potentially) parse the config json separately. Otherwise if the Legendary config structure and location are the same, the Wine information should also be fetched properly now :partying_face:


In that same commit I also removed the folder_name attribute. It is not set at all for Legendary games and does not seem to be set for GOG games. It is only set for side-loaded apps. For GOG and Heroic, they store a install_path in the install.json, which points to a folder which would be equivalent to a folder_name.

The install_path value would also conflict with our install_path, which points to the Heroic install path.

I have an idea on how to improve this and will push a commit with a refactor :-)

sonic2kk commented 1 year ago

So in my research it seems like some but not all GOG games set folder_name. Installed games will usually but not always have it, and it is a bit inconsistent.

I did a bit of a refactor as follows:

Hope the approach I used makes sense :-)

DavidoTek commented 1 year ago

The inconsistency of folder_name is quite intersting. Maybe folder_name is used internally by the GOG/sideload implementations (just a guess, didn't look up in the code)? As you pointed out, using install_path makes way more sense here.

I did a bit of a refactor as follows:

Looks good. The variable names make sense and your approach for is_gog_game_installed seems good.

sonic2kk commented 1 year ago

The Legendary integration may need further review and testing before this is "fully" ready, I'm unable to test it beyond mimicking files for Legendary games. If you find issues and want to directly push a fix please feel free! Otherwise I'm happy to take on board further feedback - I appreciate all the time you put into reviewing my PRs :-)

DavidoTek commented 1 year ago

I did some testing of the Legendary part. Except for a small typo, everything is working fine so far. I'll do some further tests tomorrow, just to be one the safe side.

I appreciate all the time you put into reviewing my PRs :-)

As always, thanks for your contribution!

sonic2kk commented 1 year ago

Just noticed a couple more comments I should probably update that were written before we figured out the Epic part

DavidoTek commented 1 year ago

I made a few little changes to the code (optimization, whitespaces (this PR/general), changed data type hints). I think we can merge it now. Is there anything else to be added/changed?

sonic2kk commented 1 year ago

Changes look good to me, I don't have anything further to do on this that comes to mind 😄 From my testing it should be ready to go!