-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add setting for enabling server-side decorations #39250
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
base: main
Are you sure you want to change the base?
Conversation
We require contributors to sign our Contributor License Agreement, and we don't have @Be-ing on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
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.
Looks good! Just one note about compatibility and a question about loading timing :)
cx.update(|cx| { | ||
let app_id = ReleaseChannel::global(cx).app_id(); | ||
let bounds = Bounds::centered(None, size(px(1024.0), px(768.0)), cx); | ||
let window_decorations = match std::env::var("ZED_WINDOW_DECORATIONS") { |
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 we keep both of these? That way people currently using the ZED_WINDOW_DECORATIONS variable will continue to function.
The environment variable should override the setting :)
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.
We could keep the environment variable. Is it worth it though? As far as I can tell, the environment variable was never documented; I only found out about it by reading #14165. Yes, it's a breaking change for people who set up the environment variable already, though I doubt they'd have a problem editing their settings.json once and be done with 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.
I think it's good practice to not break things, and it doesn't add much complexity :)
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.
Okay, done.
crates/zed/src/zed.rs
Outdated
Ok(val) if val == "server" => gpui::WindowDecorations::Server, | ||
Ok(val) if val == "client" => gpui::WindowDecorations::Client, | ||
_ => gpui::WindowDecorations::Client, | ||
let window_decorations = match WorkspaceSettings::get_global(cx).window_decorations { |
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.
Have the settings been setup at this point?
Also, we should continue to respect the environment variable even with the presence of this setting :)
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.
Yes, I've tested editing my settings.json and restarting Zed and it works. There's also code right below this that reads a different setting.
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.
Perfect!
Testing this on GNOME Wayland, I noticed that Zed wasn't drawing window min/max/close buttons when the user requested SSD. Fixed in #39313 |
38dbecd
to
5e79fc2
Compare
Previously, this was controllable via the undocumented ZED_WINDOW_DECORATIONS environment variable (added in zed-industries#13866). Using an environment variable for this is inconvenient because it requires users to set that environment variable somehow before starting Zed, such as in the .desktop file or persistently in their shell. Controlling this with a Zed setting is more convenient. The environment variable is kept for backwards compatibility. This does not modify the design of the titlebar in any way. It only moves the existing option from an environment variable to a Zed setting. Fixes zed-industries#14165
Previously, this was controllable via the undocumented ZED_WINDOW_DECORATIONS environment variable (added in #13866). Using an environment variable for this is inconvenient because it requires users to set that environment variable somehow before starting Zed, such as in the .desktop file or persistently in their shell. Controlling this via a Zed setting is more convenient.
This does not modify the design of the titlebar in any way. It only moves the existing option from an environment variable to a Zed setting.
Fixes #14165
Client-side decorations (default):

Server-side decorations in KDE Plasma:

Release Notes: