sway monitor fixes upstream#605
Conversation
Harden I3/Sway IPC monitor/workspace lifetime handling: avoid stale cross-pointers, use focused-monitor setter consistently, and guard focused monitor dereference during workspace removal.
outfoxxed
left a comment
outfoxxed
left a comment
There was a problem hiding this comment.
There are two separate changes here. In the future please make one PR per isolated change so unrelated changes aren't blocked on reviews of other changes.
| if (this->bFocusedWorkspace == oldWorkspace) { | ||
| this->bFocusedMonitor->setFocusedWorkspace(nullptr); | ||
| if (auto* focusedMonitor = this->bFocusedMonitor.value()) { | ||
| focusedMonitor->setFocusedWorkspace(nullptr); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is redundant with the destroy signal path.
There was a problem hiding this comment.
Agreed this null-check looks redundant with the existing binding and destroy handling. I added it defensively while debugging and never removed it.
There was a problem hiding this comment.
No the whole if is redundant because the monitor will be updated by the destroy signal
src/x11/i3/ipc/monitor.cpp
Outdated
| void I3Monitor::setFocusedWorkspace(I3Workspace* workspace) { this->setActiveWorkspace(workspace); }; | ||
|
|
||
| void I3Monitor::setActiveWorkspace(I3Workspace* workspace) { |
There was a problem hiding this comment.
Is there a reason to have one function that just calls the other one here?
There was a problem hiding this comment.
I left it this way to stay consistent with activeWorkspace, and to keep this fix small without touching a bunch of call sites. Mostly just a naming thing.
There was a problem hiding this comment.
Just move the body to setFocusedWorkspace or delete setFocusedWorkspace
| if (auto* focusedMonitor = this->bFocusedMonitor.value()) { | ||
| focusedMonitor->setFocusedWorkspace(nullptr); | ||
| } | ||
| this->bFocusedMonitor->setFocusedWorkspace(nullptr); |
There was a problem hiding this comment.
Sorry about all the extra nullptr checks, I was mainly trying to catch the error that was crashing QS whenever I turned off my screens in Sway. I don’t currently run mainline QS anymore since the shell I use has moved to its own fork, and this patch has been working well there with no more crashes. Please feel free to adjust this PR as needed.
2 participants
When monitors were turned off quick shell would crash. When it went into lock it would red screen. This fixes that.
This PR fixes two issues seen on Sway when monitors are powered off/on:
Root Cause
The i3/Sway IPC path was keeping cross-references between monitor/workspace objects that could become stale during rapid output/workspace updates.
Panel windows that are recreated on invisibility could lose requested-visible intent during compositor-driven hide/recreate cycles.