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

X.H.EwmhDesktops: Add setEwmhMapHiddenWorkspaceToScreen function. #926

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

m1mir
Copy link
Contributor

@m1mir m1mir commented Feb 5, 2025

Description

The default assignment of hidden workspaces to the current screen is a good default, but it is not always desired.
An example for this would be a configuration that uses independent screens, because in that context workspaces
belong to certain screens which the EWMH module can't and won't consider when using the default behavior.

As far as I am able to tell the EWMH specification does not prescribe how hidden workspaces should be
assigned to screens, therefore I believe allowing customization is acceptable.

Checklist

  • I've read CONTRIBUTING.md

  • I've considered how to best test these changes (property, unit,
    manually, ...) and concluded: That since the modification is nested in a function that returns X () manual
    testing is the only option. I am currently using this modification in my window manager and it works as expected.

  • I updated the CHANGES.md file

@m1mir m1mir force-pushed the feat/ewmh-hidden-viewport branch from 3011d86 to 1e4420f Compare February 5, 2025 00:20
@geekosaur
Copy link
Contributor

EWMH has no clue whatsoever about xmonad's workspace handling; its workspace model assumes a workspace spans all screens (compare workspaces in Gnome, KDE, MATE, etc.) so all other workspaces are necessarily hidden and the concept of independent workspaces for screens simply doesn't exist. As such, EWMH is prone to report garbage when faced with IndependentScreens in any fashion.

@geekosaur
Copy link
Contributor

Oh, and it's not enormously better even without IndependentScreens; xmonad doesn't play by its rules.

@m1mir
Copy link
Contributor Author

m1mir commented Feb 5, 2025

EWMH has no clue whatsoever about xmonad's workspace handling; its workspace model assumes a workspace spans all screens (compare workspaces in Gnome, KDE, MATE, etc.) so all other workspaces are necessarily hidden and the concept of independent workspaces for screens simply doesn't exist. As such, EWMH is prone to report garbage when faced with IndependentScreens in any fashion.

I thought workspaces(, in the EWMH context desktops,) are assigned viewports in EWMH's model.
But I'm not sure if the conceptual difference matters at all for this PR. The example code as far as I can tell works
with independent screens and polybar's ewmh module regardless of my understanding of the specification's model.

I am aware that some tools relying on the EWMH functionality produce "garbage" in some situations, that is what I want to make partially fixable with this PR.
I believe that this change makes the problem that caused the addition of disableEwmhManageDesktopViewport easily solvable in the user's configuration in a lot of cases, without complicating the EWMH module's internals or exposing significant implementation details.

@geekosaur
Copy link
Contributor

Viewports are regions within an EWMH workspace (i.e. the whole visible area) which are available to applications, intended for use by panning window managers whose workspaces are larger than the visible area. Various applications will misbehave if you use them otherwise. Given the mismatch between xmonad's workspace model and EWMH's, we normally delete the viewport information because it usually causes more trouble than it solves; but in some cases it needs to be re-added. (Notably, I have a Qt application (Calibre) which doesn't display its menus properly if I don't set _NET_DESKTOP_VIEWPORT.)

@m1mir
Copy link
Contributor Author

m1mir commented Feb 5, 2025

I don't want to seem rude, but to me it seems none of your comments are really related to the code changes in this PR.
I think it's because I named the function in such a way that it sounds like it takes a boolean instead of this
(WindowSet -> (WindowSpace -> WindowScreen)) as it's first argument, so the name of it will probably have to change.

The contents of this PR don't intend to change which workspaces are mapped to viewports, and as far as I can tell I kept the default behavior.
The only thing this PR intends to do is add a way to customize which screen each workspace is mapped to when the _NET_DESKTOP_VIEWPORT management is not disabled, meaning that all workspaces in the WindowSet are still assigned to viewports.

@geekosaur
Copy link
Contributor

You're still talking about viewports, though.

@m1mir m1mir force-pushed the feat/ewmh-hidden-viewport branch 2 times, most recently from c7e774c to 51b28e8 Compare February 7, 2025 05:31
@m1mir
Copy link
Contributor Author

m1mir commented Feb 7, 2025

Rebased from more up to date master.

@m1mir m1mir force-pushed the feat/ewmh-hidden-viewport branch 2 times, most recently from 47051f9 to 51b1639 Compare February 8, 2025 16:23
Copy link
Member

@TheMC47 TheMC47 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

Your implementation of the default behavior makes sense, I verified that this is what we are indeed doing with EWMH (I still want to run your PR though just to double check).
That said, I'm not aware of what effects this_NET_CURRENT_DESKTOP has on other applications. The spec mentions the following:

For Window Managers that don't support large desktops, this MUST always be set to (0,0).

I'm not sure what the right behavior is.

An alternative would also be to simply omit the hidden workspaces, which won't be possible with your function. I would wait for someone who actually knows what they're talking about though

@m1mir m1mir force-pushed the feat/ewmh-hidden-viewport branch from 51b1639 to 4c5b5ce Compare February 10, 2025 20:19
@m1mir
Copy link
Contributor Author

m1mir commented Feb 10, 2025

An alternative would also be to simply omit the hidden workspaces, which won't be possible with your function. I would wait for someone who actually knows what they're talking about though

I chose to not allow omitting workspaces from mapping even with customization for the following reasons:

  • It was the previous behavior and having hidden workspaces mapped did not cause issues for me.
  • I don't see how having a mix of mapped and unmapped worksapces would be interpreted unambiguously.
  • I'm not even sure if having a different amount of positions and workpaces is allowed, because I think the viewport info is only linked to workspaces by their positions in the array.

My knowledge of the spec isn't very deep so I could easily misinterpret something about it.
If however "someone who actually knows what they're talking about" says that omitting is OK/allowed I'll change the behavior happily, because modifying it later would be a BC.

@m1mir
Copy link
Contributor Author

m1mir commented Feb 11, 2025

An alternative would also be to simply omit the hidden workspaces, which won't be possible with your function. I would wait for someone who actually knows what they're talking about though

I chose to not allow omitting workspaces from mapping even with customization for the following reasons:

* It was the previous behavior and having hidden workspaces mapped did not cause issues for me.

* I don't see how having a mix of mapped and unmapped worksapces would be interpreted unambiguously.

* I'm not even sure if having a different amount of positions and workpaces is allowed, because I think the viewport info is only linked to workspaces by their positions in the array.

My knowledge of the spec isn't very deep so I could easily misinterpret something about it. If however "someone who actually knows what they're talking about" says that omitting is OK/allowed I'll change the behavior happily, because modifying it later would be a BC.

I did a little experimenting with not mapping workspaces to viewports. I found that viewports are indeed only mapped to workspaces by their positions in the _NET_DESKTOP_VIEWPORT array.

I removed the mapping of all hidden workspaces in the mkViewPorts function and then queried the desktop information using wmctrl. I have 2 montiors with 9 workspaces on each. If the viewport assignment was based on the id vieports would be set for id 0 and 9 instead of 0 and 1.

$ wmctrl -d
0  - DG: N/A  VP: 1920,0  WA: N/A  1
1  - DG: N/A  VP: 0,0     WA: N/A  2
2  - DG: N/A  VP: N/A     WA: N/A  3
3  - DG: N/A  VP: N/A     WA: N/A  4
4  - DG: N/A  VP: N/A     WA: N/A  5
5  - DG: N/A  VP: N/A     WA: N/A  6
6  - DG: N/A  VP: N/A     WA: N/A  7
7  - DG: N/A  VP: N/A     WA: N/A  8
8  - DG: N/A  VP: N/A     WA: N/A  9
9  * DG: N/A  VP: N/A     WA: N/A  1
10 - DG: N/A  VP: N/A     WA: N/A  2
11 - DG: N/A  VP: N/A     WA: N/A  3
12 - DG: N/A  VP: N/A     WA: N/A  4
13 - DG: N/A  VP: N/A     WA: N/A  5
14 - DG: N/A  VP: N/A     WA: N/A  6
15 - DG: N/A  VP: N/A     WA: N/A  7
16 - DG: N/A  VP: N/A     WA: N/A  8
17 - DG: N/A  VP: N/A     WA: N/A  9

Based on this omitting any workspace would lead to problems so I think not allowing it even trough this customization is the right choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants