X.H.EwmhDesktops: Add customization for handling the _NET_CURRENT_DESKTOP requests

[m1mir]

Description

This adds a setEwmhSwitchDesktopAction function to customize how switching to the desired workspace is done when handling a _NET_CURRENT_DESKTOP request. The motivation for this was that commands using the EWMH fuctionality such as wmctrl -s pulled workspaces to the wrong screen in certain cases when using the X.L.IndependentScreens module, instead of making them active on the screen that they belong on, but the behavior seemed to be a sensible default since use of that module is not mandatory.

With the new function I changed the action to the following:
action workspaceId = viewOnScreen (unmarshallS workspaceId) workspaceId
This no longer pulls the workspace to the wrong screen when it is hidden, and it is on a screen that does not contain the current workspace, this is also used as the example in the documentation.

Checklist

• I've read CONTRIBUTING.md

• I've considered how to best test these changes (property, unit,manually, ...) and concluded:
  that manual testing is adequate since change just added a customization option without changing the default.
  I did not find a difference in the default behavior, using this version and the example is currently working as expected
  on my system.

• I updated the CHANGES.md file

[geekosaur]

Otherwise, looks good.

[m1mir]

I've tested the example usage of the new focusWorkspace manually, as far as I can tell it works as expected.

[geekosaur]

I'm interested in seeing what @liskin or @slotThe says about it.

[geekosaur]

Sorry, I'd already re-reviewed but didn't push the "approve" button; wanted to wait on CI.

[liskin]

It is beatiful. :-)

Commented about a couple of nits anyway, as I usually do.

Plus worth noting that a complete integration with IndependentScreens would also involve using focusWindow' for activateHook (converted into a ManageHook), but it would still potentially misbehave because _NET_ACTIVE_WINDOW requests from pagers (and tools like wmctrl) aren't overridable right now. :-(

[m1mir]

I tested it again manually after the rename. It works as expected.

[m1mir]

Plus worth noting that a complete integration with IndependentScreens would also involve using focusWindow' for activateHook (converted into a ManageHook), but it would still potentially misbehave because _NET_ACTIVE_WINDOW requests from pagers (and tools like wmctrl) aren't overridable right now. :-(

Could you clarify what you meant by this paragraph?
I've did a little experimenting and if I just change the switchDesktopHook to focusWorkspace and I just leave the activate hook as the default doFocus, then wmctrl -a works as expected.
I think that's because this function in wmctrl as far as I can tell always does a _NET_CURRENT_DESKTOP request first when using XMonad and then it does the _NET_ACTIVE_WINDOW request, and the first request was probably the source of the weird behavior.

With the non wmctrl sources of activation request I use doF . focusWindow' =<< ask as the activateHook and it seems to work normally both with the new switchDesktopHook set to focusWorkspace and without it.

[liskin]

Oh okay I didn't realise wmctrl switches the desktop first - that's awesome. There may be something else that doesn't, but maybe we don't need to care - not having to complicate the code any more is good.

[geekosaur]

We are permitted to convert it to an urgency indicator if it's not on the active workspace.

[m1mir]

Oh okay I didn't realise wmctrl switches the desktop first - that's awesome. There may be something else that doesn't, but maybe we don't need to care - not having to complicate the code any more is good.

The things that I intended to convey were the following:

• Just customizing the switch desktop action is enough for wmctrl -a to work "properly" with IndependentScreens.
• With the current API, by which I mean the package's released version's, the case when just a _NET_ACTIVE_WINDOW is sent is already easily handled by changing the activate hook.

Therefore I consider the contents of this PR complete, unless modifications are requested.

[m1mir]

Also I haven't noticed until now but this probably closes this #776 issue.

[geekosaur]

That does look likely.

[slotThe]

Thanks!

[liskin]

Oh okay I didn't realise wmctrl switches the desktop first - that's awesome. There may be something else that doesn't, but maybe we don't need to care - not having to complicate the code any more is good.

The things that I intended to convey were the following:

• Just customizing the switch desktop action is enough for wmctrl -a to work "properly" with IndependentScreens.
• With the current API, by which I mean the package's released version's, the case when just a _NET_ACTIVE_WINDOW is sent is already easily handled by changing the activate hook.

Therefore I consider the contents of this PR complete, unless modifications are requested.

Yeah, I agree that the PR was good and no modifications were requested from me. Sorry for not being clear earlier, must've been in a hurry, and then went AFK.

The only little issue that may still be unclear is that there's that special case for _NET_ACTIVE_WINDOW requests from pagers (an external tool for switching windows/desktops) that is unconditionally handled using focusWindow as requests from pagers aren't meant to be ignored. The code for customization of _NET_ACTIVE_WINDOW was motivated by the need to ignore some requests, but it seems ill-prepared for use-cases such as integration with IndependentScreens — so the unconditional special case isn't overridable. Fortunately, it's not needed with wmctrl, but in theory there might be a pager that just sends a _NET_ACTIVE_WINDOW request without doing _NET_CURRENT_DESKTOP first. But I think it's okay not to complicate the code until someone actually needs it.

Hope this makes it clearer :-)

[m1mir]

Sorry, I know that this has been merged. But I

Oh okay I didn't realise wmctrl switches the desktop first - that's awesome. There may be something else that doesn't, but maybe we don't need to care - not having to complicate the code any more is good.

The things that I intended to convey were the following:

• Just customizing the switch desktop action is enough for wmctrl -a to work "properly" with IndependentScreens.
• With the current API, by which I mean the package's released version's, the case when just a _NET_ACTIVE_WINDOW is sent is already easily handled by changing the activate hook.

Therefore I consider the contents of this PR complete, unless modifications are requested.

Yeah, I agree that the PR was good and no modifications were requested from me. Sorry for not being clear earlier, must've been in a hurry, and then went AFK.

The only little issue that may still be unclear is that there's that special case for _NET_ACTIVE_WINDOW requests from pagers (an external tool for switching windows/desktops) that is unconditionally handled using focusWindow as requests from pagers aren't meant to be ignored. The code for customization of _NET_ACTIVE_WINDOW was motivated by the need to ignore some requests, but it seems ill-prepared for use-cases such as integration with IndependentScreens — so the unconditional special case isn't overridable. Fortunately, it's not needed with wmctrl, but in theory there might be a pager that just sends a _NET_ACTIVE_WINDOW request without doing _NET_CURRENT_DESKTOP first. But I think it's okay not to complicate the code until someone actually needs it.

Hope this makes it clearer :-)

I know that this has been merged, but this seems to be the appropriate space to ask. What do you mean by ill suited?

In my config I use doF . focusWindow' =<< ask for the activate hook as I mentioned previously. It is almost the same as the original doFocus but it uses the IndependentScreens module's focusWindow' instead of the the StackSet one and it does work perfectly as far as I can tell. For example it focuses Firefox and Discord without pulling them and their workspace to the wrong physical screen.

If that is a hook that solves the issues that you are thinking about, I could add it to X.L.IndependentScreens in a PR tomorrow.

[liskin]

I know that this has been merged, but this seems to be the appropriate space to ask. What do you mean by ill suited?

I'm specifically referring to this bit:

xmonad-contrib/XMonad/Hooks/EwmhDesktops.hs

Lines 509 to 512 in 4fc3642

	| mt == a_aw, 2 : _ <- d ->
	-- when the request comes from a pager, honor it unconditionally
	-- https://specifications.freedesktop.org/wm-spec/wm-spec-1.3.html#sourceindication
	if W.peek s == Just w then mempty else windows $ W.focusWindow w

Since I didn't consider the use-case of integrating IndependentScreens when implementing this, the W.focusWindow there isn't overridable.

In my config I use doF . focusWindow' =<< ask for the activate hook as I mentioned previously. It is almost the same as the original doFocus but it uses the IndependentScreens module's focusWindow' instead of the the StackSet one and it does work perfectly as far as I can tell. For example it focuses Firefox and Discord without pulling them and their workspace to the wrong physical screen.

If that is a hook that solves the issues that you are thinking about, I could add it to X.L.IndependentScreens in a PR tomorrow.

It doesn't solve the issue I'm thinking about, but then we don't know if that issue isn't purely hypothetical. 🙂

It definitely does solve the actual issue you encountered, so either documenting it (it's simple enough really) or adding it as a function to IndependentScreens seems valuable.

---

tl;dr - only read this bit if you enjoy discussing complex hypothetical technical shit

To expand a bit more on my thinking back when the hooks were implemented — I was aware of the theoretical possibility of someone needing to override all the ways _NET_ACTIVE_WINDOW is handled, but I couldn't come up with a code that would look good. It was either:

• a Bool → ManageHook, which is ugly (boolean blindness) and prevents just using an existing function of type ManageHook and thus requires Haskell knowledge to be useful
• a SomeNewSumType → ManageHook, which is a little (!) bit nicer but also too many new lines of code
• two ManageHooks, which is almost okay and I probably should've done this but then it was sorta okay to wait until it's actually needed (and we still don't know if it actually is or if it's just hypothetical)

[m1mir]

I understand now. Thank you for the explanation. I found the second part especially fascinating because I typically design poor APIs, and this gave me examples of what to consider.