Refactor Legendary/GOGDL handling #1092

CommandMC · 2022-03-11T22:00:39Z

This PR unifies every instance of running/invoking Legendary/GOGDL to use one function each (runLegendaryCommand/runGogdlCommand).It improves a few things in the process:

process.chdir() is no longer necessary
Adding directories to %PATH%/$PATH is no longer necessary
Paths with spaces are handled properly (by using the cwd option of spawn)
Every Legendary/GOGDL command can now be logged, all it takes is one argument
Alternative binaries do not require a restart
Some misc fixes/changes
- Old & unused API removed
- Download progress is formatted nicer on Linux
- ExecResult now has an optional fullCommand argument to indicate the exact command that was ran

Use the following Checklist if you have changed something on the Backend or Frontend:

Tested the feature and it's working on a current and clean install.
Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)

Note: Testing was, as usual, done on Linux and Windows. I don't have a Mac, if anyone could test on there, that would be great

adityaruplaha · 2022-03-12T18:05:03Z

Someone please add this to Project Beans. :P

Thanks for this PR, I wanted to get this done for a while but didn't have the time.

CommandMC · 2022-03-13T12:08:24Z

There's an issue with Linux-native GOG games not installing properly right now. I'm working on it

CommandMC · 2022-03-13T22:10:57Z

It seems like everything is working now, the only issue is that installing GOG games doesn't get registered properly so you have to press the "Refresh Library" button once.
I honestly can't say why this happens since I haven't really changed that code

electron/config.ts

electron/utils.ts

electron/main.ts

electron/legendary/library.ts

electron/legendary/games.ts

flavioislima

Tested here for a bit and could not find any bug or issue so looks good.
I believe testing on other platforms and on Flatpak would be good as well. I will try to test on a Mac tomorrow.
Also, @imLinguin would be nice if you could take a look at it as well, especially the GOG part.

But pretty good job with the refactor.
The code is cleaner and more efficient now. Also, more easy to scalate ⚔️

imLinguin

I looked at GOG changes. Looks good to me

imLinguin · 2022-03-15T18:51:02Z

electron/gog/games.ts

+    installedGamesStore.set('installed', array)
+    GOGLibrary.get().refreshInstalled()


About bug you mentioned. This might be a race condition we are looking for, since refreshInstalled reads from the same config. Maybe a function to update installed games directly in memory (instead of reading from the same file again) will solve the issue.

Maybe we could do it like this from now on. Perhaps on another PR:
After refreshing the library on the backend, we send an event to the frontend to refresh the state.
This way we could solve the racing conditions.

…c-Games-Launcher#1081)" This reverts commit 710b367.

This changes every legendary/gogdl-related function to call runLegendaryCommand/runGogdlCommand. With that, it unifies the ~3 different approaches used before into one (hopefully) easy-to-understand function. This has the added benefit that if this function breaks, it'll break everywhere. This should prevent a scenario like in the past few days from happening, as either everything or nothing works.

There were a few unnecessary newlines here that I remove with .trim()

When using a shell, the quotes around paths got removed automatically. Now with spawn, this no longer happened and apparently GOGDL wasn't happy about it.

Not sure why this was done like this, seems to still work fine now

This is kinda hacky, but it stops weird things from happening when starting a download, quitting heroic, and then starting it again

This changes just about all implementations of ExecResult to use the new error property

electron/config.ts

flavioislima · 2022-03-16T09:29:12Z

Tested these changes on a Mac and looks good as well.
The only issue I could find was that after logout the status of the Login Card still shows as logged in. Although I am not sure if this is Mac-specific or if it is related to these changes.

dawidgarus · 2022-03-17T01:50:46Z

electron/gog/library.ts

+
+    if (logFile) {
+      child.stdout.on('data', (data: Buffer) => {
+        appendFileSync(logFile, data.toString())


Kudos for all the changes, but maybe you shouldn't use sync method. Also, new file handle is created for each chunk. This could lead to blocking of app.

~~I suggest opening a file once and appending to it.~~
I suggest creating writeable stream from file and piping output directly to that stream
It would be also nice to use node's readline module. This would enable further changes to fix the atrocity that is the requestGameProgress handler: https://github.com/Heroic-Games-Launcher/HeroicGamesLauncher/blob/main/electron/main.ts#L1100

I agree that this definitely needs improvement, but I'm afraid that I don't know enough about the language yet to make these changes + they don't really fit into this PR. I'd say we merge this for now & then we can look into improving it.

Sure, it can be added later.

CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Mar 11, 2022

CommandMC requested a review from flavioislima March 11, 2022 22:03

CommandMC force-pushed the child_proc_experiments branch from f1713bc to 43f7d3a Compare March 12, 2022 14:56

CommandMC mentioned this pull request Mar 12, 2022

Completely refactor launch() #1044

Merged

2 tasks