Skip to content

Coalesce idempotent refreshSession calls triggered by event handlers #1249

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.

Sign up for GitHub

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

Closed
rickyz wants to merge 1 commit into from
Closed

Coalesce idempotent refreshSession calls triggered by event handlers #1249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Sources/AppBundle/GlobalObserver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class GlobalObserver {
}
let notifName = notification.name.rawValue
MainActor.assumeIsolated {
refreshAndLayout(.globalObserver(notifName), screenIsDefinitelyUnlocked: false)
scheduleRefreshAndLayout(.globalObserver(notifName), screenIsDefinitelyUnlocked: false)
}
}

Expand Down Expand Up @@ -67,7 +67,7 @@ class GlobalObserver {
// Detect close button clicks for unfocused windows. Yes, kAXUIElementDestroyedNotification is that unreliable
// And trigger new window detection that could be delayed due to mouseDown event
default:
refreshAndLayout(.globalObserverLeftMouseUp, screenIsDefinitelyUnlocked: true)
scheduleRefreshAndLayout(.globalObserverLeftMouseUp, screenIsDefinitelyUnlocked: true)
}
}
}
Expand Down
21 changes: 20 additions & 1 deletion Sources/AppBundle/layout/refresh.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,25 @@ func refreshAndLayout(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked:
refreshSession(event, screenIsDefinitelyUnlocked: screenIsDefinitelyUnlocked, startup: startup, body: {})
}

@MainActor private var havePendingRefresh = false
@MainActor private var pendingRefreshScreenIsDefinitelyUnlocked = false

@MainActor
func scheduleRefreshAndLayout(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked: Bool = false) {
if havePendingRefresh {
if screenIsDefinitelyUnlocked {
pendingRefreshScreenIsDefinitelyUnlocked = true
}
return
Comment on lines +56 to +59
Copy link
Copy Markdown
Owner

@nikitabobko nikitabobko Apr 1, 2025

Choose a reason for hiding this comment

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

This if proves that it's not 100% idempotent :/

screenIsDefinitelyUnlocked is definitely a hack anyway that I hope to get rid of in scope #1215 + #1216

}
havePendingRefresh = true
pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
Comment on lines +55 to +62
Copy link
Copy Markdown
Owner

@nikitabobko nikitabobko Apr 1, 2025

Choose a reason for hiding this comment

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

It feels more natural to me to coalesce the latest screenIsDefinitelyUnlocked state into pendingRefreshScreenIsDefinitelyUnlocked in whatever case:

Suggested change
if havePendingRefresh {
if screenIsDefinitelyUnlocked {
pendingRefreshScreenIsDefinitelyUnlocked = true
}
return
}
havePendingRefresh = true
pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
if havePendingRefresh { return }
havePendingRefresh = true

This way we will run the next refresh session with the latest known screenIsDefinitelyUnlocked

wdyt?

Copy link
Copy Markdown
Collaborator Author

@rickyz rickyz Apr 1, 2025

Choose a reason for hiding this comment

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

My logic for preferring true here was that the comments at

AeroSpace/Sources/AppBundle/tree/frozen/closedWindowsCache.swift

Lines 111 to 124 in ae0e577

// Consider the following case:
// 1. Close window
// 2. The previous step lead to caching the whole world
// 3. Change something in the layout
// 4. Lock the screen
// 5. The cache won't be updated because all alive windows are already cached
// 6. Unlock the screen
// 7. The wrong cache is used
//
// 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() {
closedWindowsCache = FrozenWorld(workspaces: [], monitors: [])
}
suggest that resetting the closed window cache on relayout is critical for correctness. Because of that, I was worried that losing a reset via

AeroSpace/Sources/AppBundle/layout/refresh.swift

Line 12 in ae0e577

if screenIsDefinitelyUnlocked { resetClosedWindowsCache() }
could potentially trigger that buggy sequence.

Copy link
Copy Markdown
Owner

@nikitabobko nikitabobko Apr 3, 2025

Choose a reason for hiding this comment

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

Thanks, makes sense.

Merged with the following cleanup patch:

diff --git a/Sources/AppBundle/initAppBundle.swift b/Sources/AppBundle/initAppBundle.swift
index 38751b79..c64d0fc8 100644
--- a/Sources/AppBundle/initAppBundle.swift
+++ b/Sources/AppBundle/initAppBundle.swift
@@ -23,7 +23,7 @@ import Foundation
     AXUIElementSetMessagingTimeout(AXUIElementCreateSystemWide(), 1.0)
     startUnixSocketServer()
     GlobalObserver.initObserver()
-    refreshAndLayout(.startup1, screenIsDefinitelyUnlocked: false, startup: true)
+    refreshSession(.startup1, screenIsDefinitelyUnlocked: false, startup: true, body: {})
     refreshSession(.startup2, screenIsDefinitelyUnlocked: false) {
         if serverArgs.startedAtLogin {
             _ = config.afterLoginCommand.runCmdSeq(.defaultEnv, .emptyStdin)
diff --git a/Sources/AppBundle/layout/refresh.swift b/Sources/AppBundle/layout/refresh.swift
index 8ac24370..91cafb31 100644
--- a/Sources/AppBundle/layout/refresh.swift
+++ b/Sources/AppBundle/layout/refresh.swift
@@ -42,27 +42,16 @@ func refreshSession<T>(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked:
     return result
 }

-@MainActor
-func refreshAndLayout(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked: Bool, startup: Bool = false) {
-    refreshSession(event, screenIsDefinitelyUnlocked: screenIsDefinitelyUnlocked, startup: startup, body: {})
-}
-
 @MainActor private var havePendingRefresh = false
-@MainActor private var pendingRefreshScreenIsDefinitelyUnlocked = false

 @MainActor
 func scheduleRefreshAndLayout(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked: Bool = false) {
-    if havePendingRefresh {
-        if screenIsDefinitelyUnlocked {
-            pendingRefreshScreenIsDefinitelyUnlocked = true
-        }
-        return
-    }
+    if screenIsDefinitelyUnlocked { resetClosedWindowsCache() }
+    if havePendingRefresh { return }
     havePendingRefresh = true
-    pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
     DispatchQueue.main.async { @MainActor in
         havePendingRefresh = false
-        refreshSession(event, screenIsDefinitelyUnlocked: pendingRefreshScreenIsDefinitelyUnlocked, startup: false, body: {})
+        refreshSession(event, screenIsDefinitelyUnlocked: false, startup: false, body: {})
     }
 }

DispatchQueue.main.async { @MainActor in
havePendingRefresh = false
refreshSession(event, screenIsDefinitelyUnlocked: pendingRefreshScreenIsDefinitelyUnlocked, startup: false, body: {})
}
}

@MainActor
func refreshModel() {
gc()
Expand Down Expand Up @@ -81,7 +100,7 @@ func refreshObs(_ obs: AXObserver, ax: AXUIElement, notif: CFString, data: Unsaf
check(Thread.isMainThread)
let notif = notif as String
MainActor.assumeIsolated {
refreshAndLayout(.ax(notif), screenIsDefinitelyUnlocked: false)
scheduleRefreshAndLayout(.ax(notif), screenIsDefinitelyUnlocked: false)
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/AppBundle/mouse/moveWithMouse.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ func movedObs(_ obs: AXObserver, ax: AXUIElement, notif: CFString, data: UnsafeM
if let windowId, let window = Window.get(byId: windowId), TrayMenuModel.shared.isEnabled {
moveWithMouseIfTheCase(window)
}
refreshAndLayout(.ax(notif), screenIsDefinitelyUnlocked: false)
scheduleRefreshAndLayout(.ax(notif), screenIsDefinitelyUnlocked: false)
}
}

Expand Down
4 changes: 2 additions & 2 deletions Sources/AppBundle/mouse/resizeWithMouse.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ func resizedObs(_ obs: AXObserver, ax: AXUIElement, notif: CFString, data: Unsaf
if let windowId, let window = Window.get(byId: windowId), TrayMenuModel.shared.isEnabled {
resizeWithMouseIfTheCase(window)
}
refreshAndLayout(.ax(notif), screenIsDefinitelyUnlocked: false)
scheduleRefreshAndLayout(.ax(notif), screenIsDefinitelyUnlocked: false)
}
}

Expand All @@ -21,7 +21,7 @@ func resetManipulatedWithMouseIfPossible() {
for workspace in Workspace.all {
workspace.resetResizeWeightBeforeResizeRecursive()
}
refreshAndLayout(.resetManipulatedWithMouse, screenIsDefinitelyUnlocked: true)
scheduleRefreshAndLayout(.resetManipulatedWithMouse, screenIsDefinitelyUnlocked: true)
}
}

Expand Down
Loading