Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a broad cleanup/hardening pass for MonitorControl that addresses thread-safety hazards in software brightness/gamma handling, crash-prone force-unwraps, DDC read/write reliability, and several memory/code-hygiene issues, while modernizing a few deprecated APIs.
Changes:
- Thread/concurrency hardening for software brightness, shade updates, gamma table writes, and the configure pipeline (with
configureID/reconfigureID/sleepIDstaleness guards moving DDC setup off the main thread). - DDC reliability fixes: persist write state only after success, validate Arm64 DDC reply header, cap retries at 30, clear stale
arm64avServicehandles before rematching, guard divide-by-zero in DDC normalization. - Removals/replacements: dead
sortDisplays()andshadeGraveleak gone, tautologicalself.percentageBox == self.percentageBoxchecks removed, duplicate audio default removed,SMLoginItemSetEnabled→SMAppServiceandkIOMasterPortDefault→kIOMainPortDefault(one site),getSystemSettingsswitched from direct plist read toUserDefaults.persistentDomain.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| MonitorControl/Support/SliderHandler.swift | Drops two tautological self.percentageBox == self.percentageBox guards. |
| MonitorControl/Support/MenuHandler.swift | Replaces currentDisplay! force-unwrap with optional-aware filtering for "relevant" sliders. |
| MonitorControl/Support/DisplayManager.swift | Removes shadeGrave leak and dead sortDisplays(); clears stale arm64 service handles before rematching. |
| MonitorControl/Support/Arm64DDC.swift | Validates DDC reply header, caps retries at 30, switches to kIOMainPortDefault on macOS 12+. |
| MonitorControl/Support/AppDelegate.swift | Adds configureID staleness guard and moves setupOtherDisplays to a background queue; migrates login-item registration to SMAppService; rewrites getSystemSettings via UserDefaults. |
| MonitorControl/Model/OtherDisplay.swift | Only persists writeDDCLastSavedValue/isTouched after the DDC write actually succeeds; guards divide-by-zero in DDC value (de)normalization; drops duplicate audio default. |
| MonitorControl/Model/Display.swift | Handles CGGetDisplayTransferByTable failure, guards zero defaultGammaTablePeak, signals swBrightnessSemaphore before async smooth dispatch, wraps shade/gamma updates with DispatchQueue.main.async/sync, defers gamma-interference alert. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DispatchQueue.main.sync { | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| } |
| func getSystemSettings() -> [String: Any]? { | ||
| UserDefaults.standard.persistentDomain(forName: ".GlobalPreferences") | ||
| } |
| let attempts = max(1, Int(min(numOfRetryAttemps ?? 4, UInt8(30)))) | ||
| for _ in 0 ..< attempts { |
| if #available(macOS 13.0, *) { | ||
| let service = SMAppService.loginItem(identifier: identifier) | ||
| do { | ||
| if enabled { | ||
| try service.register() | ||
| } else { | ||
| try service.unregister() | ||
| } | ||
| } catch { | ||
| os_log("Unable to update login item registration: %{public}@", type: .error, error.localizedDescription) | ||
| } | ||
| } else { | ||
| SMLoginItemSetEnabled(identifier as CFString, enabled) | ||
| } |
| let attempts = max(1, Int(min(numOfRetryAttemps ?? 4, UInt8(30)))) | ||
| for _ in 0 ..< attempts { |
| if smooth { | ||
| self.swBrightnessSemaphore.signal() | ||
| DispatchQueue.global(qos: .userInteractive).async { | ||
| for transientValue in stride(from: currentValue, to: newValue, by: 0.005 * (currentValue > newValue ? -1 : 1)) { | ||
| guard app.reconfigureID == 0 else { | ||
| self.swBrightnessSemaphore.signal() | ||
| return | ||
| } | ||
| if self.isVirtual || self.readPrefAsBool(key: .avoidGamma) { | ||
| _ = DisplayManager.shared.setShadeAlpha(value: 1 - transientValue, displayID: DisplayManager.resolveEffectiveDisplayID(self.identifier)) | ||
| DispatchQueue.main.async { | ||
| _ = DisplayManager.shared.setShadeAlpha(value: 1 - transientValue, displayID: DisplayManager.resolveEffectiveDisplayID(self.identifier)) | ||
| } | ||
| } else { | ||
| let gammaTableRed = self.defaultGammaTableRed.map { $0 * transientValue } | ||
| let gammaTableGreen = self.defaultGammaTableGreen.map { $0 * transientValue } | ||
| let gammaTableBlue = self.defaultGammaTableBlue.map { $0 * transientValue } | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| DispatchQueue.main.sync { | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| } | ||
| } | ||
| Thread.sleep(forTimeInterval: 0.001) // Let's make things quick if not performed in the background | ||
| } | ||
| } | ||
| return true |
| DispatchQueue.main.sync { | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| } |
| let identifier = "\(Bundle.main.bundleIdentifier!)Helper" | ||
| if #available(macOS 13.0, *) { | ||
| let service = SMAppService.loginItem(identifier: identifier) | ||
| do { | ||
| if enabled { | ||
| try service.register() | ||
| } else { | ||
| try service.unregister() | ||
| } | ||
| } catch { | ||
| os_log("Unable to update login item registration: %{public}@", type: .error, error.localizedDescription) | ||
| } | ||
| } else { | ||
| SMLoginItemSetEnabled(identifier as CFString, enabled) | ||
| } |
| let masterPort: mach_port_t | ||
| if #available(macOS 12.0, *) { | ||
| masterPort = kIOMainPortDefault | ||
| } else { | ||
| masterPort = kIOMasterPortDefault | ||
| } | ||
| let ioregRoot: io_registry_entry_t = IORegistryGetRootEntry(masterPort) |
| var writeAttempted = false | ||
| var writeSucceeded = true | ||
| for controlCode in controlCodes { | ||
| if Arm64DDC.isArm64 { | ||
| if self.arm64ddc { | ||
| _ = Arm64DDC.write(service: self.arm64avService, command: controlCode, value: value) | ||
| writeAttempted = true | ||
| writeSucceeded = Arm64DDC.write(service: self.arm64avService, command: controlCode, value: value) | ||
| } | ||
| } else { | ||
| _ = self.ddc?.write(command: controlCode, value: value, errorRecoveryWaitTime: 2000) ?? false | ||
| if self.ddc != nil { | ||
| writeAttempted = true | ||
| writeSucceeded = self.ddc?.write(command: controlCode, value: value, errorRecoveryWaitTime: 2000) ?? false | ||
| } | ||
| } | ||
| if writeAttempted, !writeSucceeded { | ||
| break | ||
| } | ||
| } | ||
| if writeAttempted, writeSucceeded { | ||
| self.writeDDCQueue.async(flags: .barrier) { | ||
| self.writeDDCLastSavedValue[command] = value | ||
| self.savePref(true, key: PrefKey.isTouched, for: command) | ||
| } | ||
| } |
waydabber
added
the
AI contribution / fork recommendation
|
The black screen issue is usually caused by a malformed custom color profile and the app using color table adjustments. It's a longstanding macOS bug - this mix results in a black screen (entirely undetectable by the app as the color table looks fine, just nothing is visible on screen 🤣). |
Thread safety: - Fix swBrightnessSemaphore misuse causing races in software brightness - Signal semaphore before async dispatch to prevent deadlock with polling job - Wrap CGSetDisplayTransferByTable in DispatchQueue.main.sync for thread safety - Wrap shade alpha updates in DispatchQueue.main.async for AppKit safety - Move DDC probe work off main thread with configureID staleness guard - Defer gamma interference alert to next run loop iteration DDC reliability: - Only persist DDC write state after confirming hardware write succeeded - Clear stale arm64avService handles before rematching on wake/reconfig - Validate DDC/CI reply header fields before accepting read values - Cap DDC retry attempts at 30 to prevent multi-minute startup hangs - Guard against division by zero in DDC value normalization Crash fixes: - Replace force-unwrap on currentDisplay in MenuHandler - Handle gamma table acquisition failure safely - Guard against zero defaultGammaTablePeak in getSwBrightness Cleanup: - Remove shadeGrave memory leak (closed windows retained forever) - Remove dead sortDisplays() with stray Turkish comments - Remove duplicate audioSpeakerVolume DDC default assignment - Remove tautological self-comparisons in SliderHandler - Replace deprecated SMLoginItemSetEnabled with SMAppService on macOS 13+ - Replace deprecated kIOMasterPortDefault with kIOMainPortDefault on macOS 12+ - Replace direct plist read with UserDefaults.persistentDomain API
- Drop DispatchQueue.main.sync around CGSetDisplayTransferByTable in the smooth-brightness loop. CoreGraphics does not require the main thread here, and hopping per gamma step (~200 iterations) both stalled the animation and contended with main-thread UI work. - Address the global preferences domain via UserDefaults.globalDomain instead of the literal ".GlobalPreferences", which is a filename and not a CFPreferences application ID. The old form returned nil, so the volume-changed beep silently never played. - Restore the original DDC attempt count. The retry cap regressed the default from 5 attempts to 4 (and minimal polling from 2 to 1); it is now min(attempts + 1, 30), preserving prior behaviour under the cap. - Migrate the login-item status read in MainPrefsViewController to SMAppService.loginItem(_:).status on macOS 13+. The writer had moved to SMAppService while the reader still used SMCopyAllJobDictionaries, so the Start at login checkbox showed stale state. - Track DDC write success across every attempted control code rather than letting the last iteration win, so writeDDCLastSavedValue and isTouched are persisted only when all attempted writes actually succeeded. kIOMainPortDefault is intentionally changed only in Arm64DDC, where it is guarded by an availability check. The remaining call sites stay on kIOMasterPortDefault because the project's deployment target predates macOS 12.
ab7c18f to
5afd215
Compare
|
Rebased onto current Fixed
Narrowed rather than expanded
Left open deliberately Signalling Unrelated build note A clean checkout of |
|
Thanks, that is genuinely useful, and it sits close to the code this PR touches. I read through the linked discussion. To be clear about scope: nothing here changes whether gamma table control is used, only how the writes are sequenced and how failures are handled. The One change is adjacent to your failure mode without actually addressing it. One question, since you have far more history with this than I do: given the profile interaction, is there an argument for defaulting |
|
It may be a bit of an overkill, as not all custom profiles are inherently wrong - the problem manifests itself with some third party app created ones. But maybe a warning could be added about it. Alternatively, defaulting to avoidGamma may make sense until it's explicitly turned off, but since the user can switch profiles any time, this is not a very safe approach if the goal is to deal with this, so instead of defaulting to something, maybe an explicit switch would be needed that enables gamma table adjustment with a custom profile. |
Summary
Fix pass addressing thread safety, DDC communication reliability, crash-inducing edge cases, and resource leaks. Rebased onto current
main; review feedback addressed in a follow-up commit.Thread Safety
DDC Reliability
Crash Fixes
Cleanup
Testing
Tested on an M3 Max with four displays (built-in plus three external TVs over HDMI and DisplayLink). Verified smooth brightness, sleep/wake recovery, and slider responsiveness. Surgical fixes only, no new features and no UI changes.
Note: a clean checkout of
maincurrently fails to build on recent Xcode because MACOSX_DEPLOYMENT_TARGET is 10.14 while Xcode floors at 12.0. That is pre-existing and untouched here; builds were verified with an override.