Allow linux users manipulate wm_class and wm_instance of windows by 0zitro · Pull Request #4609 · bevyengine/bevy

0zitro · 2022-04-27T01:32:30Z

Objective

Allow passing WM_CLASS to windows via WindowDescriptor

Solution

I changed WindowDescriptor to allow initialisation with WM_CLASS and WM_INSTANCE

MinerSebas · 2022-04-27T10:06:17Z

+            #[cfg(any(feature = "x11", feature = "wayland"))]
+            desktop_id: "app".to_string(),
+            #[cfg(feature = "x11")]
+            desktop_instance: "Main".to_string(),


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(),

MinerSebas · 2022-04-27T10:17:22Z

+        /*
+        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
+        */


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.

In winit 0.27 https://github.com/rust-windowing/winit/blob/ce890c34551d9fb542f10dcb644d22d382e0c921/src/platform/unix.rs#L288 the correct method will be with_name

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

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

MinerSebas · 2022-04-27T10:03:20Z

+    #[cfg(any(feature = "x11", feature = "wayland"))]
+    pub desktop_id: String,
+    #[cfg(feature = "x11")]
+    pub desktop_instance: String,


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,

Okay, I changed platform configuration, now committing.

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 · 2022-04-27T14:30:01Z

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

alice-i-cecile · 2022-04-27T19:37:36Z

bors try

bors · 2022-04-28T08:50:01Z

try

Timed out.

alice-i-cecile · 2022-04-28T18:09:47Z

bors try

bors · 2022-04-28T18:30:21Z

try

Build succeeded:

Weibye · 2022-06-06T15:31:38Z

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

bjorn3 · 2022-06-06T17:01:59Z

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.

alice-i-cecile

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

lovelymono · 2022-10-12T17:49:14Z

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

mockersf · 2024-02-27T13:40:00Z

done in #7650

Zitro added 3 commits April 27, 2022 04:13

Allow linux users manipulate wm_class and wm_instance of windows

63c0e84

polishing code

9840deb

Fixing up OS conflicts

4b640be

github-actions Bot added the S-Needs-Triage This issue needs to be labelled label Apr 27, 2022

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

MinerSebas reviewed Apr 27, 2022

View reviewed changes

Fix OS conflicts once more

a9ab5cc

bors Bot added a commit that referenced this pull request Apr 27, 2022

Try #4609:

75946c2

bors Bot added a commit that referenced this pull request Apr 28, 2022

Try #4609:

bdaaf43

Clarify WM_CLASS initialisation comment

3b81996

Zitro and others added 2 commits April 28, 2022 22:42

Fix unintentional doc comment

3246b79

Merge branch 'bevyengine:main' into main

763c108

alice-i-cecile reviewed Oct 10, 2022

View reviewed changes

mockersf closed this Feb 27, 2024

-            #[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(),

Uh oh!

Conversation

0zitro commented Apr 27, 2022

Objective

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0zitro commented Apr 27, 2022

Uh oh!

alice-i-cecile commented Apr 27, 2022

Uh oh!

bors Bot commented Apr 28, 2022

try

Uh oh!

alice-i-cecile commented Apr 28, 2022

Uh oh!

bors Bot commented Apr 28, 2022

try

Uh oh!

Weibye commented Jun 6, 2022

Uh oh!

bjorn3 commented Jun 6, 2022

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

lovelymono commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockersf commented Feb 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lovelymono commented Oct 12, 2022 •

edited

Loading