Skip to content

[FIX] Issues with Epic Login and refresh Library #1073

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 9 commits into from
Mar 10, 2022
Merged

[FIX] Issues with Epic Login and refresh Library #1073

merged 9 commits into from
Mar 10, 2022

Conversation

flavioislima
Copy link
Member

@flavioislima flavioislima commented Mar 10, 2022 •
edited
Loading

  • Add function to deal with spaces on all platforms
  • Add shell to spawn methods so binaries with spaces can work normally
  • change how we deal with legendary commands even if the first function to deal with spaces is not reliable

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)

This was linked to issues Mar 10, 2022
[All] Can't login to Epic Games in Heroic 2.2.4 #1067
Closed
The Metadata for Apps does not exist so it cannot launch, Mac, when trying to play Rocket League #1068
Closed
Grand Theft Auto V (GTA V) problem #1069
Closed
@flavioislima flavioislima added the pr:ready-for-review Feature-complete, ready for the grind! :P label Mar 10, 2022
Nocccer
Nocccer previously approved these changes Mar 10, 2022
View reviewed changes
Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

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

Lgtm

CommandMC
CommandMC previously approved these changes Mar 10, 2022
View reviewed changes
Copy link
Collaborator

@CommandMC CommandMC left a comment

Choose a reason for hiding this comment

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

Well, it's a band-aid fix that's surely gonna bite us in the butt in the future
But it's still a fix

Works on Linux and my Win VM

imLinguin
imLinguin previously approved these changes Mar 10, 2022
View reviewed changes
@flavioislima
Copy link
Member Author

Well, it's a band-aid fix that's surely gonna bite us in the butt in the future But it's still a fix

Works on Linux and my Win VM

Ahahaha
What are you worried about? something in specific?

I was thinking that we should make a global function that could be used across the legendary and the gogdl parts. So we could receive the full path for the binary and do the necessary treatments/checks for it to run.

But one thing I learned today was this process.chdir this is very useful and maybe you can use that @imLinguin on the gogdl side. to avoid using that workaround for windows with the & I tested and worked fine but I didn't want to commit since I would need more time testing.

@flavioislima flavioislima dismissed stale reviews from imLinguin, CommandMC, and Nocccer via b1e2582 March 10, 2022 16:54
CommandMC
CommandMC reviewed Mar 10, 2022
View reviewed changes
Copy link
Collaborator

@CommandMC CommandMC left a comment

Choose a reason for hiding this comment

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

Using process.chdir works, but I fear that we'll get very hard-to-find issues like this if every function would start using it (since then you never know in which directory we're in right now, and it's easy to miss one of these and get unexplainable issues)

electron/constants.ts Outdated
// unfortunatelly this cant go into utils since it needs to be loaded on app start
function fixPathWithSpaces(path: string) {
// to detect on with os user had used path.resolve(...)
const is_posix_os = !isWindows
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels kinda unnecessary, why not just do if (!isWindows) below?

electron/constants.ts Outdated
const should_not_escape = (major_release = '', os_build = '') =>
/1\d+\.\d+/.test(major_release) && Number(os_build) >= 17134.1184

if (is_posix_os) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be higher up, so the regex isn't applied when it's not used anyway

electron/launcher.ts
@@ -145,10 +145,14 @@ async function launch(
) {
let command = ''
if (runner == 'legendary') {
command = `${legendaryBin} launch ${appName} ${exe} ${runOffline} ${
const legendaryPath = dirname(legendaryBin).replaceAll('"', '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be defined once in constants.ts? Setting it in like 5 different places does not seem like a good idea

@flavioislima
Copy link
Member Author

@CommandMC yes, I believe using chdir for legendary and gogdl only would be good.
Unfortunately, nodeJS spawn doesn't work with name with spaces because it expects only one command and not the whole path. So that's the only way I could find.
If you find another way of fixing this, especially on windows, we can talk about it.

@flavioislima flavioislima merged commit 3c86b02 into main Mar 10, 2022
@flavioislima flavioislima deleted the fix_enoent branch March 10, 2022 17:21
@CommandMC
Copy link
Collaborator

Wait a minute, doesn't spawn have a cwd parameter? Why don't we just use that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CommandMC CommandMC CommandMC left review comments

@Nocccer Nocccer Nocccer left review comments

@imLinguin imLinguin imLinguin left review comments

Assignees
No one assigned
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Milestone
No milestone
4 participants
@flavioislima @CommandMC @Nocccer @imLinguin