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: Don't 'handleProtocol' if mainWindow doesn't exist yet #1559

Merged
merged 1 commit into from
Jul 17, 2022

Conversation

arielj
Copy link
Collaborator

@arielj arielj commented Jul 9, 2022

This fixes another issue I found on my jurney to add shortcuts on MacOS. On this pr #1501 I was testing opening heroic with arguments, but I didn't test opening heroic by letting the OS handle the heroic://launch url.

When I tested that, I notices that the open-url event happens before the mainWindow object is even created, and it was trying to handleProtocol with no mainWindow object. Then the process.argv variable was empty in the other handleProtocol in the frontendReady callback becuase it was not passed as an argument to the process by the OS.

In this PR I'm checking first if the mainWindow is exists, if it does it just calls handleProtocol, it if doesn't then it stores the url somewhere so it can be used in the frontendReady callback.

This issue happened only when executing a protocol when heroic was closed.


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)

@@ -433,7 +434,7 @@ ipcMain.on('Notify', (event, args) => {
})

ipcMain.on('frontendReady', () => {
handleProtocol(mainWindow, process.argv)
handleProtocol(mainWindow, [openUrlArgument, ...process.argv])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because of how handleProtocol works, this is fine, since it will go over all the elements of this array until if finds a heroic procotol, so if this is an empty string it's just skipped

@arielj arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label Jul 10, 2022
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!

@flavioislima flavioislima added the pr:ready-to-merge This PR is fully ready for merge. label Jul 15, 2022
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
Copy link
Collaborator

According to the Electron docs, we should add the open-url event handler a little differently than we're doing right now (at least as far as I can tell). Would changing that maybe delay the handling long enough?
Besides, I don't think there's a reason we're calling the Frontend in the protocol, I think that can also be improved

This feels more like a band-aid fix to be honest

@arielj
Copy link
Collaborator Author

arielj commented Jul 16, 2022

According to the Electron docs, we should add the open-url event handler a little differently than we're doing right now (at least as far as I can tell). Would changing that maybe delay the handling long enough?

Not sure, because we are not only depending on the front end, we are depending on the react app to be ready in the frontend, not just electron thinking the app is ready, I think the will-finish-launching event will still fire too early for us.

Besides, I don't think there's a reason we're calling the Frontend in the protocol, I think that can also be improved. This feels more like a band-aid fix to be honest

I agree, the underlaying issue is a different one and this is a workaround on the wrong thing we are doing so at least it works, it fixes something for now but I agree that a refactor of how we launch games should be done at some point to not have to wait for the frontend

@flavioislima
Copy link
Member

Besides, I don't think there's a reason we're calling the Frontend in the protocol, I think that can also be improved

The thing is that if we don't show anything on the frontend, in case the game doesn't run for some reason or we get some error, for instance, during save-sync or so on, the user will get no feedback and then will think that Heroic is broken.

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.

the underlaying issue is a different one and this is a workaround on the wrong thing we are doing so at least it works, it fixes something for now but I agree that a refactor of how we launch games should be done at some point to not have to wait for the frontend

Yup, good to see we're on the same page then
Still, let's merge this for now

@flavioislima flavioislima merged commit dac058f into main Jul 17, 2022
@flavioislima flavioislima deleted the fixes/protocol-handling branch July 17, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P pr:ready-to-merge This PR is fully ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants