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

Allow linux users manipulate wm_class and wm_instance of windows #4609

Closed
wants to merge 7 commits into from

Conversation

0zitro
Copy link

@0zitro 0zitro commented Apr 27, 2022

Objective

Allow passing WM_CLASS to windows via WindowDescriptor

Solution

I changed WindowDescriptor to allow initialisation with WM_CLASS and WM_INSTANCE

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 27, 2022
@alice-i-cecile alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in O-Linux Specific to the Linux desktop operating system and removed S-Needs-Triage This issue needs to be labelled labels Apr 27, 2022
Comment on lines 624 to 627
#[cfg(any(feature = "x11", feature = "wayland"))]
desktop_id: "app".to_string(),
#[cfg(feature = "x11")]
desktop_instance: "Main".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(any(feature = "x11", feature = "wayland"))]
desktop_id: "app".to_string(),
#[cfg(feature = "x11")]
desktop_instance: "Main".to_string(),
#[cfg(all(
any(
target_os = "linux",
target_os = "freebsd",
target_os = "netbsd",
target_os = "openbsd"
),
any(feature = "x11", feature = "wayland")
))]
desktop_id: "app".to_string(),
#[cfg(all(
any(
target_os = "linux",
target_os = "freebsd",
target_os = "netbsd",
target_os = "openbsd"
),
feature = "x11"
))]
desktop_instance: "Main".to_string(),

Comment on lines 130 to 141
/*
https://github.com/rust-windowing/winit/blob/ea1c031b54438e64576353b288848c07d2068214/src/platform/unix.rs#L337
337| fn with_class(self, class: String, instance: String) -> Self;
https://github.com/rust-windowing/winit/blob/ea1c031b54438e64576353b288848c07d2068214/src/platform/unix.rs#L383
383| fn with_class(mut self, instance: String, class: String) -> Self {

Because of type equality someone wrote those wrong and Rust didn't catch it!
rust-analyser uses only trait declaration, therefore it shows wrong parameter names

This bug persists not only in 0.26.0, but also in 0.26.1
The next update will be breaking, no more differentiation between x11 class and wayland appid
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I needed to repeatedly read over this comment to understand why it was added.

Either this comment or another comment inside the with_class call should have an actionable Instrucction.

// TODO: Switch these two calls once winit releases 0.27 or later.

With such a comment, the rest should be much clearer.
Perhaps also prepend the URLs with Trait and Trait Impl.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

It unifies these two methods and in wayland case, the instance field will be noop.

Copy link
Author

Choose a reason for hiding this comment

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

This comment explains the issue with misleading argument names, which led to me searching the internet for correct variant.

Comment on lines 597 to 600
#[cfg(any(feature = "x11", feature = "wayland"))]
pub desktop_id: String,
#[cfg(feature = "x11")]
pub desktop_instance: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(any(feature = "x11", feature = "wayland"))]
pub desktop_id: String,
#[cfg(feature = "x11")]
pub desktop_instance: String,
#[cfg(all(
any(
target_os = "linux",
target_os = "freebsd",
target_os = "netbsd",
target_os = "openbsd"
),
any(feature = "x11", feature = "wayland")
))]
pub desktop_id: String,
#[cfg(all(
any(
target_os = "linux",
target_os = "freebsd",
target_os = "netbsd",
target_os = "openbsd"
),
feature = "x11"
))]
pub desktop_instance: String,

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I changed platform configuration, now committing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it restricted to linux and {free,net,open}bsd? Is it not supported on macOS with X for example? Or on other systems using X natively?

@0zitro
Copy link
Author

0zitro commented Apr 27, 2022

So, I think all issues regarding OS conflicts should be resolved by now.

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Apr 27, 2022
@bors
Copy link
Contributor

bors bot commented Apr 28, 2022

try

Timed out.

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Apr 28, 2022
@Weibye
Copy link
Contributor

Weibye commented Jun 6, 2022

So what exactly will this change allow you to do? What is WM_Class on a window in linux?

@bjorn3
Copy link
Contributor

bjorn3 commented Jun 6, 2022

wm_class is supposed to uniquely identify the program that created the window I believe. This allows users to tell their window manager to say always open such windows in the top left corner. Or it could allow the window manager to restore the position from the last time you opened the window. See WM_CLASS in https://www.x.org/docs/ICCCM/icccm.pdf for the official specification.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, but I don't have a lot of expertise here or the ability to test it.

@lovelymono
Copy link
Contributor

lovelymono commented Oct 12, 2022

We should probably wait for #5347 to be merged first, and update this to use with_name on both X11 and Wayland.

@mockersf
Copy link
Member

done in #7650

@mockersf mockersf closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in O-Linux Specific to the Linux desktop operating system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants