Refactor display/menu plumbing and fix crash-prone edge cases

MonitorControl/Enums/PrefKey.swift

@@ -33,10 +33,10 @@ enum PrefKey: String {
   // Hide brightness sliders
   case hideBrightness
 
-  // Show volume sliders
+  // Show contrast sliders
   case showContrast
 
-  // Show volume sliders
+  // Hide volume sliders
   case hideVolume
 
   // Lower via software after brightness
@@ -48,10 +48,10 @@ enum PrefKey: String {
   // Do not show sliders for Apple displays (including built-in display) in menu
   case hideAppleFromMenu
 
-  // Disable slider snapping
+  // Enable slider snapping
   case enableSliderSnap
 
-  // Disable slider snapping
+  // Show percentage label next to sliders
   case enableSliderPercent
 
   // Show tick marks for sliders
@@ -160,7 +160,7 @@ enum PrefKey: String {
   // Max command value display
   case curveDDC
 
-  // Is the specific control is set as unavailable for display?
+  // Is the specific control set as unavailable for the display?
   case unavailableDDC
 
   // Invert DDC scale?

MonitorControl/Model/Display.swift

@@ -74,7 +74,7 @@ class Display: Equatable {
   }
 
   private func getKey(key: PrefKey? = nil, for command: Command? = nil) -> String {
-    (key ?? PrefKey.value).rawValue + (command != nil ? String((command ?? Command.none).rawValue) : "") + self.prefsId
+    (key ?? PrefKey.value).rawValue + (command.map { String($0.rawValue) } ?? "") + self.prefsId
   }
 
   init(_ identifier: CGDirectDisplayID, name: String, vendorNumber: UInt32?, modelNumber: UInt32?, serialNumber: UInt32?, isVirtual: Bool = false, isDummy: Bool = false) {
@@ -331,7 +331,7 @@ class Display: Equatable {
       DisplayManager.shared.gammaInterferenceWarningShown = true
       let alert = NSAlert()
       alert.messageText = NSLocalizedString("Is f.lux or similar running?", comment: "Shown in the alert dialog")
-      alert.informativeText = NSLocalizedString("An other app seems to change the brightness or colors which causes issues.\n\nTo solve this, you need to quit the other app or disable gamma control for your displays in LumaControl!", comment: "Shown in the alert dialog")
+      alert.informativeText = NSLocalizedString("Another app seems to be changing the brightness or colors, which causes issues.\n\nTo solve this, quit the other app or disable gamma control for your displays in LumaControl!", comment: "Shown in the alert dialog")
       alert.addButton(withTitle: NSLocalizedString("I'll quit the other app", comment: "Shown in the alert dialog"))
       alert.addButton(withTitle: NSLocalizedString("Disable gamma control for my displays", comment: "Shown in the alert dialog"))
       alert.alertStyle = NSAlert.Style.critical
@@ -359,21 +359,14 @@ class Display: Equatable {
     guard !self.isVirtual, !self.isDummy else {
       return false
     }
-    if self.getSwBrightness() < 1 {
-      return true
-    }
-    return false
+    return self.getSwBrightness() < 1
   }
 
   func refreshBrightness() -> Float {
     0
   }
 
   func isBuiltIn() -> Bool {
-    if CGDisplayIsBuiltin(self.identifier) != 0 {
-      return true
-    } else {
-      return false
-    }
+    CGDisplayIsBuiltin(self.identifier) != 0
   }
 }

MonitorControl/Model/OtherDisplay.swift

@@ -19,11 +19,18 @@ class OtherDisplay: Display {
       case PollingMode.minimal.rawValue: return 1
       case PollingMode.normal.rawValue: return 5
       case PollingMode.heavy.rawValue: return 20
-      case PollingMode.custom.rawValue: return prefs.integer(forKey: PrefKey.pollingCount.rawValue + self.prefsId)
-      default: return PollingMode.none.rawValue
+      // Clamp at the source: a stale negative value saved by an older version must not
+      // reach `UInt(self.pollingCount)` in the setup paths, where the conversion is a
+      // fatal trap. The upper bound matches the Arm64 path's 255-retry cap, so a huge
+      // value cannot hang the Intel read loop for minutes on end. Zero means "no polling",
+      // which is the sensible reading of a bad value. The default arm must also return 0,
+      // NOT `PollingMode.none.rawValue` (-2): an out-of-range stored polling mode falls
+      // through here, and -2 would trap the same way.
+      case PollingMode.custom.rawValue: return min(255, max(0, prefs.integer(forKey: PrefKey.pollingCount.rawValue + self.prefsId)))
+      default: return 0
       }
     }
-    set { prefs.set(newValue, forKey: PrefKey.pollingCount.rawValue + self.prefsId) }
+    set { prefs.set(max(0, newValue), forKey: PrefKey.pollingCount.rawValue + self.prefsId) }
   }
 
   override init(_ identifier: CGDirectDisplayID, name: String, vendorNumber: UInt32?, modelNumber: UInt32?, serialNumber: UInt32?, isVirtual: Bool = false, isDummy: Bool = false) {
@@ -93,13 +100,10 @@ class OtherDisplay: Display {
     var maxDDCValue = UInt16(DDC_MAX_DETECT_LIMIT)
     var currentDDCValue: UInt16
     switch command {
-    case .audioSpeakerVolume: currentDDCValue = UInt16(Float(DDC_MAX_DETECT_LIMIT) * 0.125)
+    case .audioSpeakerVolume: currentDDCValue = UInt16(Float(DDC_MAX_DETECT_LIMIT) * 0.125) // lower default audio value as high volume might rattle the user.
     case .contrast: currentDDCValue = UInt16(Float(DDC_MAX_DETECT_LIMIT) * 0.750)
     default: currentDDCValue = UInt16(Float(DDC_MAX_DETECT_LIMIT) * 1.000)
     }
-    if command == .audioSpeakerVolume {
-      currentDDCValue = UInt16(Float(DDC_MAX_DETECT_LIMIT) * 0.125) // lower default audio value as high volume might rattle the user.
-    }
     os_log("Setting up display %{public}@ for %{public}@", type: .info, String(self.identifier), String(reflecting: command))
     if !self.isSw() {
       if prefs.integer(forKey: PrefKey.startupAction.rawValue) == StartupAction.read.rawValue, self.pollingCount != 0, !app.safeMode {
@@ -259,11 +263,7 @@ class OtherDisplay: Display {
   }
 
   func isSw() -> Bool {
-    if prefs.bool(forKey: PrefKey.forceSw.rawValue + self.prefsId) || self.isSwOnly() {
-      return true
-    } else {
-      return false
-    }
+    prefs.bool(forKey: PrefKey.forceSw.rawValue + self.prefsId) || self.isSwOnly()
   }
 
   let swAfterOsdAnimationSemaphore = DispatchSemaphore(value: 1)
@@ -423,6 +423,12 @@ class OtherDisplay: Display {
     guard app.sleepID == 0, app.reconfigureID == 0, !self.readPrefAsBool(key: .forceSw), !self.readPrefAsBool(key: .unavailableDDC, for: command) else {
       return values
     }
+    // `pollingCount` is clamped at its getter, so callers should never pass a bad value
+    // here. This is defense in depth: zero tries means "don't read", and the loops below
+    // would otherwise also clamp it themselves.
+    if tries == 0 {
+      return nil
+    }
     let controlCodes = self.getRemapControlCodes(command: command)
     let controlCode = controlCodes.count == 0 ? command.rawValue : controlCodes[0]
     if Arm64DDC.isArm64 {
@@ -491,7 +497,7 @@ class OtherDisplay: Display {
     let deNormalizedValue = (maxDDCValue - minDDCValue) * curvedValue + minDDCValue
     var intDDCValue = UInt16(min(max(deNormalizedValue, minDDCValue), maxDDCValue))
     if from > 0, command == Command.audioSpeakerVolume {
-      intDDCValue = max(1, intDDCValue) // Never let sound to mute accidentally, keep it digitally to at digital 1 if needed as muting breaks some displays
+      intDDCValue = max(1, intDDCValue) // Never let the volume mute accidentally; keep the digital value at 1 or higher if needed, as muting breaks some displays
     }
     return intDDCValue
   }

MonitorControl/Support/AppDelegate.swift

@@ -214,7 +214,7 @@ class AppDelegate: NSObject, NSApplicationDelegate {
     guard self.sleepID == 0, dispatchedReconfigureID == self.reconfigureID else {
       return
     }
-    os_log("Request for configuration with reconfigreID %{public}@", type: .info, String(dispatchedReconfigureID))
+    os_log("Request for configuration with reconfigureID %{public}@", type: .info, String(dispatchedReconfigureID))
     self.reconfigureID = 0
     DisplayManager.shared.gammaInterferenceCounter = 0
     DisplayManager.shared.configureDisplays()
@@ -400,8 +400,11 @@ class AppDelegate: NSObject, NSApplicationDelegate {
   }
 
   func setStartAtLogin(enabled: Bool) {
-    let identifier = "\(Bundle.main.bundleIdentifier!)Helper" as CFString
-    SMLoginItemSetEnabled(identifier, enabled)
+    guard let bundleID = Bundle.main.bundleIdentifier else {
+      os_log("Cannot toggle start at login without a bundle identifier.", type: .error)
+      return
+    }
+    SMLoginItemSetEnabled("\(bundleID)Helper" as CFString, enabled)
   }
 
   func getSystemSettings() -> [String: AnyObject]? {

MonitorControl/Support/Arm64DDC.swift

@@ -70,17 +70,14 @@ class Arm64DDC: NSObject {
   }
 
   static func read(service: IOAVService?, command: UInt8, writeSleepTime: UInt32? = nil, numOfWriteCycles: UInt8? = nil, readSleepTime: UInt32? = nil, numOfRetryAttemps: UInt8? = nil, retrySleepTime: UInt32? = nil) -> (current: UInt16, max: UInt16)? {
-    var values: (UInt16, UInt16)?
     var send: [UInt8] = [command]
     var reply = [UInt8](repeating: 0, count: 11)
-    if Self.performDDCCommunication(service: service, send: &send, reply: &reply, writeSleepTime: writeSleepTime, numOfWriteCycles: numOfWriteCycles, readSleepTime: readSleepTime, numOfRetryAttemps: numOfRetryAttemps, retrySleepTime: retrySleepTime) {
-      let max = UInt16(reply[6]) * 256 + UInt16(reply[7])
-      let current = UInt16(reply[8]) * 256 + UInt16(reply[9])
-      values = (current, max)
-    } else {
-      values = nil
+    guard Self.performDDCCommunication(service: service, send: &send, reply: &reply, writeSleepTime: writeSleepTime, numOfWriteCycles: numOfWriteCycles, readSleepTime: readSleepTime, numOfRetryAttemps: numOfRetryAttemps, retrySleepTime: retrySleepTime) else {
+      return nil
     }
-    return values
+    let max = UInt16(reply[6]) * 256 + UInt16(reply[7])
+    let current = UInt16(reply[8]) * 256 + UInt16(reply[9])
+    return (current, max)
   }
 
   static func write(service: IOAVService?, command: UInt8, value: UInt16, writeSleepTime: UInt32? = nil, numOfWriteCycles: UInt8? = nil, numOfRetryAttemps: UInt8? = nil, retrySleepTime: UInt32? = nil) -> Bool {
@@ -92,13 +89,15 @@ class Arm64DDC: NSObject {
   static func performDDCCommunication(service: IOAVService?, send: inout [UInt8], reply: inout [UInt8], writeSleepTime: UInt32? = nil, numOfWriteCycles: UInt8? = nil, readSleepTime: UInt32? = nil, numOfRetryAttemps: UInt8? = nil, retrySleepTime: UInt32? = nil) -> Bool {
     let dataAddress = ARM64_DDC_DATA_ADDRESS
     var success = false
-    guard service != nil else {
+    guard let service = service else {
       return success
     }
     var packet: [UInt8] = [UInt8(0x80 | (send.count + 1)), UInt8(send.count)] + send + [0] // Note: the last byte is the place of the checksum, see next line!
     packet[packet.count - 1] = self.checksum(chk: send.count == 1 ? ARM64_DDC_7BIT_ADDRESS << 1 : ARM64_DDC_7BIT_ADDRESS << 1 ^ dataAddress, data: &packet, start: 0, end: packet.count - 2)
-    for _ in 1 ... (numOfRetryAttemps ?? 4) + 1 {
-      for _ in 1 ... max((numOfWriteCycles ?? 2) + 0, 1) {
+    // Int arithmetic: a retry count of 255 (the clamp in OtherDisplay.readDDCValues allows
+    // exactly that) would overflow UInt8 on the `+ 1` and trap.
+    for _ in 1 ... Int(numOfRetryAttemps ?? 4) + 1 {
+      for _ in 1 ... max(numOfWriteCycles ?? 2, 1) {
         usleep(writeSleepTime ?? 10000)
         success = IOAVServiceWriteI2C(service, UInt32(ARM64_DDC_7BIT_ADDRESS), UInt32(dataAddress), &packet, UInt32(packet.count)) == 0
       }

MonitorControl/Support/DisplayManager.swift

@@ -101,7 +101,7 @@ class DisplayManager {
   func destroyAllShades() -> Bool {
     var ret = false
     for displayID in self.shades.keys {
-      os_log("Attempting to destory shade for display  %{public}@", type: .info, String(displayID))
+      os_log("Attempting to destroy shade for display  %{public}@", type: .info, String(displayID))
       if self.destroyShade(displayID: displayID) {
         ret = true
       }
@@ -240,23 +240,12 @@ class DisplayManager {
     self.displays.compactMap { $0 as? OtherDisplay }
   }
 
-  func sortDisplays() {
-    // Opsiyonel: sıralamadan önce log al
-    let before = displays.map { $0.name }
-    os_log("Displays before sorting: %{public}@", before)
-
-    // In‑place sıralama
-    displays.sort { lhs, rhs in
-      lhs.name.localizedStandardCompare(rhs.name) == .orderedAscending
-    }
-
-    // Opsiyonel: sıralamadan sonra log al
-    let after = displays.map { $0.name }
-    os_log("Displays after sorting: %{public}@", after)
-  }
-
   /// Sorts the given displays by their friendly name.
   ///
+  /// The result is in descending name order on purpose: callers insert each display's
+  /// menu block at index 0, so the last display processed ends up at the top of the
+  /// menu. Feeding the blocks in descending order renders the menu in ascending order.
+  ///
   /// The list is passed in explicitly. An earlier version sorted `self.displays` and
   /// returned that, which silently discarded whatever filtering the caller had done —
   /// notably the "hide Apple displays from menu" preference.
@@ -272,11 +261,8 @@ class DisplayManager {
     }
   }
 
-
-
-  /// displays dizisini sıralar ve döner
   func getAllDisplays() -> [Display] {
-    return displays
+    self.displays
   }
 
   func getDdcCapableDisplays() -> [OtherDisplay] {
@@ -320,19 +306,10 @@ class DisplayManager {
   }
 
   func addDisplayCounterSuffixes() {
-    var nameDisplays: [String: [Display]] = [:]
-    for display in self.displays {
-      if nameDisplays[display.name] != nil {
-        nameDisplays[display.name]?.append(display)
-      } else {
-        nameDisplays[display.name] = [display]
-      }
-    }
-    for nameDisplayKey in nameDisplays.keys where nameDisplays[nameDisplayKey]?.count ?? 0 > 1 {
-      for i in 0 ... (nameDisplays[nameDisplayKey]?.count ?? 1) - 1 {
-        if let display = nameDisplays[nameDisplayKey]?[i] {
-          display.name = "" + display.name + " (" + String(i + 1) + ")"
-        }
+    let displaysByName = Dictionary(grouping: self.displays, by: { $0.name })
+    for (_, sameNameDisplays) in displaysByName where sameNameDisplays.count > 1 {
+      for (index, display) in sameNameDisplays.enumerated() {
+        display.name = "\(display.name) (\(index + 1))"
       }
     }
   }
@@ -386,7 +363,7 @@ class DisplayManager {
       if (otherDisplay.readPrefAsFloat(for: .brightness) == 0 && !prefs.bool(forKey: PrefKey.disableCombinedBrightness.rawValue)) || (otherDisplay.readPrefAsFloat(for: .brightness) < otherDisplay.combinedBrightnessSwitchingValue() && !prefs.bool(forKey: PrefKey.separateCombinedScale.rawValue) && !prefs.bool(forKey: PrefKey.disableCombinedBrightness.rawValue)) || otherDisplay.isSw() {
         let savedPrefValue = otherDisplay.readPrefAsFloat(key: .SwBrightness)
         if otherDisplay.getSwBrightness() != savedPrefValue {
-          OSDUtils.popEmptyOsd(displayID: otherDisplay.identifier, command: Command.brightness) // This will give the user a hint why is the brightness suddenly changes.
+          OSDUtils.popEmptyOsd(displayID: otherDisplay.identifier, command: Command.brightness) // This gives the user a hint why the brightness suddenly changed.
         }
         otherDisplay.savePref(otherDisplay.getSwBrightness(), key: .SwBrightness)
         os_log("Restoring sw brightness to %{public}@ on other display %{public}@", type: .info, String(savedPrefValue), String(otherDisplay.identifier))
@@ -553,7 +530,7 @@ class DisplayManager {
         return name
       }
     }
-    if let screen = getByDisplayID(displayID: displayID) { // MARK: This, and NSScreen+Extension.swift will not be needed when we drop MacOS 10 support.
+    if let screen = getByDisplayID(displayID: displayID) { // MARK: This, and NSScreen+Extension.swift will not be needed when we drop macOS 10 support.
 
       if #available(macOS 10.15, *) {
         return screen.localizedName

MonitorControl/Support/IntelDDC.swift

@@ -70,7 +70,7 @@ public class IntelDDC {
     return success
   }
 
-  public func read(command: UInt8, tries: UInt = 1, replyTransactionType _: IOOptionBits? = nil, minReplyDelay: UInt64? = nil, errorRecoveryWaitTime: UInt32? = nil, writeSleepTime: UInt32 = 10000) -> (UInt16, UInt16)? {
+  public func read(command: UInt8, tries: UInt = 1, minReplyDelay: UInt64? = nil, errorRecoveryWaitTime: UInt32? = nil, writeSleepTime: UInt32 = 10000) -> (UInt16, UInt16)? {
     var data: [UInt8] = Array(repeating: 0, count: 5)
     var replyData: [UInt8] = Array(repeating: 0, count: 11)
 
@@ -82,7 +82,8 @@ public class IntelDDC {
     let dataCount = UInt32(data.count)
     let replyDataCount = UInt32(replyData.count)
 
-    for i in 1 ... tries {
+    // A try count of zero would make `1 ... 0` an invalid range and trap, so treat it as a single try.
+    for i in 1 ... max(tries, 1) {
       usleep(writeSleepTime)
       usleep(errorRecoveryWaitTime ?? 0)
       let sent = data.withUnsafeMutableBytes { sendBuffer -> Bool in

MonitorControl/Support/MediaKeyTapManager.swift

@@ -156,10 +156,7 @@ class MediaKeyTapManager: MediaKeyTapDelegate {
       keys.append(contentsOf: [.mute, .volumeUp, .volumeDown])
     }
     // Remove brightness keys if no external displays are connected, but only if brightness fine control is not active
-    var hasExternalDisplay = false
-    for display in DisplayManager.shared.getAllDisplays() where !display.isBuiltIn() {
-      hasExternalDisplay = true
-    }
+    let hasExternalDisplay = DisplayManager.shared.getAllDisplays().contains { !$0.isBuiltIn() }
     // Disengage brightness keys on sleep so MacBook native screen can be controlled meanwhile
     let isTransient = app.sleepID != 0 || app.reconfigureID != 0
     let disengageBrightness = !hasExternalDisplay || isTransient

MonitorControl/Support/MenuHandler.swift

@@ -9,13 +9,7 @@ class MenuHandler: NSMenu, NSMenuDelegate {
   var lastMenuRelevantDisplayId: CGDirectDisplayID = 0
 
   func clearMenu() {
-    var items: [NSMenuItem] = []
-    for i in 0 ..< self.items.count {
-      items.append(self.items[i])
-    }
-    for item in items {
-      self.removeItem(item)
-    }
+    self.removeAllItems()
     self.combinedSliderHandler.removeAll()
   }
 
@@ -41,7 +35,7 @@ class MenuHandler: NSMenu, NSMenuDelegate {
       let externalDisplays = DisplayManager.shared.displays.filter {
         CGDisplayIsBuiltin($0.identifier) == 0
       }
-      if externalDisplays.count > 0 {
+      if !externalDisplays.isEmpty {
         showIcon = true
       }
     }
@@ -72,7 +66,7 @@ class MenuHandler: NSMenu, NSMenuDelegate {
       }
     }
     if numOfDisplays != 0 {
-      let asSubMenu: Bool = (displays.count > 3 && !relevant && !combine && app.macOS10()) ? true : false
+      let asSubMenu = displays.count > 3 && !relevant && !combine && app.macOS10()
       var iterator = 0
       for display in displays where (!relevant || DisplayManager.resolveEffectiveDisplayID(display.identifier) == relevantDisplayID) && !display.isDummy {
         iterator += 1
@@ -215,7 +209,7 @@ class MenuHandler: NSMenu, NSMenuDelegate {
   }
 
   func updateDisplayMenu(display: Display, asSubMenu: Bool, numOfDisplays: Int) {
-    os_log("Addig menu items for display %{public}@", type: .info, "\(display.identifier)")
+    os_log("Adding menu items for display %{public}@", type: .info, "\(display.identifier)")
     let monitorSubMenu: NSMenu = asSubMenu ? NSMenu() : self
     var addedSliderHandlers: [SliderHandler] = []
     display.sliderHandler[.audioSpeakerVolume] = nil