-
Notifications
You must be signed in to change notification settings - Fork 211
Hide ComfyUI instead of quitting when window is closed #1206
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
Closed
+192
−16
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
482c7d9
Hide ComfyUI instead of quitting when window is closed
crowecawcaw ddc55a6
Merge branch 'main' into minimize
crowecawcaw ca17463
Merge branch 'main' into minimize
crowecawcaw 90af9c4
Put close-to-background behind a new setting
crowecawcaw b09cf6f
Apply suggestions from code review
crowecawcaw cd0cae3
Apply suggestions from code review
crowecawcaw 99ecd76
Clean up unit tests
crowecawcaw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the setting is updated in frontend the state should be sent to the desktop process and kept synchronised (e.g.
AppState). IPC is mostly handled inpreload.ts,constants.ts.Dynamically calling settings every time performs a round-trip IPC call to a single-threaded browser process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new IPC handler with logic on both sides seems a bit over-engineered to me. Even if we read the setting every time, seems like it'd be fast enough for an infrequent even. Other code seems to be using a similar approach.
If we did want to IPC in here, it might make sense to make a generic settings syncing mechanism that would transfer the whole set of settings from the frontend whenever it's updated, then the desktop app can just read the local settings copy. That's a bit out of scope of this PR though.
Let me know your thoughts. If you still prefer IPC syncing, I can add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for taking the time to respond. I've been out of action for a bit.
For this one, it's more about how the app should work, and preventing weird error states/lockups by just using proper separation of concerns. Using code running in a browser process (which isn't always available) as the runtime memory storage for the parent process' state is just too circular and brittle.
p.s. If you can point me at the other code, I'll add it to the issues list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking closer at this,
useSettings().get('key')looks like it just reads a key from an in-memory object: https://github.com/Comfy-Org/desktop/blob/main/src/config/comfySettings.ts#L153So to make this work correctly, I'm thinking I'll add a new IPC handler for
reloadComfySettings. The frontend will call after updating a setting, and in response the backend code will reread the settings file from disk. This mechanism will work more broadly with other settings that the frontend updates which need to be synced to the server.Does this sound right to you? It would not touch AppState and would continue to use
useSettings(). I might have missed a detail of the implementation or misunderstood what you were pointing out.