search

Adamcake / Bolt

An alternative launcher for your favourite MMO
GNU Affero General Public License v3.0
122 stars 18 forks source link

Change window close logic to prevent hung process #16

Closed Jertzukka closed 5 months ago

Jertzukka commented 5 months ago

Currently whenever we arrive at OnBeforeClose(), is_closing is always false and I believe closing_windows_remaining can never be 0 here, so the check against it will always fail. This means that we never actually get into the Exit() path which closes the application eventually. This can be easily tested by running the application in a terminal, closing all the GUI windows and observing that the program stays running with bunch of processes until killed.

I tried understanding the current logic, but couldn't really figure out what was intended with the closing_windows_remaining part. I had my hand at adapting it to what made sense to me, and it seems to work fine. Though I don't have too much experience with CEF so take it with a grain of salt.

So with this PR, DoClose() logic is not changed, it removes the closed window from the windows vector and OS closes the window. OnBeforeClose() which is called next checks whether the windows vector is empty, if there are still any windows left, we don't continue to Exit(). Exit() is only called when the application is fine to quit, and this eventually calls CefQuitMessageLoop() that quits the app.

After this PR CountBrowsers() is left unused. If you think there's a better way to fix the initial issue, take this as a bug report instead.

Adamcake commented 5 months ago

The intention right now is for Exit() to be called by the user right-clicking the tray icon and choosing "Exit" (implemented in main.cc). The reason for this is because I'll want Bolt running in the background to act as a CEF browser host for the currently-in-progress helper feature for RS3, similar to alt1 if you know of that. Your PR would instead cause the whole process to stop when the launcher window is closed.

This would be unclear if the user doesn't have a system tray, though, which I guess is your situation?

Adamcake commented 5 months ago

I think I'll have the process exit when there are no windows open and no games depending on it. I'll also need to change what happens when the user runs Bolt while it's already running in the background, to make a launcher window open on the existing process.

Then I can get rid of the tray icon and all the deprecated gtk stuff.

Jertzukka commented 5 months ago

That's a good point. I was testing this on my Ubuntu VM which I don't think has a tray, so I never though that it was used to close the app. I thought it to be reasonable that closing all launcher windows would also close the launcher process, but I wasn't aware of your other plans for the background process. I tested it for my use case where closing the main window closes the launcher, and launching Runelite and closing launcher keeps the Runelite running.

Adamcake commented 5 months ago

That's okay. Not every Linux user uses a DE or has a tray, and it was remiss of me not to think about how that would work. So I'll try to come up with a full fix for this so I can remove the tray icon.