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

Add custom window / native window controls #457

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

webfiltered
Copy link
Contributor

@webfiltered webfiltered commented Dec 10, 2024

Modern linux / win32 window

Example implementation:

image

windowStyle.mp4
// Configured in config.json
"windowStyle": "default" | "custom"

┆Issue is synchronized with this Notion page by Unito

@webfiltered webfiltered changed the title Add native window controls Add custom window / native window controls Dec 10, 2024
Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

Can you also add a UI screenshot when menu location is set to Bottom?

@webfiltered
Copy link
Contributor Author

I have not handled that case here.

If we want to support bottom menu on desktop, we will need another title bar - having the window drag from the bottom is not .. really an option from a UX perspective.

I'll use the original window w/default Windows titlebar. I'll have to add something in to recreate the window (or require desktop restart).

@huchenlei
Copy link
Member

  • Add desktop custom window / native window controls ComfyUI_frontend#1856

Ok then we probably want to disallow "Bottom" location of menu bar on electron app.

@webfiltered
Copy link
Contributor Author

@huchenlei Fixed the broken logic in this PR - it now just does what frontend tells it to, instead of forcing new default behaviour.

Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

There doesn't seem to be a write to windowStyle. Can you explain how is the store value initialized and updated?

@webfiltered
Copy link
Contributor Author

webfiltered commented Dec 14, 2024

There doesn't seem to be a write to windowStyle. Can you explain how is the store value initialized and updated?

There isn't one. That flag was for troubleshooting only.

This PR was originally designed to simply replace the title bar, without any on/off switches. Allowing this requires destroying and recreating the app window; it would either require restarting the app, or a bit of re-architecting.

Fixing this right now is not optimal. This PR will have to wait until other changes are in.

Fix enum imported via meta file
Limitation: Cannot theme hover background of OS-controlled buttons - using a white background in dark mode appears to have no hover
Removes initial test height
It resets the height of the buttons, but the CSS is set explicitly to zero, breaking anything relying on it.
- Removes on-by-default behaviour, leaving no changes to default behaviour
- Allows linked commits to happen async
@webfiltered webfiltered marked this pull request as ready for review December 26, 2024 17:48
@webfiltered webfiltered requested a review from a team as a code owner December 26, 2024 17:48
@webfiltered
Copy link
Contributor Author

Previous comments are all resolved; API is complete, no changes to default behaviour. Impl. detail / UX can be handled entirely in frontend.

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.

2 participants