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: MacOS crash on startup #428

Merged
merged 15 commits into from
Mar 13, 2025
Merged

fix: MacOS crash on startup #428

merged 15 commits into from
Mar 13, 2025

Conversation

larssn
Copy link

@larssn larssn commented Feb 17, 2025

So I've tried my hand at fixing the issue in #427.

I've gotten it working with the npm run dev command, and verified that it works ingame, when the game is running in Steam using CrossOver. I've only verified it in dev mode.

I need someone to verify that it still works in Windows.

The solution itself should be self-evident: If there's no accessibility permissions, it waits 15 seconds for it to be granted, polling every second.


Fixes #427

@Kvan7 Kvan7 changed the base branch from master to dev February 17, 2025 22:11
@Kvan7
Copy link
Owner

Kvan7 commented Feb 18, 2025

I'm guessing you've looked at #215

This will need a few code changes for working on windows but I think this makes sense to me overall. couple questions though:

  1. What happens if the user is in settings from the prompt trying to give access and the 15 seconds expires then they give access after the program has closed in the background, are we back in the same situation as Does not run on macos #215? not saying to increase it yet but that may be an outcome.
  2. If the program is restarted, does the change in accessibility settings persist? Just since it was mentioned that changing via that prompt would cause it to get added but you had to remove and then re add to the allow list for it to actually work.

@larssn
Copy link
Author

larssn commented Feb 19, 2025

  1. If they are too slow, the program will exit. I don't think there's a way to listen for the permission request.
  2. It does persist, that's why our first check uses false as an argument. This avoids the popup. This also answers your question in 1.

@larssn
Copy link
Author

larssn commented Feb 19, 2025

Not sure if it fixes #215 tho. Seems similar, but they had no crash

@larssn
Copy link
Author

larssn commented Feb 19, 2025

I tried making a release bundle using the instructions (yarn), and it also works. 😃

@Kvan7
Copy link
Owner

Kvan7 commented Feb 19, 2025

I will probably bump the timeout to like 5-10 minutes and the polling up to maybe 10 seconds.
one followup for q1. Can you check the specific scenario for a user gets the popup, they hit the button to go into settings to change it, while settings is still open the timeout(15 seconds) expires, then then in the settings that is open they do actually grant permissions. Trying to check that if the user grants permissions but the program quits/crashes while they are doing that, the permissions they grant still apply when the program is restarted.

@Kvan7
Copy link
Owner

Kvan7 commented Feb 19, 2025

Mainly checking this specific case, since that seems to be the same problem between #215 and #427, ei the user gets a popup to ask for permissions and goes and gives permissions, but because the program closed, for some reason that causes the permissions to not get granted maybe?

@larssn
Copy link
Author

larssn commented Feb 19, 2025

Remember we're waiting to start the program. Polling every 10 seconds is very slow, once per second is not resource intensive - if that's your concern. Also keeping the program open for 5-10 minutes, waiting for permissions, while the program does absolutely nothing might be confusing for the user. A compromise would be: wait for 30 seconds, if, after 30 seconds, there's still no permission display a popup explaining the situation and gracefully quit.

I can make that if we agree on it.

Can you check the specific scenario for a user gets the popup

I did as you requested, and it still works. 🙂

In any case, the best test would be for the user in question to get a new build in their hands, to see for themselves. I have no reason to believe it wouldn't work for him, as our issues were so similar.

@Kvan7
Copy link
Owner

Kvan7 commented Feb 23, 2025

Ok I think I'm going to merge this then. I'm going to move stuff around a tiny bit, I don't think having the whole thing in an async arrow will cause issues but just as a caution since I'm not super familiar with this part of the code, I'm going to move this to another function and make it only run async on mac. I'll ping you once I change that so you can test to confirm it still works

@larssn
Copy link
Author

larssn commented Feb 24, 2025

Up to you, but the async code shouldn't cause any issues. But I can relate to being cautious. 🙂

Let me know. I'll test it.

@Kvan7
Copy link
Owner

Kvan7 commented Feb 27, 2025

may be pushed back until next release after v0.8.0, been playing too much poe1 and major poe2 update for me just dropped so i need to push a fix

Mac calls that in async, other platforms proceed in sync.
@Kvan7
Copy link
Owner

Kvan7 commented Feb 28, 2025

@larssn Let me know if this change works, I just moved the main startup stuff (everything after the accessibility check you added) into a function, then called that asynchronously when on mac and synchronously otherwise.

@larssn
Copy link
Author

larssn commented Mar 3, 2025

I'll give it a look later today

@larssn
Copy link
Author

larssn commented Mar 4, 2025

I've tested it and there are 2 problems now:
Since we're listening for app readiness using app.on("ready", ...), this event has already fired when we're done granting accessibility permissions. I've modified the code to await for it to be ready. I had to go back on your change to not have async functions because of this. We're using a supported API call though. Electron has already thought about asynchronous initialization functions it seems.

Second problem is that when I get EE2 running, it no longer works in-game. There's no notification popup anymore, that EE2 is running, nor does any of the shortcuts work.

I've tried the usual: doing the code sign again, and re-adding the accessibility entry. Didn't help.

Unless you have any suspicions about what is going on here, I'll have to dig deeper, but I'm pretty busy this week.

Thanks! 😊

@Kvan7
Copy link
Owner

Kvan7 commented Mar 4, 2025

Fixed merge conflict.

Second problem is that when I get EE2 running, it no longer works in-game. There's no notification popup anymore, that EE2 is running, nor does any of the shortcuts work.

So just clarifying, this is after the onready has been fixed it just doesn't work now. gotta go to work, so either way I'll look onto the diff from what you originally had tonight

@larssn
Copy link
Author

larssn commented Mar 4, 2025

Correct, it doesn't work, even after I've made the startup fix.

@Kvan7
Copy link
Owner

Kvan7 commented Mar 4, 2025

not urgent since you said you were busy this week

Not quite sure why the previous one didn't work, made a change, let me know if you want me to keep trying stuff or if you want to try on your end.

@larssn
Copy link
Author

larssn commented Mar 11, 2025

I think I'll have time to test it today. If it still doesn't work, I'll tackle it. Doesn't make sense for you to blindly go through all the changes.

@larssn
Copy link
Author

larssn commented Mar 11, 2025

It works! However a caveat: It only works if you reset with codesign and start the app while path of exile is running. If you run it before, it doesn't seem to detect when the game is launched.

Minor issue I suppose.
Good work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mac - crash on startup
2 participants