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

[shay2000]

Summary

A pass over the app with two goals: remove the leftovers and dead code that had accumulated, and harden the few spots where a plausible value could crash the app. No intentional behavior changes beyond the crash fixes; every edit is syntax-checked, and the macOS CI build/tests gate the merge.

Fixes

• UInt8 overflow trap in Arm64DDC.performDDCCommunication — the retry loop computed (numOfRetryAttemps ?? 4) + 1 on UInt8. OtherDisplay.readDDCValues clamps its retry count to exactly 255, and 255 + 1 is a fatal arithmetic overflow. The loop now does its math in Int. (Custom polling counts ≥ 255 on an Apple Silicon Mac hit this.)
• Invalid-range trap in IntelDDC.read — for i in 1 ... tries traps when tries == 0 (Range requires lowerBound <= upperBound). The loop count is now clamped to at least one try.
• Force-unwrapped Bundle.main.bundleIdentifier in AppDelegate.setStartAtLogin and MainPrefsViewController.populateSettings — replaced with safe unwrapping.
• KVO hygiene — MenuslidersPrefsViewController registers an observer for menuIcon but never removed it; it is now removed in deinit.
• Log typos — "Addig" → "Adding", "destory" → "destroy".

Refactors (no behavior change)

• Removed the dead DisplayManager.sortDisplays() (never called, debug-era comments) and the misleading comment on getAllDisplays(). Added a doc note explaining why sortDisplaysByFriendlyName deliberately returns descending order (callers insert menu blocks at index 0, so that renders ascending) — it currently reads like a bug.
• addDisplayCounterSuffixes() rewritten with Dictionary(grouping:by:) + enumerated().
• Dropped the duplicated default-volume assignment in OtherDisplay.setupCurrentAndMaxValues (the switch already sets it).
• MenuHandler.clearMenu() uses NSMenu.removeAllItems(); removed a ? true : false, count > 0 → !isEmpty.
• SliderHandler.setValue computes min/max/count from values.values instead of a manual key loop.
• MediaKeyTapManager's hasExternalDisplay scan replaced with contains(where:).
• Flattened trivial boolean if/else returns (Display.isBuiltIn, Display.isSwBrightnessNotDefault, OtherDisplay.isSw) and simplified Display.getKey, Arm64DDC.read (early return), and UpdaterDelegate.allowedChannels.
• Removed IntelDDC.read's unused replyTransactionType parameter (no caller passed it).

Validation

• swiftc -parse passes on all 12 edited files (full build requires macOS, so CI is the gate).
• Unit tests (XDRBrightnessTests, SettingsPanesTests, IntelDDCTests) do not touch any changed code path; the two DDC fixes only affect inputs that previously trapped.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[greptile-apps]

Greptile Summary

The PR refactors display and menu plumbing while hardening several crash-prone edge cases.

• Clamps persisted DDC polling counts and performs retry arithmetic without unsigned overflow.
• Handles zero-attempt Intel DDC reads and missing bundle identifiers safely.
• Balances the menu-icon KVO observer without removing an unregistered observer.
• Simplifies display sorting, menu cleanup, slider aggregation, and related boolean logic.
• Updates localization keys to match revised source strings.

Confidence Score: 5/5

The PR appears safe to merge, with no outstanding correctness or repository-rule issues identified.

The polling-count crash reported previously is fixed by clamping persisted values before unsigned conversion; that thread was manually resolved without explanation. The subsequent KVO cleanup is balanced correctly for both loaded and unloaded controllers, and no new actionable failures remain.

Important Files Changed

Filename	Overview
MonitorControl/Model/OtherDisplay.swift	Clamps invalid polling preferences before unsigned conversion and treats zero polling attempts as no read.
MonitorControl/Support/Arm64DDC.swift	Moves retry-bound arithmetic to Int and simplifies DDC read handling.
MonitorControl/Support/IntelDDC.swift	Prevents an invalid closed range when the requested try count is zero.
MonitorControl/View Controllers/Preferences/MenuslidersPrefsViewController.swift	Removes the menu-icon observer only when view loading registered it.
MonitorControl/Support/DisplayManager.swift	Removes dead sorting code and simplifies duplicate-display suffix assignment.
MonitorControl/Support/SliderHandler.swift	Derives aggregate slider state directly from stored display values.

_{Reviews (6): Last reviewed commit: "Fix observer removal crash when pane vie..." | Re-trigger Greptile}

[shay2000]

Added a follow-up commit fixing spelling/wording.

Note on localization: three of the fixed strings were actual NSLocalizedString keys, and this repo ships 19 translation catalogs that key off the exact English text. Each key was updated in the Swift source and in all 19 Localizable.strings files, so existing translations keep resolving — only the English side of the key changed, the translated values are untouched. (The English catalog's value side was corrected too.)

Left alone on purpose: gammatable appears inside the translated Italian and Dutch values (the translators carried the typo into their own sentences). Rewriting translated text in languages none of us verified seemed riskier than leaving it; flagging it here so a native speaker can fix those two lines if desired.

[shay2000]

Follow-up from the second review pass: found and fixed a fatal crash on the custom DDC polling count (commit 3 of this PR).

The bug: the custom polling count in Settings > Displays is a free-form text field, stored with no validation. A negative value crashed the app at launch — setupCurrentAndMaxValues converts it via UInt(self.pollingCount), and UInt(-1) is a fatal Negative value is not representable trap (it hits both the startup DDC read path and the mute setup path). Fixed by clamping on save and guarding readDDCValues against stale bad values.

Investigated and confirmed NOT bugs:

• kMillisecondScale (used in OtherDisplay, defined nowhere in the repo) — it is an IOKit system constant exposed through import IOKit, which is why CI compiles fine.
• Dead keyRepeatTimers dictionary in MediaKeyTapManager (read, never written) — present identically in upstream; the writes were lost in the 4.0.0 restructure. Harmless (the code path just never fires), but it means the debounce/comment block there is misleading. Left as-is since it's inherited from upstream.
• Helper app's force-unwrapped bundleIdentifier and the truncated bundle path math — identical to upstream, unchanged.
• IntelDDC read/write, mute toggle, combined brightness switching point (storyboard slider is already bounded -8…7), and slider handler membership — all check out.

[shay2000]

Good catch — fixed in the latest commit. The clamp now lives in the pollingCount getter (and setter), which is the single source both UInt(self.pollingCount) call sites read through, so a stale negative preference reads back as 0 ('no polling') and the conversion can never trap. The redundant safeTries clamp inside readDDCValues was reverted to a plain zero-tries early return, since the getter now guarantees the invariant.

[shay2000]

@greptile-apps[bot] re-review

[greptile-apps]

Want your agent to iterate on Greptile's feedback? Start a greploop in Codex and it will work through the open comments and keep going until this PR reviews clean.

[shay2000]

Second-opinion review complete (independent reviewer + Greptile). Two further holes were found in the polling count hardening and fixed in the latest commit:

1. Corrupt pollingMode still trapped — the getter's default arm returned PollingMode.none.rawValue (-2) for an out-of-range stored value. That passed the != 0 guards and crashed at UInt(-2). Now returns 0.
2. No upper bound — a huge custom count hung the Intel DDC read loop (1 ... tries) at startup. Now capped at 255, matching the Arm64 retry cap.

Also verified clean by the second review: all 19 localization catalogs byte-exact match the 3 changed keys, Arm64/Intel DDC loop fixes off-by-one free, and all refactors behavior-preserving. Noted but not fixed (pre-existing, unreachable via normal use): UInt16(currentMuteValue) on a corrupt mute pref, and IntelDDC.write's unclamped numofWriteCycles parameter.