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] Steam Shortcuts / Make checkIfAlreadyAdded case-insensitive #3320

Merged
merged 4 commits into from
Dec 31, 2023
Merged

[Fix] Steam Shortcuts / Make checkIfAlreadyAdded case-insensitive #3320

merged 4 commits into from
Dec 31, 2023

Conversation

HazardousBackup
Copy link
Contributor

@HazardousBackup HazardousBackup commented Dec 15, 2023

This change should make checkIfAlreadyAdded insensitive to the case of 'AppName' so if it changes it'll still be recognized and able to be removed and not double added.

Fixes #3124

Change is based on this StackOverflow post

Tested but not on a completely clean install


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)

@flavioislima
Copy link
Member

flavioislima commented Dec 15, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@HazardousBackup
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@flavioislima flavioislima added the pr:ready-for-review Feature-complete, ready for the grind! :P label Dec 15, 2023
@arielj
Copy link
Collaborator

arielj commented Dec 16, 2023

I'm curious, is heroic adding the information with the wrong case for the key? or is steam transforming them in some way?

because maybe we should make heroic use whatever case steam expects instead? do you have an example of this data with keys that are not AppName? maybe we should add the games to steam with appname as the key if that makes them all consistent for example

@HazardousBackup
Copy link
Contributor Author

I'm curious, is heroic adding the information with the wrong case for the key? or is steam transforming them in some way?

because maybe we should make heroic use whatever case steam expects instead? do you have an example of this data with keys that are not AppName? maybe we should add the games to steam with appname as the key if that makes them all consistent for example

It's a bit more than that based on #2030

Apparently Steam isn't 100% consistent on the use of case and can vary over time or OSes, but that doesn't matter to Steam because it doesn't appear to care about the case at all (you can use randomized case with no issues). However, Steam does like to convert entrees to whatever case is standard for it at the moment.

So Heroic can't really rely on just changing AppName to appname because it seems like it could change to appName or something else without warning later on.

@HazardousBackup
Copy link
Contributor Author

I just realized that a change needs to be made to removeNonSteamGame as well. It uses the AppName value of the shortcut, which since it might be a different case could end up being undefined and so the AppID used for removing the artwork won't be correct.

@HazardousBackup
Copy link
Contributor Author

I've committed the change to removeNonSteamGame and tested it on my install.
Before:
Add to Steam -> images added
Change Collection
Remove from Steam -> images stay

After:
Add to Steam -> images added
Change Collection
Remove from Steam -> images removed (as expected)

It's basically the same as the first change and I don't see any other points where the shortcuts file is being accessed that way, so it should be complete.

@HazardousBackup HazardousBackup changed the title Make checkIfAlreadyAdded case-insensitive [Fix] Steam Shortcuts / Make checkIfAlreadyAdded case-insensitive Dec 24, 2023
Copy link
Collaborator

@arielj arielj left a comment

Choose a reason for hiding this comment

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

maybe this is a bit nitpicky, but since we already have something similar in this function

https://github.com/Heroic-Games-Launcher/HeroicGamesLauncher/blob/main/src/backend/shortcuts/nonesteamgame/nonesteamgame.ts#L143

maybe we move getting the value to a function too for consistency:

function getAppName(object) {
  const foundKey = Object.keys(object).find((key) => key.toLowerCase() === 'appname') ?? 'AppName'
  return object[foundKey]
}

then you can do return getAppName(shortcuts) and const appName = getAppName(surtcubOjb)

This change should make checkIfAlreadyAdded insensitive to the case of 'AppName' so if it changes it'll still be recognized and able to be removed and not double added.
Move title finding to separate function for consistency
@HazardousBackup
Copy link
Contributor Author

@arielj Nitpicky or not, it's not a big deal and it helps with consistency. Had I noticed that other function then I likely would've implemented it the same way.

Bonus is that it helps keep the other functions cleaner.

Copy link
Collaborator

@arielj arielj 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 to me 👍

@arielj arielj merged commit 5b46c6d into Heroic-Games-Launcher:main Dec 31, 2023
9 checks passed
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators Dec 31, 2023
@HazardousBackup HazardousBackup deleted the patch-1 branch December 31, 2023 15:16
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.

Heroic may fail to detect entries it added to Steam
3 participants