Allow using multiple XWindowTypes on X11 (#1140)

[Toqozz]

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

---

I'm not familiar with rustdoc. Does documentation need to be updated manually for this change? Note that the change is breaking due to the case change of XWindowType.

get_atoms() is quite ugly, but seems necessary.

Please let me know of any changes/improvements necessary and I'll update as soon as I can.

[Osspial]

Thanks for the PR!

This largely looks good to me (except for the build target things below), but keep in mind that I'm not familiar with the Linux code. I've requested a review from @zegentzy who's more familiar with that backend, mainly because I'm not comfortable merging code that I can't properly review, but I'd be surprised if they find an issue.

[goddessfreya]

I'm not liking this solution, the specs say:

The Client SHOULD specify window types in order of preference (the first being most preferable) but MUST include at least one of the basic window type atoms from the list below.

This bit flags thing stops users from specifying their order. I'd rather we keep the old enum WindowType and instead accept a T: AsRef<[utils::WindowType]> in set_window_type.

[Toqozz]

How would this change face the user? -- with_x11_window_type().

[goddessfreya]

Changed my mind on the AsRef, but we should still be accepting an slice of them.

So:
fn with_x11_window_type(self, x11_window_type: &[utils::WindowType]) -> WindowBuilder
fn set_window_type(&self, window_type: &[util::WindowType]) -> util::Flusher<'_>

and so forth.

[Toqozz]

I've pushed the slice variant. A static lifetime seems like the neatest way to do this, but stops users adding window types dynamically -- which is fine, IMO. I admit I'm not too confident with lifetimes so maybe this has worse consequences.

Something I noticed is that the window type is not set at all if the window type is the default:

winit/src/platform_impl/linux/x11/window.rs

Lines 299 to 301 in b03e589

	if pl_attribs.x11_window_type != Default::default() {
	window.set_window_type(pl_attribs.x11_window_type).queue();
	}

Either this behaviour or the documentation should be changed, since the documentation states: Build window with `_NET_WM_WINDOW_TYPE` hint; defaults to `Normal`. Only relevant on X11.

Is it ok to just hack this into the if statement? Manually impl Default for PlatformSpecificWindowBuilderAttributes sounds more elegant but a couple things already seem to work around this in the same file. Note that the code is different with this commit, so we can't just remove the if:

winit/src/platform_impl/linux/x11/window.rs

Lines 301 to 303 in 1e83d9d

	if pl_attribs.x11_window_types.len() > 0 {
	window.set_window_types(pl_attribs.x11_window_types).queue();
	}

[goddessfreya]

I've pushed the slice variant. A static lifetime seems like the neatest way to do this, but stops users adding window types dynamically -- which is fine, IMO. I admit I'm not too confident with lifetimes so maybe this has worse consequences.

I'd rather not have it &'static. I think giving WindowBuilder a lifetime like glutin's ContextBuilder is the best option, but if that cannot be done (any objections to adding a lifetime, @Osspial?), I'd rather we accept a vec.

Something I noticed is that the window type is not set at all if the window type is the default:

winit/src/platform_impl/linux/x11/window.rs

Lines 299 to 301 in b03e589

	if pl_attribs.x11_window_type != Default::default() {
	window.set_window_type(pl_attribs.x11_window_type).queue();
	}

Either this behaviour or the documentation should be changed, since the documentation states: Build window with `_NET_WM_WINDOW_TYPE` hint; defaults to `Normal`. Only relevant on X11.

Is it ok to just hack this into the if statement? Manually impl Default for PlatformSpecificWindowBuilderAttributes sounds more elegant but a couple things already seem to work around this in the same file. Note that the code is different with this commit, so we can't just remove the if:

winit/src/platform_impl/linux/x11/window.rs

Lines 301 to 303 in 1e83d9d

	if pl_attribs.x11_window_types.len() > 0 {
	window.set_window_types(pl_attribs.x11_window_types).queue();
	}

I don't really know. cc @murarth

[Osspial]

any objections to adding a lifetime, @Osspial?

Not in principal, but I don't think forcing it via platform-specific extension traits is a good place to do it - it'll force us to hack in lifetimes for PlatformSpecificWindowBuilderAttributes on all other platforms to support a relatively minor feature. Creating a window is a fairly rare and expensive operation anyway, so I don't think asking for another allocation will have much impact.

[murarth]

winit/src/platform_impl/linux/x11/window.rs

Lines 301 to 303 in 1e83d9d

	if pl_attribs.x11_window_types.len() > 0 {
	window.set_window_types(pl_attribs.x11_window_types).queue();
	}

According to the WM spec (at the bottom of this section), if no _NET_WM_WINDOW_TYPE hint is provided, the WM must assume a type of _NET_WM_WINDOW_TYPE_NORMAL, so it's perfectly fine for winit documentation to state that Normal is the default and to provide no hints to the WM if the application has not provided any to winit.

The replacement if statement appears correct to me and the comment preceding it should be removed or changed to indicate that the WM is required to assume _NET_WM_WINDOW_TYPE_NORMAL if none is provided.

[Toqozz]

According to the WM spec (at the bottom of this section), if no _NET_WM_WINDOW_TYPE hint is provided, the WM must assume a type of _NET_WM_WINDOW_TYPE_NORMAL, so it's perfectly fine for winit documentation to state that Normal is the default and to provide no hints to the WM if the application has not provided any to winit.

I think this is a bit obtuse. If the documentation says the window type defaults to Normal, one would expect winit to apply the Normal hint, right? At the very least the wording should be changed to something like Build window with `_NET_WM_WINDOW_TYPE` hint. If no hint is set, the window manager will (should) assume `Normal`. Only relevant on X11., but this sounds weird because we'd be doing something weird.

Pretty much every application I can xprop has the _NET_WM_WINDOW_TYPE_NORMAL atom. I don't think it's even possible to leave it unset in gtk. It doesn't make sense for winit to be an exception here.

Also note, from the specification:

The Client SHOULD specify window types in order of preference (the first being most preferable) but MUST include at least one of the basic window type atoms from the list below.

The more I think about it the more I want to impl Default for PlatformSpecificWindowBuilderAttributes. This way users would be able to set no window hints if desired, while having a sensible default that aligns with the rest of the ecosystem.

Creating a window is a fairly rare and expensive operation anyway, so I don't think asking for another allocation will have much impact.

So just to confirm, Vec<WindowType>?

[murarth]

_NET_WM_WINDOW_TYPE_NORMAL indicates that this is a normal, top-level window, either managed or override-redirect. Managed windows with neither _NET_WM_WINDOW_TYPE nor WM_TRANSIENT_FOR set MUST be taken as this type. Override-redirect windows without _NET_WM_WINDOW_TYPE, must be taken as this type, whether or not they have WM_TRANSIENT_FOR set.

(Edit: Accidentally submitted too soon.)

The "MUST" language in the spec is pretty clear, but if you want to pass _NET_WM_WINDOW_TYPE_NORMAL by default, I'm not opposed to that.

[Toqozz]

_NET_WM_WINDOW_TYPE_NORMAL indicates that this is a normal, top-level window, either managed or override-redirect. Managed windows with neither _NET_WM_WINDOW_TYPE nor WM_TRANSIENT_FOR set MUST be taken as this type. Override-redirect windows without _NET_WM_WINDOW_TYPE, must be taken as this type, whether or not they have WM_TRANSIENT_FOR set.

(Edit: Accidentally submitted too soon.)

The "MUST" language in the spec is pretty clear, but if you want to pass _NET_WM_WINDOW_TYPE_NORMAL by default, I'm not opposed to that.

I edited my post a while after submitting to include a relevant section from the specification, probably after you started typing. Sorry!

[murarth]

I think you may be misreading that portion of the spec. I don't think it's saying that the client MUST supply _NET_WM_WINDOW_TYPE, but that, if present, it MUST include one of the given standard atoms.

This is how the section begins:

This SHOULD be set by the Client before mapping to a list of atoms indicating the functional type of the window.

A SHOULD; not a MUST. However, you're right that it is expected of a client and there's no reason not to provide the hint, so I concede the point. I agree that the default value should be vec![WindowType::Normal] and a manual impl Default for PlatformSpecificWindowBuilderAttributes is the way to go.

[Toqozz]

I think you may be misreading that portion of the spec. ...

Ahhh it seems like I am. It's almost like I'm reading some legislation with this spec -- thanks for understanding anyway.

Hopefully we've arrived at an implementation with this latest commit. Funnily enough we've almost gone right back to what I originally had to fix the issue locally.

Thanks for your patience.

[goddessfreya]

Good to merge, @murarth ?

[murarth]

@Toqozz This needs an entry in CHANGELOG.md. (It looks it had one, but it was removed in one of your revert commits.)

Otherwise, it looks great.