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

Set appid/wmclass for zed window #10909

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

jakobhellermann
Copy link
Contributor

fixes #9132

By setting the app id, window managers like sway can apply custom configuration like for_window [app_id="zed"] floating enable.
Tested using wlprop/hyprctl activewindow for wayland, xprop for x11.

Release Notes:

  • Zed now sets the window app id / class, which can be used e.g. in window managers like sway/i3 to define custom rules

Copy link

cla-bot bot commented Apr 23, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @jakobhellermann 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'.

Comment on lines +461 to +501
let mut data = Vec::with_capacity(app_id.len() * 2 + 1);
data.extend(app_id.bytes()); // instance https://unix.stackexchange.com/a/494170
data.push(b'\0');
data.extend(app_id.bytes()); // class
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The x11 WM_CLASS property contains two null-terminated strings, often referred to as "instance"/"class".
In this PR I just wrote a single set_app_id function on the PlatformWindow that sets both to the same value since as far as I can tell x11 is the only platform on which this is relevant.

https://tronche.com/gui/x/icccm/sec-4.html#WM_CLASS

@jakobhellermann
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Apr 23, 2024
Copy link

cla-bot bot commented Apr 23, 2024

The cla-bot has been summoned, and re-checked this pull request!

crates/collab_ui/src/collab_ui.rs Outdated Show resolved Hide resolved
crates/zed/src/zed.rs Outdated Show resolved Hide resolved
@mikayla-maki
Copy link
Contributor

Looks like the tests end up calling this function, I'd suggest not using unimplemented!() ever, just let it be a noop :)

@jakobhellermann
Copy link
Contributor Author

The new test failure is in test_chunk_text_grammar (and only on mac) so nothing caused by these changes.

@mikayla-maki
Copy link
Contributor

so nothing caused by these changes

That seems incorrect. The test is relying on text length checks, and this PR modifies the sample text. You should remove that change or recalculate the values. macOS is the only CI check that actually runs the tests right now, due to crashes in other implementations.

@mikayla-maki mikayla-maki marked this pull request as draft April 26, 2024 20:51
@mikayla-maki
Copy link
Contributor

Mark it for review once that test is fixed!

@jakobhellermann
Copy link
Contributor Author

That seems incorrect. The test is relying on text length checks, and this PR modifies the sample text. You should remove that change or recalculate the values. macOS is the only CI check that actually runs the tests right now, due to crashes in other implementations.

Oh right, I totally forgot I changed that test.

I've changed the identifier to dev.zed.Zed and fixed that test.

@jakobhellermann
Copy link
Contributor Author

jakobhellermann commented Apr 27, 2024

There's still a CI failure during cargo xtask clippy on linux that I can't seem to reproduce or understand.

It's complaining about

no field `xcb_connection` on type `&mut platform::linux::x11::window::X11Window`

on line 503, suggesting self.0.xcb_connection,

but

  1. the code should be actually at line 466. Line 503 is unrelated
  2. the other code like set_title also directly calls self.xcb_connection.
  3. X11Window has the fields xcb_connection and x_window

not sure what's going on there...

@apricotbucket28
Copy link
Contributor

@jakobhellermann It's a conflict with #11008, you just need to rebase and it should be good to go 🙂

@jakobhellermann
Copy link
Contributor Author

rebased

@jakobhellermann jakobhellermann marked this pull request as ready for review April 27, 2024 12:17
@mikayla-maki
Copy link
Contributor

Thank you!

@mikayla-maki mikayla-maki merged commit 2386ae9 into zed-industries:main Apr 29, 2024
8 checks passed
@jakobhellermann jakobhellermann deleted the window-appid-class branch April 29, 2024 17:28
mrnugget added a commit that referenced this pull request May 3, 2024
This has to match the WMClass/AppID, which was added here: #10909
mrnugget added a commit that referenced this pull request May 3, 2024
This has to match the WMClass/AppID, which was added here: #10909

Release Notes:

- N/A
@Moshyfawn Moshyfawn mentioned this pull request Jun 7, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give zed window class
3 participants