Skip to content

[TECH] Move default save computation into the backend #1887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 29 commits into from
Nov 2, 2022
Merged

[TECH] Move default save computation into the backend #1887

merged 29 commits into from
Nov 2, 2022

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Oct 9, 2022 •
edited
Loading

This PR does two main things:

  • Default save path computation is now done in the backend
  • Instead of parsing the path ourselves (and filling in any variables), we now let Legendary do it for us

Since I got a bit carried away, it also features:

  • Types for a few of the GOG-related data structures

TODO:

  • Move GOG's path resolution to the backend as well
  • Pass WINEPREFIX/STEAM_COMPAT_DATA_PATH/CX_BOTTLE to Legendary

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.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@CommandMC
Update legendary to 0.20.29
a01e9bc
@CommandMC
Misc: Minor IPC changes
40ae1c0 
The IPC call removed here did not actually do anything, since there's
never a listener for `requestSettings` created in the frontend
@CommandMC
Add save_path to GameInfo
7441c6b 
This is the save path computed by Legendary, only populated after
`sync-saves` was ran
@CommandMC
[Epic] Move default save computation into the backend & rely on Legen…
ec574de 
…dary to compute it
@CommandMC CommandMC added the pr:wip WIP, don't merge. label Oct 9, 2022
@CommandMC CommandMC self-assigned this Oct 9, 2022
@CommandMC
Fixup: Actually refresh game info after running sync-saves
901258f 
Not sure why this issue didn't happen before, but the game info
wasn't actually being refreshed after we ran the `save-sync` command,
leading to, in theory, no `save_path` ever getting set. Again, no
idea how this worked before
@CommandMC
Pass Wine env vars to Legendary
6fd4765
@CommandMC
Move save computation into its own file
80eb710
@CommandMC
Extract getShellPath into its own function
a124647
@CommandMC
Add a bunch of new GOG interfaces/types
29422b9
@CommandMC
Use the new types in GOGLibrary
14c3c8d
@CommandMC
Move GOG save path computation into the backend
1b35d93
@CommandMC CommandMC force-pushed the feat/dont-guess-save-path branch from 2c42f05 to 1b35d93 Compare October 11, 2022 18:36
@CommandMC CommandMC changed the title [Epic] Move default save computation into the backend & rely on Legendary to compute it Move default save computation into the backend Oct 11, 2022
@CommandMC CommandMC requested a review from imLinguin October 11, 2022 18:37
@CommandMC
Copy link
Collaborator Author

CommandMC commented Oct 11, 2022 •
edited
Loading

Some of the changes in the last 3 commits are a bit off-topic, but I just couldn't live with everything just being the any type, so I annoyed @imLinguin for the last 2 days about all of the structure info he had, and put that into 29422b9

@CommandMC CommandMC added pr:testing This PR is in testing, don't merge. pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:wip WIP, don't merge. labels Oct 11, 2022
@CommandMC CommandMC mentioned this pull request Oct 13, 2022
Merged
6 tasks
@CommandMC CommandMC requested review from a team, arielj, flavioislima, Nocccer and redromnon and removed request for a team October 14, 2022 18:15
@CommandMC CommandMC mentioned this pull request Oct 16, 2022
Merged
4 tasks
@flavioislima
Copy link
Member

Some other GOG games don't even show the input anymore:
image

@CommandMC
Copy link
Collaborator Author

CommandMC commented Oct 17, 2022 •
edited
Loading

I can't find a reference to gogSaves anywhere in the code, everything is using gogSavesLocations. I also can't find savesPath getting set for GOG games (it's only used by Epic games, since those only have 1 possible save path)
I suspect you have some older settings in your config files

After much debugging, I've now reproduced the first issue: winepath indeed just... doesn't run when ran inside of Proton. Will see if I can find why that happens
I'm not sure how that 2nd issue happened though, what's in gogSavesLocations for that one now?

@flavioislima
Copy link
Member

Hmm, I am 90% sure that we use savesPath for GOG as well but I am not sure anymore after the latest save-sync refactor. 🤔

@CommandMC
Copy link
Collaborator Author

CommandMC commented Oct 17, 2022 •
edited
Loading

Well GOG games don't just have one savesPath, so I can't really imagine what we'd put there

Anyways, found the issue with Proton: There's a specific verb (getnativepath) just for running winepath on a path (and another, getcompatpath for converting from Unix to Windows paths)
Looks like I'll have to add a new getWinePath function that uses that then

@CommandMC
Copy link
Collaborator Author

Proton path computation should be fixed now
As for your second issue, I don't quite know how it happened, and I can't reproduce it. I've added a lot of debug logging now (intended to be removed before merging this of course), hopefully that helps in finding the issue

@CommandMC CommandMC mentioned this pull request Oct 29, 2022
Closed
4 tasks
@CommandMC
Copy link
Collaborator Author

I've now managed to reproduce this 2nd issue. It's indeed caused by something set in the cache when using older development versions of Heroic
Clearing your cache (and optionally restarting Heroic) fixed it for me though

@CommandMC
Merge branch 'beta' into feat/dont-guess-save-path
29916f5
@CommandMC
Fixup runWineCommand
d9e5611 
This brings runWineCommand closer to how callRunner works for example.
The same `commandParts` idiom is used to make it possible to exactly
specify what the arguments are that should be passed to `wine`

In the process, I've also made the `wait` variable actually
work again (with spawn, we're no longer using a shell, so we have
to put in a little more work than adding `&& <extra_command>`
to run another command after the first one finishes)
@CommandMC
Update translation files
9c33b5d
@CommandMC CommandMC force-pushed the feat/dont-guess-save-path branch from abe1df7 to 9c33b5d Compare October 30, 2022 00:02
@CommandMC CommandMC removed the pr:testing This PR is in testing, don't merge. label Nov 1, 2022
@flavioislima flavioislima changed the title Move default save computation into the backend [TECH] Move default save computation into the backend Nov 2, 2022
flavioislima
flavioislima approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and it is working fine now with both Proton and Wine

@CommandMC CommandMC merged commit 9b29065 into beta Nov 2, 2022
@CommandMC CommandMC deleted the feat/dont-guess-save-path branch November 2, 2022 21:33
@CommandMC CommandMC mentioned this pull request Nov 29, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flavioislima flavioislima flavioislima approved these changes

@imLinguin imLinguin Awaiting requested review from imLinguin

@arielj arielj Awaiting requested review from arielj arielj was automatically assigned from Heroic-Games-Launcher/development

@Nocccer Nocccer Awaiting requested review from Nocccer Nocccer was automatically assigned from Heroic-Games-Launcher/development

@redromnon redromnon Awaiting requested review from redromnon redromnon was automatically assigned from Heroic-Games-Launcher/development

Assignees

@CommandMC CommandMC

Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CommandMC @flavioislima