Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Sources/AppBundle/command/impl/ReloadConfigCommand.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ struct ReloadConfigCommand: Command {
configUrl = url
activateMode(activeMode)
syncStartAtLogin()
startConfigFileWatcher()
MessageModel.shared.message = nil
}
return true
Expand Down
1 change: 1 addition & 0 deletions Sources/AppBundle/config/Config.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ struct Config: ConvenienceCopyable {
var defaultRootContainerLayout: Layout = .tiles
var defaultRootContainerOrientation: DefaultContainerOrientation = .auto
var startAtLogin: Bool = false
var autoReloadConfig: Bool = false
var automaticallyUnhideMacosHiddenApps: Bool = false
var accordionPadding: Int = 30
var enableNormalizationOppositeOrientationForNestedContainers: Bool = true
Expand Down
159 changes: 159 additions & 0 deletions Sources/AppBundle/config/ConfigFileWatcher.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import AppKit
import Common
import Foundation
import CoreServices

@MainActor
class ConfigFileWatcher {
private var eventStream: FSEventStreamRef?
private var watchedFileUrl: URL?
private var lastModificationDate: Date?
Comment on lines +9 to +10
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like these two properties can be passed as simple parameter or simply captured by closures they are used in. No need to be "global" class state

private var debounceTask: Task<Void, Never>?
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need our own debouncer? Isn't the 0.3 that you pass to FSEventStreamCreate already sufficient?


func startWatching(configUrl: URL) {
Copy link
Owner

Choose a reason for hiding this comment

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

You always pass the same parameter here

Copy link
Owner

Choose a reason for hiding this comment

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

Does this logic handle the cases when the config changes its location? We have several possible locations: ~/.aerospace.toml, ~/.config/aerospace/aerospace.toml

stopWatching()

guard config.autoReloadConfig else { return }
Copy link
Owner

Choose a reason for hiding this comment

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

Please use plain old if where possible. I only use guard for guard let/guard case

+ other similar guard usages in this PR


guard configUrl != defaultConfigUrl else { return }

watchedFileUrl = configUrl

if let attrs = try? FileManager.default.attributesOfItem(atPath: configUrl.path),
let modDate = attrs[.modificationDate] as? Date
{
lastModificationDate = modDate
}

let directoryPath = configUrl.deletingLastPathComponent().path
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we watch the config directory path, and not the config itself?


var context = FSEventStreamContext(
version: 0,
info: Unmanaged.passUnretained(self).toOpaque(),
retain: nil,
release: nil,
copyDescription: nil,
)

let pathsToWatch = [directoryPath] as CFArray
let callback: FSEventStreamCallback = { (
streamRef: ConstFSEventStreamRef,
clientCallBackInfo: UnsafeMutableRawPointer?,
numEvents: Int,
eventPaths: UnsafeMutableRawPointer,
eventFlags: UnsafePointer<FSEventStreamEventFlags>,
eventIds: UnsafePointer<FSEventStreamEventId>,
) in
guard let info = clientCallBackInfo else { return }
let watcher = Unmanaged<ConfigFileWatcher>.fromOpaque(info).takeUnretainedValue()
Comment on lines +47 to +48
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to use clojure capturing over clientCallBackInfo: UnsafeMutableRawPointer if possible


// Filter events to only react to our specific file
let paths = unsafeBitCast(eventPaths, to: NSArray.self) as? [String] ?? []
guard let watchedPath = watcher.watchedFileUrl?.path else { return }

for i in 0 ..< numEvents {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
for i in 0 ..< numEvents {
check(numEvents == eventFlags.count && numEvents == paths.count)
for i in 0 ..< numEvents {

let flags = eventFlags[i]
let isFile = (flags & FSEventStreamEventFlags(kFSEventStreamEventFlagItemIsFile)) != 0
let isRelevant = (flags & FSEventStreamEventFlags(
kFSEventStreamEventFlagItemModified |
kFSEventStreamEventFlagItemRenamed |
kFSEventStreamEventFlagItemCreated |
kFSEventStreamEventFlagItemChangeOwner,
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to track owner change?

)) != 0

if isFile && isRelevant && paths[i] == watchedPath {
// Already on main queue
Copy link
Owner

Choose a reason for hiding this comment

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

What does this comment relate to?

watcher.handleFileChange()
break
}
}
}

guard let stream = FSEventStreamCreate(
kCFAllocatorDefault,
Copy link
Owner

Choose a reason for hiding this comment

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

According to the docs, one can also pass null with the same effect. null looks much simplier alternative to me

callback,
&context,
pathsToWatch,
FSEventStreamEventId(kFSEventStreamEventIdSinceNow),
0.3, // latency in seconds, letting the system coalesce events
FSEventStreamCreateFlags(kFSEventStreamCreateFlagFileEvents),
) else {
return
}

FSEventStreamSetDispatchQueue(stream, DispatchQueue.main)

if !FSEventStreamStart(stream) {
FSEventStreamInvalidate(stream)
FSEventStreamRelease(stream)
Copy link
Owner

Choose a reason for hiding this comment

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

Do we really need to manually work with the reference counter here?

I am not a Swift/Apple-runtime expert, but when I did my research last time, I figured that we don't need to manually call CFRelease. Swift/runtime does it for us unless we create the Core Foundation object ourself. I wonder if it's the same case with FSEventStream

return
}

eventStream = stream
Comment on lines +30 to +92
Copy link
Owner

Choose a reason for hiding this comment

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

It would help me to review this PR if you could share how you wrote this code so that I could follow along.

Did you copy the code from somewhere?
Did you google the API and then followed the docs?
Did you generate it with LLM?
Something else?

Did you verify the code yourself by reading the docs after you wrote it?

}

func stopWatching() {
debounceTask?.cancel()
debounceTask = nil

if let stream = eventStream {
FSEventStreamStop(stream)
FSEventStreamInvalidate(stream)
FSEventStreamRelease(stream)
eventStream = nil
}

watchedFileUrl = nil
lastModificationDate = nil
}

private func handleFileChange() {
guard let url = watchedFileUrl else {
return
}

guard FileManager.default.fileExists(atPath: url.path) else {
return
}

guard let attrs = try? FileManager.default.attributesOfItem(atPath: url.path),
let modDate = attrs[.modificationDate] as? Date
else {
return
}

guard modDate > (lastModificationDate ?? .distantPast) else {
return
}
Comment on lines +125 to +127
Copy link
Owner

Choose a reason for hiding this comment

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

What is this guard for? Do we have reasons not to trust the file watcher API?


lastModificationDate = modDate

debounceTask?.cancel()

// Debounce the reload to handle atomic save sequences (write → rename → swap)
Copy link
Owner

Choose a reason for hiding this comment

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

to handle atomic save sequences (write → rename → swap)

I don't understand what you mean

What is renamed? What is swapped with what?

debounceTask = Task { @MainActor in
try? await Task.sleep(for: .milliseconds(300))
guard !Task.isCancelled else { return }
Comment on lines +135 to +136
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
try? await Task.sleep(for: .milliseconds(300))
guard !Task.isCancelled else { return }
try await Task.sleep(for: .milliseconds(300))

The try here is exactly to check for cancellation

// Reload config within a session to ensure layout refresh happens
Copy link
Owner

Choose a reason for hiding this comment

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

Useless comment

guard let token: RunSessionGuard = .isServerEnabled else { return }
try? await runSession(.autoReloadConfig, token) {
_ = reloadConfig(forceConfigUrl: url)
}
}
}
}

@MainActor private var _configFileWatcher: ConfigFileWatcher?

@MainActor
func startConfigFileWatcher() {
if _configFileWatcher == nil {
_configFileWatcher = ConfigFileWatcher()
}
_configFileWatcher?.startWatching(configUrl: configUrl)
}
Comment on lines +146 to +154
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
@MainActor private var _configFileWatcher: ConfigFileWatcher?
@MainActor
func startConfigFileWatcher() {
if _configFileWatcher == nil {
_configFileWatcher = ConfigFileWatcher()
}
_configFileWatcher?.startWatching(configUrl: configUrl)
}
@MainActor private var _configFileWatcher = ConfigFileWatcher()
@MainActor
func startConfigFileWatcher() {
_configFileWatcher.startWatching(configUrl: configUrl)
}

+ inline startConfigFileWatcher function

Given that you never assign nil to the variable, I don't understand what this initialization ceremony is for


@MainActor
func stopConfigFileWatcher() {
Copy link
Owner

Choose a reason for hiding this comment

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

Unused function

_configFileWatcher?.stopWatching()
}
1 change: 1 addition & 0 deletions Sources/AppBundle/config/parseConfig.swift
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ private let configParser: [String: any ParserProtocol<Config>] = [
"default-root-container-orientation": Parser(\.defaultRootContainerOrientation, parseDefaultContainerOrientation),

"start-at-login": Parser(\.startAtLogin, parseBool),
"auto-reload-config": Parser(\.autoReloadConfig, parseBool),
"automatically-unhide-macos-hidden-apps": Parser(\.automaticallyUnhideMacosHiddenApps, parseBool),
"accordion-padding": Parser(\.accordionPadding, parseInt),
"exec-on-workspace-change": Parser(\.execOnWorkspaceChange, parseExecOnWorkspaceChange),
Expand Down
1 change: 1 addition & 0 deletions Sources/AppBundle/initAppBundle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import Foundation
if !reloadConfig() {
check(reloadConfig(forceConfigUrl: defaultConfigUrl))
}
startConfigFileWatcher()
Copy link
Owner

Choose a reason for hiding this comment

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

Your just reloaded the config in the lines above. Config reloading starts the file watcher

Why do you need to start it here again?


checkAccessibilityPermissions()
startUnixSocketServer()
Expand Down
2 changes: 2 additions & 0 deletions Sources/Common/util/commonUtil.swift
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public enum RefreshSessionEvent: Sendable, CustomStringConvertible {
case ax(String)
case onFocusedMonitorChanged
case onFocusChanged
case autoReloadConfig

public var isStartup: Bool {
if case .startup = self { return true } else { return false }
Expand All @@ -94,6 +95,7 @@ public enum RefreshSessionEvent: Sendable, CustomStringConvertible {
case .startup: "startup"
case .onFocusedMonitorChanged: "onFocusedMonitorChanged"
case .onFocusChanged: "onFocusChanged"
case .autoReloadConfig: "autoReloadConfig"
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions docs/config-examples/default-config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ after-startup-command = []
# Start AeroSpace at login
start-at-login = false

# Automatically reload the config when the config file is changed
auto-reload-config = false

# Normalizations. See: https://nikitabobko.github.io/AeroSpace/guide#normalization
enable-normalization-flatten-containers = true
enable-normalization-opposite-orientation-for-nested-containers = true
Expand Down
Loading