- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 335
Auto reload config #1743
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.
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
base: main
Are you sure you want to change the base?
Auto reload config #1743
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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? | ||||||||||||||||||||||||||||||||
| private var debounceTask: Task<Void, Never>? | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need our own debouncer? Isn't the  | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| func startWatching(configUrl: URL) { | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You always pass the same parameter here There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:  | ||||||||||||||||||||||||||||||||
| stopWatching() | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| guard config.autoReloadConfig else { return } | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use plain old  + other similar  | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to use clojure capturing over  | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| // 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 { | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
        Suggested change
       
 | ||||||||||||||||||||||||||||||||
| let flags = eventFlags[i] | ||||||||||||||||||||||||||||||||
| let isFile = (flags & FSEventStreamEventFlags(kFSEventStreamEventFlagItemIsFile)) != 0 | ||||||||||||||||||||||||||||||||
| let isRelevant = (flags & FSEventStreamEventFlags( | ||||||||||||||||||||||||||||||||
| kFSEventStreamEventFlagItemModified | | ||||||||||||||||||||||||||||||||
| kFSEventStreamEventFlagItemRenamed | | ||||||||||||||||||||||||||||||||
| kFSEventStreamEventFlagItemCreated | | ||||||||||||||||||||||||||||||||
| kFSEventStreamEventFlagItemChangeOwner, | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the docs, one can also pass  | ||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe 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  | ||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| eventStream = stream | ||||||||||||||||||||||||||||||||
| 
      Comment on lines
    
      +30
     to 
      +92
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this  | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| lastModificationDate = modDate | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| debounceTask?.cancel() | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| // Debounce the reload to handle atomic save sequences (write → rename → swap) | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
 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
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
        Suggested change
       
 The  | ||||||||||||||||||||||||||||||||
| // Reload config within a session to ensure layout refresh happens | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
        Suggested change
       
 + inline  Given that you never assign  | ||||||||||||||||||||||||||||||||
|  | ||||||||||||||||||||||||||||||||
| @MainActor | ||||||||||||||||||||||||||||||||
| func stopConfigFileWatcher() { | ||||||||||||||||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused function | ||||||||||||||||||||||||||||||||
| _configFileWatcher?.stopWatching() | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -14,6 +14,7 @@ import Foundation | |
| if !reloadConfig() { | ||
| check(reloadConfig(forceConfigUrl: defaultConfigUrl)) | ||
| } | ||
| startConfigFileWatcher() | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|  | ||
There was a problem hiding this comment.
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