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

feat(xworkspaces): Add group-by-monitor flag #2926

Merged
merged 1 commit into from Oct 29, 2023

Conversation

slotThe
Copy link
Contributor

@slotThe slotThe commented Mar 1, 2023

We had someone else get bit by this, so I figured why not pick up the (pretty much already done) patch by @scaramangado from #2603 and add some docs.

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Other: Replace this with a description of the type of this PR

Description

By default, we group workspaces by monitor with the help of _NET_DESKTOP_VIEWPORT. However, some users may experience this as an unpredictable "shuffling" of workspaces. While WMs could disable advertising the property itself, it seems more sensible to handle this at the level of polybar. Hence, introduce a new group-by-monitor flag—defaulting to true—which can be used to disable this behaviour.

Related Issues & Documents

Closes: #2603
Related: xmonad/xmonad-contrib#791
Related: qtile/qtile#3375

Documentation (check all applicable)

  • This PR requires changes to the Wiki documentation (describe the changes)

    I suppose documentation of the flag, as well as the incompatibility with monitor-label should be put into the relevant xworkspaces page. Possibly with references to some of the above issues (although I don't know whether that'd help user find it).

  • This PR requires changes to the documentation inside the git repo (please add them to the PR).

    I added a changelog entry, not sure if there's anything else to be done.

  • Does not require documentation changes

@slotThe
Copy link
Contributor Author

slotThe commented Jun 30, 2023

@patrick96 friendly ping :)

@patrick96 patrick96 added this to the 3.7.0 milestone Jun 30, 2023
@patrick96
Copy link
Member

Hey, sorry, really don't have too much time right now. It's on my list though

Thanks :)

@slotThe slotThe force-pushed the feat/xworkspaces/group-by-monitor branch from 8c5e204 to 7337279 Compare July 1, 2023 10:04
@slotThe
Copy link
Contributor Author

slotThe commented Jul 1, 2023

Okay, no worries, just making sure it didn't fall between the cracks somewhere :)

(the force push rebased it on top of current master, for convenience)

@anthraxx
Copy link

anthraxx commented Oct 5, 2023

I've been hit by precisely this bug and found the solution here. Thanks for the work so I didn't have to write this patch myself 👍
@patrick96 thank you as well for all your work with this great project ❤️ Is there any way to help out on this PR in any way? Cheers!

Copy link
Member

@patrick96 patrick96 left a comment

Choose a reason for hiding this comment

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

Finally found some time (and motivation) to review this. This is the second-last thing on my list before I want to release 3.7.0!

Code looks good. Just some minor changes, I think the logic is reversed. Otherwise there is nothing else to do

CHANGELOG.md Outdated Show resolved Hide resolved
src/modules/xworkspaces.cpp Outdated Show resolved Hide resolved
By default, we group workspaces by monitor with the help of
_NET_DESKTOP_VIEWPORT.  However, some users may experience this as an
unpredictable "shuffling" of workspaces.  While WMs could disable
advertising the property itself, it seems more sensible to handle this
at the level of polybar.  Hence, introduce a new group-by-monitor
flag—defaulting to true—which can be used to disable this behaviour.

Closes: polybar#2603
Related: xmonad/xmonad-contrib#791
Related: qtile/qtile#3375

Co-authored-by: scaramangado <scaramangado@gmail.com>
@slotThe slotThe force-pushed the feat/xworkspaces/group-by-monitor branch from 7337279 to 6f2a804 Compare October 23, 2023 05:55
Copy link
Member

@patrick96 patrick96 left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks for your help :)

Now to actually preparing the release

@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

Merging #2926 (6f2a804) into master (c747599) will decrease coverage by 0.01%.
Report is 17 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2926      +/-   ##
==========================================
- Coverage   12.69%   12.69%   -0.01%     
==========================================
  Files         160      160              
  Lines       12531    12533       +2     
==========================================
  Hits         1591     1591              
- Misses      10940    10942       +2     
Flag Coverage Δ
unittests 12.69% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
include/modules/xworkspaces.hpp 0.00% <ø> (ø)
src/modules/xworkspaces.cpp 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes

@patrick96 patrick96 merged commit f7a7557 into polybar:master Oct 29, 2023
8 of 9 checks passed
@patrick96 patrick96 mentioned this pull request Nov 5, 2023
24 tasks
@slotThe slotThe deleted the feat/xworkspaces/group-by-monitor branch November 6, 2023 07:35
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.

[Bug]: _NET_DESKTOP_VIEWPORT causes workspace order to change when using multiple monitors (xworkspaces)
3 participants