Skip to content
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.

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

[Fix] Fix calculation of appName from command parts #3522

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

arielj
Copy link
Collaborator

@arielj arielj commented Feb 4, 2024

After merging the changes to make logging async I found a bug with the code that gets the appName from the command parts. This was not a big problem before because it was used for the the abortId and some error handling, but with the new async logs the appName is used to find the log path and it was breaking.

The original code had 3 problems:

  • if the command was not launch, it was still always adding 1 and picking a random value for the appName
  • it was only handling launch but not other commands like update, repair, verify (for nile) or download (for gogdl)
  • the appName is only the part after 'launch' for legendary, for gogdl and nile the position in the array is different

This PR addresses:

  • if the command is not one with an appName, we ignore it
  • it handles install/download/update and repair/verify commands too
  • it considers the runner name for the index of the appName in commandParts

To test the problem you can add a console.log({ appName }) after the line I'm changing in this PR in callRunner.ts and see the different appNames that it's calculating in main.

Ideally, in the future, we could just pass the appName as a parameter, but that involves some refactoring that I'd rather do after a release and not before it.


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)

@arielj arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label Feb 4, 2024
@arielj arielj added this to the 2.13.0 milestone Feb 4, 2024
@arielj arielj requested review from a team, flavioislima, CommandMC, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team February 4, 2024 21:23
@imLinguin
Copy link
Member

Can't we just pass appName to callRunner? Parsing command parts that were just assigned seems weird. I know it was like that before, but this really looks like the X Y problem to me.

@arielj
Copy link
Collaborator Author

arielj commented Feb 4, 2024

@imLinguin yes, I have this at the end of the description haha Ideally, in the future, we could just pass the appName as a parameter, but that involves some refactoring that I'd rather do after a release and not before it.

@CommandMC
Copy link
Collaborator

+1 on that; relying on the array structure being the same will 100% "randomly break" in the future (when someone, for whatever reason, changes the structure)

@imLinguin
Copy link
Member

@arielj reading is hard 💀

@CommandMC
Copy link
Collaborator

Reading is hard

How much refactoring are we talking here? Would that really be more changed lines compared to this function (that'd then get thrown away once the proper fix is implemented)

@arielj
Copy link
Collaborator Author

arielj commented Feb 4, 2024

I'm on board of doing that refactor after the release though, I also don't like the parsing of the command, but we use that from places where we don't have an appName and I don't want to break anything at this point

@arielj
Copy link
Collaborator Author

arielj commented Feb 4, 2024

Reading is hard

How much refactoring are we talking here? Would that really be more changed lines compared to this function (that'd then get thrown away once the proper fix is implemented)

well, my issue is that I'd have to make sure that I didn't break any command, we call runRunnerCommand in 15 different places for different actions

and for some commands I don't even have a way to test them (I don't have an amazon account)

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 fine, agree with the refactor after the next release

@arielj arielj merged commit 0fb4813 into main Feb 5, 2024
9 checks passed
@arielj arielj deleted the fix-appName-from-command branch February 5, 2024 21:27
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants