-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Implement Window Controls Overlay #8707
Conversation
Thank you! Might be a stupid question, but what are the advantages of WCO besides it being native? PS issue 1: you'd need to watch for config changes and respond to them in |
Not a stupid question at all! That's exactly its advantage. It unifies window control implementations across macOS and Windows. IIRC setting |
@Eugeny Pinging for the latest update - the WCO colors now respect the theme that you're using :) electron_pxyW3eUJHX.mp4Also, apologies for the messy commits below. Trying to rapidly iterate and it's evidently not the wisest thing to do right now. This is practically finished IMO, just waiting for your review. |
After a day of using a built version of the last commit here, I can say it's working quite smoothly now. Not sure how to resolve the CI errors, but I'm sure it's nothing a re-run can't fix. No rush to merge or anything :) Edit: it looks like we're going to be all green following this merge! |
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.
Added a few comments, otherwise LGTM ✌️
app/lib/window.ts
Outdated
@@ -94,12 +94,19 @@ export class Window { | |||
} else { | |||
if (process.platform === 'darwin') { | |||
bwOptions.titleBarStyle = 'hidden' | |||
// If not macOS and native appearance is not toggled, use WCO. | |||
} else { |
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.
Does WCO work on Linux? This code path implies it does, the comment in titleBar.component.scss implies it doesn't.
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.
It doesn't, but good catch, will fix
app/lib/window.ts
Outdated
@@ -94,12 +94,19 @@ export class Window { | |||
} else { | |||
if (process.platform === 'darwin') { | |||
bwOptions.titleBarStyle = 'hidden' | |||
// If not macOS and native appearance is not toggled, use WCO. | |||
} else { | |||
bwOptions.titleBarStyle = 'hidden' |
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.
can be moved out of the if
app/lib/window.ts
Outdated
} | ||
} | ||
|
||
if (process.platform === 'darwin') { | ||
this.window = new BrowserWindow(bwOptions) as GlasstronWindow | ||
} else { | ||
// this.window = new BrowserWindow(bwOptions) as GlasstronWindow |
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.
Please clean up commented code across the PR
@@ -1,16 +1,20 @@ | |||
title-bar( | |||
*ngIf='ready && !hostWindow.isFullscreen && config.store.appearance.frame == "full" && config.store.appearance.dock == "off"', | |||
(dblclick)='hostWindow.toggleMaximize()', | |||
[class.hide-controls]='hostApp.platform !== Platform.Linux && !hostWindow.isFullscreen', |
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.
should be controlled via an @input on TitleBarComponent, not a class
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.
Done in the latest commit, window controls in the titlebar component is an ngIf now ✨
I believe that resolves your comments :) |
Tested on all 3 platforms and it works nicely - well done! @all-contributors add @ferothefox for code |
I've put up a pull request to add @ferothefox! 🎉 |
Hi @Eugeny, @ferothefox :) I encountered an error while working on tabby with the master branch and I think it has something to do with this PR (I'm on Linux):
Lines 390 to 403 in 0101ffd
After a quick look in the electron doc, it seems that the function |
Hey, normally when Electron docs refer to functions as only available on platform X, that means it's a noop on other platforms (also supported by the electron TS typings). I've seen it too, but the error by itself is harmless as the setTitleBarOverlay call is at the very end of the event handler. I'm waiting to see if it's going to be fixed in the electron updates and if it's not, I'll just replace it with |
Oh I see, I was not sure what was the impact exactly so, in doubt, i preferred report this. |
Fixes: #7199 #8650 #6278 #4292 (on Windows)
Hey @Eugeny, I've daily-driven Tabby for a little while and I love its customizability. It reminds me of Vivaldi in a lot of ways! This is my first PR here and I'd love to try to fix a long-standing issue with the tab bar location and the window controls.
This PR aims to mitigate this issue by replacing the custom window controls with the Window Controls Overlay API that Electron provides.
Specifically, my code:
Known issues
I've tested this throroughly on Windows 11, but there's some other issues with this implementation that need to be addressed in order for this to be considered production-ready.
Issue 1
here's a visual example:
![image](https://private-user-images.githubusercontent.com/52982404/253498265-8891618a-ba00-46a2-aff5-9cbbfb85aef5.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQzNzY4NjUsIm5iZiI6MTcyNDM3NjU2NSwicGF0aCI6Ii81Mjk4MjQwNC8yNTM0OTgyNjUtODg5MTYxOGEtYmEwMC00NmEyLWFmZjUtOWNiYmZiODVhZWY1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIzVDAxMjkyNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWFjZTZiM2I3ODIwY2E0N2NlYmY1NGFhNTdkZThjN2EwNDliMmY3NmFhMWMwNGRkOWRkYzIyN2UyOTE0MWI3MjQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.suZtTmBnhuBFudw-Av5Lv4uSwok4TLMnseqe_Y4vK98)
Dark themes work flawlessly, however in lighter themes...
It is impossible to know the window buttons exist, even when hovering directly over them.
We'd need to implement a way to pass the currently applied theme's colors to Electron (probably via IPC) and update the colors of the window controls accordingly so that they don't cause issues like this.
Issue 2
Issue 3
Other than that, I'm proud of how this turned out!
![electron_un4bK8oc2L](https://private-user-images.githubusercontent.com/52982404/253498772-8dd1aa20-9cab-4af8-87aa-057d010e5464.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQzNzY4NjUsIm5iZiI6MTcyNDM3NjU2NSwicGF0aCI6Ii81Mjk4MjQwNC8yNTM0OTg3NzItOGRkMWFhMjAtOWNhYi00YWY4LTg3YWEtMDU3ZDAxMGU1NDY0LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIzVDAxMjkyNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWQzZTQzYmEzMjRiMmFlNDdmOTIyNWFmZTViMGFiMmZlYjU3MTM2Y2ZiZWZhNzNiNTM4MTA1MDgyYzgyMzMyYjMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.W9JIPk87NFh9dsxs_cO8z4F-lGIcOAKtpb4ALbnlqVM)