xwayland: add sensible rules for NET_WM_WINDOW_TYPE_DESKTOP windows

[grisha128]

Reference:

• xwayland: support querying more window types #1185
• Respect _NET_WM_WINDOW_TYPE_DESKTOP atom #1472
• Strategy for supporting _NET_WM_WINDOW_TYPE_DESKTOP #1474

I tried to avoid using HAVE_XWAYLAND macro according to @johanmalm idea:

@Consolatis and I brainstormed a bit on IRC and thought we might be able to extend the window-rule criteria to accept a wlr_surface rather than identifier, etc. and just add window rules for properties in the set_type handler? If that works, it would potentially be quite neat and self-contained within xwayland.c We'd probably have to deal with set_type()s happening after map().

But couldn't find a better way than this... If it's not good enough I'm ready to cooperate ;)

CC: @johanmalm @Consolatis @jlindgren90

[grisha128]

By the way, set_window_type handler works, but unfortunately it does much earlier than there's a surface available so I couldn't add window rule with wlr_surface criteria in this handler. Does it make sense? Please correct me if I'm talking nonsense ;)

[Consolatis]

I actually think this is even better than to create internal wlr_surface rules as we only need to setup the rule(s) once and it just magically works for all xwayland surfaces.

I didn't check in detail but I assume that we have to react to the Reconfigure event to keep the automatically added rule(s) around when reloading the config. The validate() function in src/config/rcxml.c might also need a slight adjustment.

We might also want to add another function pointer to struct view_impl so we don't need the HAVE_XWAYLAND check in view_matches_criteria(). Something like

// add in include/view.h
struct view_impl {
  ..
  bool (*contains_window_type)(struct view *view, int32_t window_type);
}

// assign in src/xwayland.c, can then be turned into a static function
static const struct view_impl xwayland_view_impl = { 
  ..
  .contains_window_type = xwayland_contains_window_type,
}

// and be called like
if (rule->window_type >= 0 && view->impl->contains_window_type) {
  return view->impl->contains_window_type(view, rule->window_type);
}

In general I really like this approach! Thanks for working on it.

[grisha128]

Thank you for a review :) I adjusted the patch!

I'm not sure what's wrong with CI - builds fine on my side.

[Consolatis]

Other than the last minor nitpick above this looks very good to me.

I do not merge the PR right away to give @johanmalm and possibly @jlindgren90 a chance to state their opinions.

Edit:
With this in place it should be trivial to add other internal window rules like for NET_WM_DOCK. In the long term we could think about exposing that capability to user configs as well but for now I don't see the necessity for that. If a user wants to have a different rule they can currently do so via ->title and ->identifier (user rules should come later in the list and thus have priority over the automatically added internal ones).

[grisha128]

Just figured that ToggleFullscreen action is quite handy for this.

[Consolatis]

Just figured that ToggleFullscreen action is quite handy for this.

With that one I am not quite sure:

• it disables top layer layershell (wayland native) panels
• It might actually unfullscreen a desktop client that starts up fullscreen by itself
• It doesn't adjust its geometry when a new X11 or wayland native panel requests exclusive screen space

So maybe a Maximize (rather than ToggleMaximize) might be better suited here?

[grisha128]

Just figured that ToggleFullscreen action is quite handy for this.

With that one I am not quite sure:

* it disables top layer layershell (wayland native) panels

* It might actually unfullscreen a desktop client that starts up fullscreen by itself

* It doesn't adjust its geometry when a new X11 or wayland native panel requests exclusive screen space

So maybe a Maximize (rather than ToggleMaximize) might be better suited here?

The thing is that when I use Maximize it doesn't turn off decorations despite having server_decoration set to LAB_PROP_FALSE. I tested it really quick and found out that it happens for me because I have

<windowRule identifier="*"><action name="ToggleDecorations"/></windowRule>

in rc.xml. It toggles decorations for NET_WM_WINDOW_TYPE_DESKTOP window as well, which is not nice.

This line fixed it though %)

<windowRule identifier="*" serverDecoration="no"></windowRule>

Changed ToggleFullscreen to Maximize.

[jlindgren90]

Sorry I haven't had time to really follow this thread or look at the changes. Don't let me hold up progress :)

[johanmalm]

Nice work.

Looks good in principle, except that I'm nervous about hard-coding properties and actions with no ability to configure at run-time. I think it's good to pre-configure labwc to work out of the box if a generic rule is possible, but my two comments above (against ignore_focus_request and Maximize) make me thing that a user/maintainer should be able to add/delete these rules via rc.xml too.

I could do some testing Thu/Fri.
Could you help by recommending some clients that use _NET_WM_WINDOW_TYPE_DESKTOP. I guess conky and pcmanfm --desktop use it, but am not sure.

[jlindgren90]

xfdesktop4 would be a well-known client to test

[Consolatis]

but my two comments above (against ignore_focus_request and Maximize)

I don't see them in this PR, stuck review?

make me think that a user/maintainer should be able to add/delete these rules via rc.xml too.

The internal rules have the lowest priority so a user could modify them by targeting ->identifier or ->title. However I sympathize with your general observation. We should support them for rc.xml and then throw the hardcoded rules into rc.xml.all as well. Same thing as before, they have the lowest priority so if a user would then configure their own rule targeting _DESKTOP (or _DOCK or whatever) they would be matched first and thus be applied.

Edit:
Skip that, we would still call their actions via window_rules_apply(). Hrm, that makes things slightly more complex.

However it should still be manageable: we could add the internal rules first and when parsing the rules we try to detect if a rule with the same criteria already exists and if yes we overwrite its values rather than creating a new one. Likely easiest to do in post_processing() similar to the keybind and mousebind merge.

[grisha128]

Nice work.

Thank you :^)

Looks good in principle, except that I'm nervous about hard-coding properties and actions with no ability to configure at run-time. I think it's good to pre-configure labwc to work out of the box if a generic rule is possible, but my two comments above (against ignore_focus_request and Maximize) make me thing that a user/maintainer should be able to add/delete these rules via rc.xml too.

I'm not sure if revealing this functionality to rc.xml makes a lot of use. For example, IceWM handles many different window types internally and has only title and class for window rule criteria. I reckon it's been like that for ages and no one seem to complain. Moreover, as @Consolatis said, user always has the ability to override these rules by title or identifier.

I could do some testing Thu/Fri. Could you help by recommending some clients that use _NET_WM_WINDOW_TYPE_DESKTOP. I guess conky and pcmanfm --desktop use it, but am not sure.

pcmanfm --display=:0 --desktop

QT_QPA_PLATFORM=xcb pcmanfm-qt --desktop

conky

(--display=:0 and QT_QPA_PLATFORM=xcb force apps to run in X11 mode)

I couldn't get xfdesktop to start.

[johanmalm]

I'm not sure we should ignore focus request. Might be fine for conky, but probably not pcmanfm --desktop

[grisha128]

I'm not so sure either, but as far as I can understand ignore_focus_request setting makes it so that window unable to take over user focus? If that's the case, can you give an example in which desktop window might want to take over user focus? In my opinion it's not very convenient for users to stumble upon desktop which randomly focuses itself

[johanmalm]

I haven't tested, but imagine an desktop client showing icons (like pcmanfm --desktop and xfdesktop) would want to take focus to operate right-click menus, rename icons and so on.

[grisha128]

Yeah, it needs focus to do that of course, but it's user-initiated focus, rather than focus requested by window, isn't it? Sorry if I'm talking nonsense, that's just what I figured from the setting name without looking deep into implementation. This is also confirmed by the fact that I can navigate right-click menus and rename files with this setting set to true.

[johanmalm]

I don't see them in this PR, stuck review?

It was 😄

[johanmalm]

I'll do some testing with clients tomorrow.

[grisha128]

Rebased on top of master with the work from #1731 and tested these:

pcmanfm --display=:0 --desktop

QT_QPA_PLATFORM=xcb pcmanfm-qt --desktop

conky

[johanmalm]

Are we sure ignore_focus_request should be true.

We should be able to operate pcmanfm--desktop context menus with the keyboard, which necessitates keyboard-focus.

[Consolatis]

Are we sure ignore_focus_request should be true.

We should be able to operate pcmanfm--desktop context menus with the keyboard, which necessitates keyboard-focus.

That is only for the window requesting focus itself. AFAIR it should receive keyboard focus just fine when being clicked on.

[grisha128]

Are we sure ignore_focus_request should be true.

We should be able to operate pcmanfm--desktop context menus with the keyboard, which necessitates keyboard-focus.

Since context menus usually appear only after user's mouse clicks which set focus, it should work just fine.

If you believe some windows of desktop type could pick up user focus without any input at first, the rule strictness can be lessen then.

[johanmalm]

Are we sure ignore_focus_request should be true. We should be able to operate pcmanfm--desktop context menus with the keyboard, which necessitates keyboard-focus.

Since context menus usually appear only after user's mouse clicks which set focus, it should work just fine. If you believe some windows of desktop type could pick up user focus without any input at first, the rule strictness can be lessen then.

Thanks for being patient 😄 ...and sorry for not keeping up with the conversation higher up. The ignoreFocusRequest thing makes sense now. I've probably spent too long on layer-shell clients and was stuck thinking that the clients would be able to grab focus at all (whereas this window-rules relates to the client requesting focus).

It feels like we ought to:

• Allow users to opt-out of this hard coded window-rule. With <keybinds> and <mousebinds> we've taken the approach that we will use builtin values if none were defined by the user. But the user can use the <default> child element to use the builtin values as a starting point to build on. We also have a None action to "clear" binds. I feel that window-rules are a bit more specialist, so maybe a simple <windowRules><clear> would be enough to destroy anything in rc.window_rules
• Document that a default window-rule exists for NET_WM_WINDOW_TYPE_DESKTOP and in big handfuls what it looks like.

[grisha128]

@johanmalm Hi, sorry for the wait. I adjusted the patch to meet these points ;) Tested the <clear /> element, seems to be working on my side!