-
Notifications
You must be signed in to change notification settings - Fork 15k
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
feat: enable Windows Control Overlay on Linux #41769
base: main
Are you sure you want to change the base?
Conversation
7170094
to
8d1d279
Compare
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.
API LGTM
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.
API LGTM
Sorry for the delay here, need some time to verify the changes. I will have an update later this week. |
API LGTM |
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.
Works great, left feedback on the frame border.
// px on each side regardless of the system window border size. This is | ||
// overridable by subclasses, so RestoredFrameBorderInsets() should be used | ||
// instead of using this constant directly. | ||
static constexpr int kFrameBorderThickness = 4; |
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.
This causes a think frame border for resizing outside the client area in window restored mode, also this seems chrome UI specific. Can we remove it ?
There is also the issue with theme adjustment with the current implementation, custom titlebar: I believe the issue arises from the class used to draw the caption buttons @ruihe774 since you have been working on this support for wayland, do you have thoughts on the above ? My question is mainly if the window caption container code can be shared between wayland and x11. |
Yes. I've also used |
Missed that, thanks for clarifying! If that works well for both x11 and wayland then seems like a better route. @codebytere thoughts ? |
I would mostly be OK with either this or 41840. Both PRs have strengths and weaknesses; so if any other maintainer has strong feelings about which to use, TBH I could go along with either. The only blockers on this PR IMO are the merge conflicts and |
Co-authored-by: Erick Zhao <erick@hotmail.ca>
8c9d1fe
to
789e9ae
Compare
Description of Change
Closes #23665.
This PR enables the Windows Control Overlay API on Linux. To do so, we create a new subclass of
FramelessView
. This primarily follows logic from upstream's implementation with modifications for our use case and stripped of logic irrelevant to Electron.cc @bpasero @deepak1556
Todo:
Functionality on Wayland will be approached as a follow-up as I don't currently have the requisite hardware to test this.
WCO RTL
WCO LTR
Dynamic WCO Update
Gif Demonstration with the following code:
Checklist
npm test
passesRelease Notes
Notes: Enabled the Windows Control Overlay API on Linux.