frontend: Skip saveAll after closeWindow#13346
frontend: Skip saveAll after closeWindow#13346umireon wants to merge 2 commits intoobsproject:masterfrom
Conversation
On the exit of OBS frontend, commitData can be called after closeWindow. Since closeWindow releases OBSBasic's resources, saveAll on commitData may write corrupted configuration. For instance, creating a profile, connecting to YouTube, and closing the OBS frontend immediately results in removing Auth.Type in the profile's basic.ini. This is problematic for users because they cannot persist their connection with some specific procedure. This change adds a guard to prevent commitData from saving all configuration when the instance of OBSBasic is already released. This is safe because closeWindow is also responsible for saveAll. Signed-off-by: Kaito Udagawa <umireon@kaito.tokyo>
|
Please do not request AI reviews on our repo. This calls in to question that this change was also AI-generated. Can you please confirm that this PR is not AI-assisted? |
|
@Fenrirthviti I'm sorry, my user profile on GitHub is misconfigured and AI review is automatically requested. I've just turned off that settings.
I confirm that the whole content of this Pull Request including code, PR description, and commit message are written by me and contains no AI-generated contents. I apologize to behave in the way not aligning the project policy, but this is not intentional and I won't do this again. Thanks, |
|
The issue is correctly identified, mainly that the duplicate call to Any saving of user data or application state should take place in Only when the user does not cancel it (or the application does not intend the user to have a say in this) is a So what happens on macOS is that The core issue here is that OBS treats the main window as "the application" and ties up a whole lot other state to its lifetime even though that's not how Qt is designed to handle it rather than simply storing its internal window state before simply emitting the So what happens currently is that when you:
Then Only if you quit OBS by closing the main window will Indeed it's the design of |
|
@umireon Would you be willing to test a different "hypothesis" if you will? @Warchamp7 and I discussed the different wrinkles of shutdown on Windows and macOS and the "trigger" for
My hypothesis is that we only want one trigger to succeed, but it also needs to be the first caller that succeeds. So we could potentially add a guard inside If you could implement that and test it with your use case (as you got the environment in which it goes "wrong" available already), that'd be very helpful. |
|
@PatTheMav Thank you for your great feedback! I have both a macOS and Windows environment and I can implement and verify your idea works. Once I finish, I will push and make this PR ready. |
Description
On the exit of OBS frontend, commitData can be called after closeWindow. Since closeWindow releases OBSBasic's resources, saveAll on commitData may write corrupted configuration.
This change adds a guard to prevent commitData from saving all configuration when the instance of OBSBasic is already released. This is safe because closeWindow is also responsible for saveAll.
Motivation and Context
Creating a profile, connecting to YouTube, and closing the OBS frontend immediately results in removing Auth.Type in the profile's basic.ini. This is problematic for users because they cannot persist their connection with some specific procedure.
How Has This Been Tested?
I have built it on my head branch on macOS Tahoe (arm64). I confirmed that create, connect, close flow saves correct configuration, and it is fixed to persist YouTube connection between runs.
Types of changes
Checklist: