Preserve workspace monitor assignments #1522
Conversation
Can you please describe exact steps to reproduce? Are the steps reproducible every time? I usually have my lid open with external monitor, so it may be the case that some scenario doesn't work as expected I've tried the following:
Windows to workspaces assignment is not lost (everything works as expected) |
|
And I don't understand why you change the logic of workspace to monitor assignment if the described bug is in windows to workspaces assignment |
It's not 100% reproducible. Most of the time the disconnect of the external Display seems to be the cause, but I can't 100% reproduce it, there must be a factor that I still couldn't identify.
If it's not clear from the description: I only use one display at a time, the built-in or the external display. All my workspaces are on that display and should switch between the two. |
It looks like unplugging the external Display is the trigger for this bug. I read the Aerospace code a bit and tried to understand the logic and flow. My pull request seems to solve the problem, so it couldn't be completely wrong. I just read the code of rearrangeWorkspacesOnMonitors again: is it possible that the assigments in line 190 & 191 |
|
Thanks, Workspace-to-monitor (w-t-m) assignment and windows-to-workspace (w-t-w) assignment are two unrelated subsystems. You can have no workspaces assigned to any monitors, but w-t-w assignment will still hold. You can see it Sorry, but there is still too much mystery around the patch. We are trying to fix the bug in w-t-w assignment, but your patch is about w-t-m assignment. I know and agree that there are hardly catchable bugs when all the windows may loose their workspace assignment. Tracking issue for this: #1216 I do have my own plans on how to fix it in a brute force way: #1215 which should fix your bug as well, but it will take much more time to implement it. I don't mind temporary solutions though.
It's easy to confirm it if it's really the case. Can you please try running with this logging? diff --git a/Sources/AppBundle/layout/refresh.swift b/Sources/AppBundle/layout/refresh.swift
index ea28beaf..b19c08f7 100644
--- a/Sources/AppBundle/layout/refresh.swift
+++ b/Sources/AppBundle/layout/refresh.swift
@@ -10,6 +10,7 @@ func runRefreshSession(
screenIsDefinitelyUnlocked: Bool, // todo rename
optimisticallyPreLayoutWorkspaces: Bool = false,
) {
+ print("# runRefreshSession \(NSScreen.screens.count)")
if screenIsDefinitelyUnlocked { resetClosedWindowsCache() }
activeRefreshTask?.cancel()
activeRefreshTask = Task { @MainActor in
diff --git a/Sources/AppBundle/tree/Workspace.swift b/Sources/AppBundle/tree/Workspace.swift
index e499a782..1ec72af8 100644
--- a/Sources/AppBundle/tree/Workspace.swift
+++ b/Sources/AppBundle/tree/Workspace.swift
@@ -170,6 +170,7 @@ private func rearrangeWorkspacesOnMonitors() {
var oldVisibleScreens: Set<CGPoint> = screenPointToVisibleWorkspace.keys.toSet()
let newScreens = monitors.map(\.rect.topLeftCorner)
+ print("# rearrangeWorkspacesOnMonitors \(NSScreen.screens.count)")
var newScreenToOldScreenMapping: [CGPoint: CGPoint] = [:]
for newScreen in newScreens {
if let oldScreen = oldVisibleScreens.minBy({ ($0 - newScreen).vectorLength }) {
diff --git a/Sources/AppBundle/tree/frozen/closedWindowsCache.swift b/Sources/AppBundle/tree/frozen/closedWindowsCache.swift
index 79541686..cbf54bff 100644
--- a/Sources/AppBundle/tree/frozen/closedWindowsCache.swift
+++ b/Sources/AppBundle/tree/frozen/closedWindowsCache.swift
@@ -120,5 +120,6 @@ private func restoreTreeRecursive(frozenContainer: FrozenContainer, parent: NonL
// That's why we have to reset the cache every time layout changes. The layout can only be changed by running commands
// and with mouse manipulations
@MainActor func resetClosedWindowsCache() {
+ print("# resetClosedWindowsCache \(NSScreen.screens.count)")
closedWindowsCache = FrozenWorld(workspaces: [], monitors: [])
}
I have been playing with the test scenario of unplugging the external monitor when the lid is closed, and I managed to reproduce the bug randomly twice within ~7 minutes, but I could never see the number 0 in my logs If diff --git a/Sources/AppBundle/tree/frozen/closedWindowsCache.swift b/Sources/AppBundle/tree/frozen/closedWindowsCache.swift
index 79541686..730712ef 100644
--- a/Sources/AppBundle/tree/frozen/closedWindowsCache.swift
+++ b/Sources/AppBundle/tree/frozen/closedWindowsCache.swift
@@ -120,5 +120,6 @@ private func restoreTreeRecursive(frozenContainer: FrozenContainer, parent: NonL
// That's why we have to reset the cache every time layout changes. The layout can only be changed by running commands
// and with mouse manipulations
@MainActor func resetClosedWindowsCache() {
+ if NSScreen.screens.count <= 0 { return }
closedWindowsCache = FrozenWorld(workspaces: [], monitors: [])
}
|
|
Thanks, I'll try the logging patch and take a look. But good to know that you could reproduce the bug! That is defined in appBundleUtil (if I read that right) and I think it's wrong, should be + and not - ? And square roots of negative numbers (if y>x) would return NaN? |
The credit goes to Stefan Grothkopp for finding the bug #1522 (comment) Co-authored-by: Stefan Grothkopp <[email protected]>
Oh wow, It has been there since 2023 and went unnoticed 🤦 Thank you, fixed: 6b3b87b |
|
I compiled the current main with the debug prints and I couldn't reproduce the Bug with a few disconnects and reconnects of the display. Keeping my fingers crossed and I'll test for a bit longer. |
|
I am closing this PR as I don't think it fixes the bug. And I think I found the solution for the bug ae8a7eb I'm going to test the fix this whole week to see if it's ok to release it on weekend. You are welcome to test the fix before the release and report back |
|
To make the bug easier to reproduce you can try running with the following patch: diff --git a/Sources/AppBundle/GlobalObserver.swift b/Sources/AppBundle/GlobalObserver.swift
index b1da2224..eeab2bb5 100644
--- a/Sources/AppBundle/GlobalObserver.swift
+++ b/Sources/AppBundle/GlobalObserver.swift
@@ -3,11 +3,11 @@ import Common
class GlobalObserver {
private static func onNotif(_ notification: Notification) {
- // Third line of defence against lock screen window. See: closedWindowsCache
- // Second and third lines of defence are technically needed only to avoid potential flickering
- if (notification.userInfo?[NSWorkspace.applicationUserInfoKey] as? NSRunningApplication)?.bundleIdentifier == lockScreenAppBundleId {
- return
- }
+ // // Third line of defence against lock screen window. See: closedWindowsCache
+ // // Second and third lines of defence are technically needed only to avoid potential flickering
+ // if (notification.userInfo?[NSWorkspace.applicationUserInfoKey] as? NSRunningApplication)?.bundleIdentifier == lockScreenAppBundleId {
+ // return
+ // }
let notifName = notification.name.rawValue
Task { @MainActor in
if !TrayMenuModel.shared.isEnabled { return }
diff --git a/Sources/AppBundle/tree/MacApp.swift b/Sources/AppBundle/tree/MacApp.swift
index d63b66e5..382ceeff 100644
--- a/Sources/AppBundle/tree/MacApp.swift
+++ b/Sources/AppBundle/tree/MacApp.swift
@@ -299,12 +299,12 @@ final class MacApp: AbstractApp {
var result: [UInt32: AxWindow] = windows.threadGuarded
// Second line of defence against lock screen. See the first line of defence: closedWindowsCache
// Second and third lines of defence are technically needed only to avoid potential flickering
- if frontmostAppBundleId != lockScreenAppBundleId {
- result = try result.filter {
- try job.checkCancellation()
- return $0.value.ax.containingWindowId() != nil
- }
+ // if frontmostAppBundleId != lockScreenAppBundleId {
+ result = try result.filter {
+ try job.checkCancellation()
+ return $0.value.ax.containingWindowId() != nil
}
+ // }
for (id, window) in axApp.threadGuarded.get(Ax.windowsAttr) ?? [] {
try job.checkCancellation()
The patch disables some "the screen is locked" heuristics which technically should not be needed and should only help to fight flickering, but right now they also minize the probability of the bug |
When using Aerospace on my MacBook I encountered a bug:
I figured that this might be because MacOS briefly has no display when the external screen is disconnected while the vuilt-in display is also not in use. So all active workspace would get cleaned up.
This pull request fixes that behavior for me (I tested it for about a week in my normal daily use). I think the fix is lines 177-180 where rearrangeWorkspacesOnMonitors returns if no newScreens are present.
The introduction of assignedMonitorName helped to eliminate some unwanted behavior when waking up from sleep with the external display attached. It would often show the windows smaller and then resize them (maybe the built-in display was somehow used during sleep).
I would love to have some feedback on this pull request, as I'm pretty new to swift and also to the Aerospace codebase it could be very flawed. If it's not (or you can fix it) I would also be happy to see it merged, as that bug I try to fix really has been a pain in my daily use.