paperwm / PaperWM

Tiled scrollable window management for Gnome Shell
GNU General Public License v3.0
2.75k stars 122 forks source link

Fix multi-monitor persistence and improve PaperWM dynamic workspaces support #577

Closed jtaala closed 9 months ago

jtaala commented 10 months ago

Fixes #571.

Multi-monitor workspace to monitor persistence was a bit broken (okay, quite broken).

This PR fixesworkspace-to-monitor persistence (e.g. PaperWM's ability to restore monitor-workspace layouts after a unlock or monitor change.

It also improves dynamic workspaces behaviour in PaperWM by:

It now also attempts to remember and restore the previous multimonitor layout across where you had a multimonitor layout, switched to single monitor (during the session) and then reconnect to multimonitors.

The previous approach used a heuristic based on some properties of the monitors - this was unstable though as monitor ordering and (indexes) can change (it's generally stable but sometimes can change).

This PR replace the heuristic (guess) with more stable monitor connector ids (e.g. "eDP-1", "HDMI-1" etc.). It does this by:

If you now check the INFO_LEVEL logs on unlock (or disable/enable PaperWM, monitor changes etc.), you'll now see lines like:

Aug 13 21:23:21 jay-blade gnome-shell[1599991]: #PaperWM enabled
Aug 13 21:23:21 jay-blade gnome-shell[1599991]: Workspace 6 restored to monitor DP-1
Aug 13 21:23:21 jay-blade gnome-shell[1599991]: Workspace 2 restored to monitor eDP-1

which shows a successful restoration of persisted data.

Summary of space-to-monitor restore logic:

YaLTeR commented 10 months ago

Okay, so I:

  1. Relogged with second monitor disabled
  2. Made some windows, so I had workspaces 1, 2, 3 and (empty) 4
  3. Went to workspace 2
  4. Enabled the second monitor in Settings

That resulted in Workspace 3 with its windows moving over to the second monitor, which is unwanted. So the main monitor ended up with workspaces 1, 2 and (empty) 4.

YaLTeR commented 10 months ago

When that happened it's also a bit peculiar that the second monitor had no other workspaces, so I couldn't switch away and then do the Super+` trick from the main monitor.

YaLTeR commented 10 months ago

Right now after some messing around I still have only 4 workspaces in total for some reason with no extra workspaces on either monitor to move windows to.

YaLTeR commented 10 months ago

After putting a window on Workspace 4, I now have an empty Workspace 11 (???) which I can switch to from either the main screen or the second screen.

jtaala commented 10 months ago

Thanks @YaLTeR, just so I can see what you're on, can you please confirm commit id?

jtaala commented 10 months ago

Okay, so I:

  1. Relogged with second monitor disabled
  2. Made some windows, so I had workspaces 1, 2, 3 and (empty) 4
  3. Went to workspace 2
  4. Enabled the second monitor in Settings

That resulted in Workspace 3 with its windows moving over to the second monitor, which is unwanted. So the main monitor ended up with workspaces 1, 2 and (empty) 4.

Okay, let me try to reproduce this exactly and see what I get.

jtaala commented 10 months ago

Let's sync up here. Need precision here so I can reproduce:

Also, for testing , please try disableing dynamic workspaces and see if behaviour is any different (that will help see if these issues only occur on dynamic workspaces).

jtaala commented 10 months ago

Also, need to make sure we're on exactly the same starting point (e.g. commit id).

Lastly, to be sure can you please check that there's only one paperwm folder in:

~/.local/share/gnome-shell/extensions

Thanks!

Jay.

YaLTeR commented 10 months ago

can you please confirm commit id?

830318fafff2ed0028deabd7bbb7d40296201347

how many windows did you create on each workspace (I'll just use terminals);

3 on first, 2 on second (one of which is the Settings window where I enabled the second monitor) and uhh, 3 on the third I think.

when you said relogged - you mean logout?

Yep.

please try disableing dynamic workspaces and see if behaviour is any different (that will help see if these issues only occur on dynamic workspaces).

I'll try a bit later.

can you please check that there's only one paperwm folder in:

That is correct.

jtaala commented 10 months ago

Awesome. Okay, let me take a video of what I'm seeing - and let me know what I might be doing wrong here (to reproduce).

jtaala commented 10 months ago

Hey @YaLTeR, here's what I'm seeing (short video) of trying to replicate what you were doing:

video: multimonitor persistence attempt 0

Ah, dang, stupid good drive, will try upload elsewhere. now link is working apparently.

YaLTeR commented 10 months ago

Thanks! Yeah, so this seems to match what happened for me. Before turning on the second monitor, I switched from Workspace 3 to 2, so 3, being the most recently used, showed up on the second monitor. However, I guess I just expect slightly different behavior here: since the second monitor wasn't plugged in at all before during this session, I do not expect any workspaces to move there, instead I expect it to start with a blank workspace. So it might be that Paper works as intended for this case?

jtaala commented 10 months ago

Thanks! Yeah, so this seems to match what happened for me. Before turning on the second monitor, I switched from Workspace 3 to 2, so 3, being the most recently used, showed up on the second monitor. However, I guess I just expect slightly different behavior here: since the second monitor wasn't plugged in at all before during this session, I do not expect any workspaces to move there, instead I expect it to start with a blank workspace. So it might be that Paper works as intended for this case?

Ah, okay - well, it's working as I coded it to.... but I'm not saying it's the best approach. Was a mix of what I thought would be good/cool and some code constraints.

However, I'm open to suggestions on how it "should" work - given the scenario in the video linked - describe how you think it should work. When you say a blank space, like one that hasn't been used previously? - or sequential from the current workspace? Creating a new workspace wouldn't work with fixed workspaces.

Not sure we'd be able to implement it, but open to suggestions. Also, I note that workspaces are a little different in PaperWM - pretty much all workspaces (except the ones that are currently shown on monitors) are alwasys available to all monitors (i.e. with workspace stacking switching etc.). So, there's really no hard concept of "workspace a, b belong to monitor 1, and workspaces c, d belong to monitor 2".

jtaala commented 10 months ago

The "most recent workspace" tracking approach made sense to me as it seemed natural when switching between single and multi-monitor (e.g. if I'm working in multi-monitor with 2 workspaces => then if I switch to single monitors I have those same two as most recent and hence easiest to switch between). That seemed more seemless to me (although might not be what other systems do).

The other upside is that it overcomes any issues re delay of monitors waking up after sleep etc.

YaLTeR commented 10 months ago

Creating a new workspace wouldn't work with fixed workspaces.

Right, for fixed workspaces you'll be the better judge; my brain very much works in dynamic workspaces mode. :)

I'm open to suggestions on how it "should" work

I'm actually currently in the middle of thinking of a design on how dynamic workspaces should work regarding connecting and disconnecting monitors, but it'll be some time until I've had the chance to properly test it and see what actually works and what doesn't. However, some very rough notes are:

  1. There's always a single empty workspace available at the end on each monitor (same idea as in vanilla Shell).
  2. Connecting an entirely new monitor shouldn't move any workspaces around, so it should only have the one new empty workspace.
  3. Disconnecting a monitor moves all its non-empty workspaces to the primary monitor (they have to go somewhere). However, reconnecting the same monitor should be able to restore the workspaces back onto it. So:
    1. All workspaces remember their "original monitor".
    2. When a monitor connects again, any workspaces for which it is the "original monitor" get moved back onto it.
    3. If a new window is created or moved onto a workspace, its "original monitor" is updated to its current monitor. This is to avoid the situation where the user works with two monitors, then unplugs one, keeps working with one monitor for a while (using workspaces without regard to their "original monitor"), then plugs it back, and suddenly seemingly random workspaces move.

This even works with Paper's ability to open an existing workspace on a different monitor: just update the workspace's "original monitor" to the new monitor in that case.

The "most recent workspace" tracking approach made sense to me as it seemed natural when switching between single and multi-monitor (e.g. if I'm working in multi-monitor with 2 workspaces => then if I switch to single monitors I have those same two as most recent and hence easiest to switch between). That seemed more seemless to me (although might not be what other systems do).

I personally do most of my work on the primary monitor, while the second monitor has auxiliary windows. This is even more true with my current setup where my second monitor is a TV off to the side, so I only move things there when e.g. I want to watch a video on the TV or when I'm livestreaming or recording something so I put the OBS window there. Therefore it never makes sense to automatically move the MRU workspace there.

jtaala commented 10 months ago

Cool - yes, the current implementation will at least solve the issue of restoring workspaces as they were, in the case of an external monitor connected.

The design above seems very interesting and can see some good thoughts there. At face value, I can see some issues that would require quite a bit of refactoring and design changes, but that's to be expected.

I guess it's personal use cases and preference as I don't use multimonitors like that (but each to their own!) - I often chop/change through workspaces and often switch them around depending on what I'm doing. I also very often disconnect and go sit in the lounge while working and then come back to plug in again, so this PR design makes some sense there.

Would definitely consider any PR when you get your design working in PaperWM for sure though. I don't use dynamic workspaces but the changes in this PR went a long way in cleaning up and making multimonitor things a bit simpler and should provide a better base for working on.

Let me know how you go with the design on dynamic workspaces.

YaLTeR commented 10 months ago

Is it reasonably easy to at least make sure there's always an empty workspace in the end of each monitor with dynamic workspaces with the current implementation?

jtaala commented 10 months ago

Is it reasonably easy to at least make sure there's always an empty workspace in the end of each monitor with dynamic workspaces with the current implementation?

Well, I guess easy is a relative term (and relatively easy is even more relaive!).

However, have a look at gnome shells' _checkWorkspacess method and compare to PaperWM's version which overrides that behaviour (and which modifies the behaviour around the extra space at end etc.)

jtaala commented 10 months ago

reasonably easy

It's referenced by line 176 which you can remove/comment-out to restore default gnome behaviour with dynamic workspaces. You'll likely have a bunch of side-effects though, but that should get you started.

YaLTeR commented 10 months ago

Unfortunately, after a lock/unlock one of my workspaces swapped monitors again

jtaala commented 10 months ago

Unfortunately, after a lock/unlock one of my workspaces swapped monitors again

Oh poops, I've been hammering it and haven't seen that yet.

Can you reproduce it? (is it like you leave it for a while and come back or it just happens?). Try to reproduce or get steps to reproduce. I'll try as well but haven't had that yet today.

jtaala commented 10 months ago

I'll try as well but haven't had that yet today.

Okay, I just decided to hammer it and went through 32 consecutive lock-unlock cycles and workspaces were stable and didn't switch at all. There must be something else going on in your setup.

Tell me, are you monitors the exact same resolution?

jtaala commented 10 months ago

Ha, maybe it's multiple monitor-changed signals during unlock? I've seen that referenced somewhere else. I try a few guards (or delay time for restore) to see if that helps.

YaLTeR commented 10 months ago

Tell me, are you monitors the exact same resolution?

Nope, laptop is 2560x1600 and secondary is 1920x1080.

jtaala commented 10 months ago

Tell me, are you monitors the exact same resolution?

Nope, laptop is 2560x1600 and secondary is 1920x1080.

Okay, do a pull now. I've implemented a 100ms guard and delay after receiving a monitor-changed signal (and we ignore subsequent monitor-changed signals during the 100ms guard). After guard delay we'll kick off an actual monitorChanged() method call to restore.

I've also remove the heuristic check on resolution checks for restore.

May be a long shot but give https://github.com/paperwm/PaperWM/commit/b8867ad3f90755199e803a6da6b732e69eb0108c a good work out.

jtaala commented 10 months ago

Whoops, just a sec

jtaala commented 10 months ago

Whoops, just a sec

Okay, good to go. Should be commit https://github.com/paperwm/PaperWM/commit/b8867ad3f90755199e803a6da6b732e69eb0108c.

jtaala commented 10 months ago

A little cleanup just pushed. See how you go.

I've now done a total of 70 consecutive lock/unlock cycles and a similar number of "monitor yanks" (or just changing monitor setup between multimonitor and single) - haven't been able to reproduce a single workspace swap. Not sure what else to try.

It's got to be some weird corner case. Keep an eye out for any reproducible steps, as unless I can reproduce it, I'm not going to be able to isolate and fix it.

This PR fixes the monitor-swap I could reproduce (which was the monitor disconnect/reconnect one in the video I posted). Whatever is happening with your setup we haven't been able to catch yet.

Sadly, you might have to get good at workspace stack switching to work around it until we've got a reproducible path.

If it helps, the default key for Switch to previously active workspace in PaperWM makes this much easier Super+` (Super + backtick). Do that on the workspace you want to move to the other monitor, move to the other monitor (with mouse or Switch to right monitor etc. shortcut) and then do Super + backtick again and you'll open the workspace on the new monitor).

If you're interested I can look at implementing a nice shortcut to swap monitor around (although I wonder how that will work with 3 monitors etc.).

jtaala commented 10 months ago

I just realised we aren't saving the layout (for monitor restore) for dynamic workspaces when a new workspace is created, removed etc). Now we do that when a workspace moves monitors, but forseeable, one could create a dynamic workspace and not move a workspace - then lock/unlock and restore could get a bit messed?

Does that sound like something that could happen in your workflow @YaLTeR? Just an thought. I don't use dynamic workspaces so that may have something to do with it?

There is something else we could try as well - and that's switching from our restore heuristic to dbus display connector ids (it's a bit of work to replace it but eventually we might need to try that).

YaLTeR commented 10 months ago

I just realised we aren't saving the layout (for monitor restore) for dynamic workspaces when a new workspace is created, removed etc). Now we do that when a workspace moves monitors, but forseeable, one could create a dynamic workspace and not move a workspace - then lock/unlock and restore could get a bit messed?

Hmm, well, yeah, I pretty much never move workspaces between monitors currently. Only to move it back when Paper unwantedly relocated it.

jtaala commented 10 months ago

Hmm, well, yeah, I pretty much never move workspaces between monitors currently. Only to move it back when Paper unwantedly relocated it.

well, cross that one out then. Only things I've got left are something to do with dynamic workspaces (maybe that's why I haven't been able to reproduce on my system?) and monitor indexes being unstable (which is a known thing in gnome).

Let's try implement actual connector id's as monitor identifiers (instead of relying on the monitor index order as reported by gnome LayoutManger).

On a sidenote, I've been playing with dynamic workspaces and PaperWM does seem to have some issues there (for example, doing a <Super+Shift+pg_up>, which is supposed to move the current window to a new dynamic workspace seems to mess things up in paperwm). Anyways, I reckon we give improving that a go and implement something like the design you mentioned above.

Can look at that in a separate PR though after we get some of the fixes in this PR in.

jtaala commented 10 months ago

Hey all, converting this to draft as I'm implementing the async dbus proxy connector (which upgrades Main.layout.monitors to store the monitors connector value, e.g. "eDP-1" etc.).

Looking promising but does need a little bit of time to get right.

YaLTeR commented 10 months ago

Uh, so I have no idea what I've done or if it's related to this PR, but after one of the unlocks I got a super glitched session:

Screenshot from 2023-08-13 15-00-21

This somehow cuts in the middle of the screenshot, but since my screens are not equal size (1920 and 2560 in that order), the cut is actually in the left part of my right screen.

I managed to kb-nav to an extensions window and disable Paper, then it showed this and the session didn't unglitch (I had to relogin to fix things):

Screenshot from 2023-08-13 15-01-06

I'm on 6230668498c9c8b8d5d5d1fc68f7802dc8811b3f.

jtaala commented 10 months ago

Hey @YaLTeR - yeah, that commit turned out to be not a great approach.

I've just finished up, what I think is a winner! Def pull the latest on this PR, it should be commit https://github.com/paperwm/PaperWM/commit/a2cdf3deaaa47dbbfaf0a435c6cf4718104e1d4a.

jtaala commented 10 months ago

I'm just about ready to bring this one out of draft state.

So, here's what's different (pretty much a redesign of the restore approach):

jtaala commented 10 months ago

or if it's related to this PR,

Yeah, that was related to that commit state of this PR... turns out the issue was not what that commit assumed (thought might be multiple monitor changes signals) - adding the guard on there had a side-effect of messing some stuff up).

jtaala commented 10 months ago

So, I'm thinking that one improvement would be that, with multimonitors and a fresh login - that workspace 1 should always be on the primary monitor? Mainly for consistency.

Currently, if no saved previous layouts (e.g. new login) that may not be the case, at least on my setup (which may be a bit confusing).

jtaala commented 10 months ago

Currently, if no saved previous layouts (e.g. new login) that may not be the case

Found the reason, basically because primary monitor isn't guaranteed to be the first montor in Main.layoutManager.monitors, and for restore PaperWM iterates through that monitor array (and hence the first monitor in that array might not be primary).

YaLTeR commented 10 months ago

Ah, cool, thanks

jtaala commented 10 months ago

So, now that I've used this for a few days, and although it does fix the original issue, still not quite happy with some of the behaviour, especially when you switching between single/multiple monitors.

I think the approach where we try to keep/persist the last multi-monitor layout, even amidst switching to single monitor mode, and changing things up leads to some non-intuitive behaviour.

I'm going to change this slightly to just persisting the monitor layouts whenever:

This will simplify things and hopefully make the restore behaviour more intuitive.

jtaala commented 10 months ago

My only concern with the simplified approach here (which may just be theoretical as I haven't proven it), is that if an external monitor takes a while to "wake up", then it may be treated as a new space.

For example, say we have space 3 on the primary monitor and space 6 on the external:

When we lock, we save that layout.... then external monitor sleeps/turns off etc. Then we wake our computer and unlock... but the external monitor is off (or takes a while to wake up). PaperWM on unlock will try to restore space 6 to the external monitor... uh oh, it's not there (since is off or waking up) --> PaperWM says "oh, well, not there, can't restore" and then restore space 3 to primary and will save this new layout (for the next restore).

Then, when the external monitor is woken up (or turned on) - PaperWM restores the "last" saved layout (which now doesn't have the last multimonitor layout saved)... and will assign another space to it.

This was what I was thinking when I chose to persist the last multimonitor layout.

At @YaLTeR - does this sound like a feasible use case to consider? It's worth testing with this new simplified approach in any case to see how it goes.

jtaala commented 10 months ago

Some testing - yes, this does happen if the monitor is "off" when we unlock PaperWM. So it seems you actually might want to save the last space on a monitor and persist that, regardless of whether it's there on restore or not (which is fine btw) - so that when it does turn up again, we attempt to restore the last space that was on it.

YaLTeR commented 10 months ago

Not sure off the bat, we'll see. FWIW the way I use the second monitor now, if it always starts with an empty workspace, would be better than if it sometimes gets a workspace from primary.

YaLTeR commented 10 months ago

Not sure if it has anything to do with this PR but over the past two days I've noticed something aggressively trying to warp my cursor to the center of the laptop monitor every time I try to move the cursor to the second monitor. Only occasionally it lets me do that.

jtaala commented 10 months ago

Cool, pull the latest - we now persist monitor connectors, and the last workspace that was on it (whether the monitor exists or not). Means that, if primary monitor hasn't used that workspace (after external monitor) then it will be restored whenever that external monitor shows up.

jtaala commented 10 months ago

if it always starts with an empty workspace, would be better than if it sometimes gets a workspace from primary.

Stealing from primary shouldn't happen on latest (https://github.com/paperwm/PaperWM/pull/577/commits/ad5fc1a23f3633277403c478be1d6856b28e6c2a).

We persist that monitor's last used workspace now. So, unless the primary has used that workspace (after external) then it will be restore to external.

It now shouldn't steal the workspace from primary monitor. This implicitly works since in restoration, primary monitor is checked first, then other monitors.

So basically should work like:

jtaala commented 10 months ago

I've noticed something aggressively trying to warp my cursor to the center of the laptop monitor

Let me know if you get steps to reproduce this. Only thing I've seen that has ever warped pointer to the center is the switch monitor keybinds for Switch to the right monitor, Switch to the left monitor.

jtaala commented 10 months ago

would be better than if it sometimes gets a workspace from primary

Just to clarify, in PaperWM, only the spaces currently shown on monitors "belong" to that monitor - all other spaces belong in the void! (for lack of a better word) and can be "opened" on any monitor (through PaperWM workspace switching etc.).

But yes, on the latest, we shouldn't get a space that is currently shown on a primary monitor "yanked" to another monitor (unless there's a bug or some corner case I haven't thought of!).

jtaala commented 10 months ago

unless there's a bug or some corner case I haven't thought of!)

Okay, now it shouldn't ever steal a space from a primary monitor! https://github.com/paperwm/PaperWM/pull/577/commits/935760462c503b33674cc0cda5e4da023b3db465.

jtaala commented 10 months ago

See latest - we now persist and restore viewport position (e.g. tiling position) for each space as well - for a seamless restore (i.e. will look just like before disabling).

This PR turned into a much larger PR than first thought, but in general I'm now happy with the behaviour of restores and am planning on merging this in soon (next couple of day or so).