GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fix concurrency bugs, DDC reliability, crashes, and memory leaks by MyronKoch · Pull Request #1874 · MonitorControl/MonitorControl Conversation
|
| 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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) | ||
| } |
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.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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) | ||
| } | ||
| } |
|
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 🤣). |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch has not been deployed
No deploymentsReviewers
At least 1 approving review is required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.