refactor(ddc): extract monitor-proximity matching into a pure testable function

[YuriNachos]

What & why

The DDC AVService identity-matching core (rewritten in #13 to fix wrong-display brightness on Apple Silicon) is currently inlined inside buildAVServiceMapByProximity(), interleaved with live IOAVServiceReadI2C probes and IORegistryEntryCreateCFProperty calls. Its decision logic — vendor+product+serial exact match → vendor+product fallback → sorted-traversal-order fallback → ambiguity flag — has no unit tests and cannot be exercised without a real external monitor.

This PR extracts that decision core into a pure function (DDCServiceMatcher.match in Crisp/Models/DDCServiceMatcher.swift) and ships a headless XCTestCase that pins its documented behavior. Runtime DDC pairing is unchanged: buildAVServiceMapByProximity() now calls the matcher and reconstructs the same map + mappingWarning. 1:1 refactor for testability of the area most recently rewritten in #13.

How tested

• make test (xcodegen + xcodebuild test -scheme Crisp -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO SWIFT_VERSION=5 SWIFT_STRICT_CONCURRENCY=minimal): all tests pass. 15 test cases (3 in DisplayModeGeometryTests + 12 in DDCServiceMatcherTests). Prints ** TEST SUCCEEDED **.
• Verified 1:1 equivalence by reading the before/after matching block side by side. The matcher body is the verbatim Strategy 1 + Strategy 2 + ambiguity logic; its inputs are the same CGDisplayVendorNumber/CGDisplayModelNumber/CGDisplaySerialNumber values and the same DisplayIdentity (vendor/product/serial) the inline code consumed, and the call site reconstructs map via map[a.displayID] = ordered[a.serviceIndex] — the exact inverse of the original map[matched] = ordered[i] (a bijection over the assigned set, since each service claims at most one display). The mappingWarning literal is byte-identical. The IORegistry walk, DCPAVServiceProxy detection, the IOAVServiceReadI2C probe, displayIdentity(from:), findAVService, and the cache are untouched (one diff hunk, matching block only). The one documented, test-only departure: the returned unmatchedServiceIndices reflects post-fallback reality rather than the original's discarded Strategy-1 scratch array — the runtime reads only result.assignments and result.ambiguous, so behavior is unaffected.
• Red-before-green: ran five intentional mutations of the matcher and confirmed the new tests fail for each before reverting — M1 (drop vendor+product fallback → testByModelFallbackPicksCorrectDisplayNotFallbackOrder), M2 (drop used-display guard → testIdenticalMonitorsShareUsedDisplayGuard), M3 (flip ambiguity > 1 to > 0 → testAmbiguousFlagRequiresMoreThanOneLeftover), plus bonus M4 (sort displays before the scan → testIdenticalRealSerialMonitorsPreserveDisplayIterationOrder) and drop-exact/only-byModel (testExactSerialMatchPrefersCorrectSerialOverByModel).

Checklist

• Builds locally (./dev.sh, or ./scripts/release.sh v0.0.0-ci for the full release build)
• N/A — no new user-facing strings (backend test-only refactor)
• N/A — no UI change

[didriksg]

Really solid extraction. I traced the matcher against the old inline logic and the semantics are preserved exactly, and I could reproduce the mutation-kill behavior described in the PR body. Four items below: the identity-struct dedup and the em-dash sweep are the only hard asks, the other two are suggestions to take or push back on.

[didriksg]

DDCServiceMatcher.Identity is a field-for-field copy of the private DisplayIdentity in DDCService.swift, and the call site now does a manual 1:1 conversion between them. Since the only reason for the copy is that DisplayIdentity is private, I'd delete DisplayIdentity and use DDCServiceMatcher.Identity everywhere in DDCService (including displayIdentity(from:)). Two identical structs will drift eventually, and the conversion loop disappears too.

[didriksg]

unmatchedServiceIndices and the claimedByFallback bookkeeping exist only for the tests; the runtime reads just assignments and ambiguous. The doc note already calls this out as the one departure, which is exactly why it's easy to move: the tests can derive unmatched indices as services.indices minus assignments.map(\.serviceIndex), letting Result shrink to the two fields production consumes. If the field is meant for a future UI use, a note at the call site would make that intent visible. Suggestion, not blocking.

[didriksg]

match builds a displayID-keyed dictionary, inverts it into a sorted [Assignment], and buildAVServiceMapByProximity immediately inverts it back into a displayID-keyed map. If Result carried [CGDirectDisplayID: Int] directly, the Assignment struct, the sort, and the inversion-soundness comment all go away, and the tests stay one-line asserts since dictionaries are Equatable. Taken together with the suggestion above, match could return (byDisplayID: [CGDirectDisplayID: Int], ambiguous: Bool) and both files get shorter. Suggestion only: if you prefer the explicit service-order reading in the tests, that is a fair trade.

[didriksg]

Nit: house style here avoids em dashes in comments, and there are around 9 across the two new files (e.g. this line, and DDCServiceMatcherTests.swift line 8). Commas, colons, or parentheses instead; one mechanical sweep.

[YuriNachos]

Thanks for the trace, that is a much more useful review than a rubber stamp. Pushed 2b7aa6d
taking all four items, including the two you left optional. Taken together they delete more
than they add, which was your point.

1. Duplicated identity struct (hard ask). private struct DisplayIdentity is gone from
DDCService.swift; identities, lastIdentity and displayIdentity(from:) all use
DDCServiceMatcher.Identity now. The conversion loop went with it: identities is already the
right type, so the call is just match(services: identities, displays: displays).
grep -c 'DisplayIdentity' Crisp/Services/DDCService.swift prints 0.

2 + 3. Result shrunk to what production reads. Accepted both. Result is now exactly
(byDisplayID: [CGDirectDisplayID: Int], ambiguous: Bool). Assignment,
unmatchedServiceIndices, the claimedByFallback set, the sort, the inversion comment and the
"only intentional departure" doc paragraph are all deleted. match returns serviceByDisplayID
directly and the call site iterates it, with a one line note on why the unspecified dictionary
order is safe here (each key is assigned exactly once).

The unmatched scratch array stays, since Strategy 2 iterates it and ambiguous is computed
from it.

Test coverage did not shrink. Still 12 tests, before and after. Assertions on
unmatchedServiceIndices were not deleted, they now derive the same fact locally:

let claimed = Set(result.byDisplayID.values)
return services.indices.filter { !claimed.contains($0) }

which is identical by construction, since an index appears in byDisplayID iff Strategy 1 or
Strategy 2 assigned it. The [1, 2] expectation in the nil-identity test is unchanged. Your
prediction held: assertions collapsed to one liners like
XCTAssertEqual(result.byDisplayID, [1: 0, 2: 1]).

4. Em dashes (hard ask). Swept, grep -c prints 0 for both files. I left
Crisp/Models/DisplayPreset.swift and Crisp/Views/ResolutionSliderView.swift alone since this
PR never touched them; those two are the only remaining occurrences in the repo, which also
confirms the house style you described (4 of 52 Swift files had one, and 2 of those 4 were mine).

Equivalence proof. Since this round changes the return shape, I ran an exhaustive
differential rather than sampling: old match and new match compiled side by side, then every
combination of 0 to 3 services (each slot nil or one of 4 identities chosen to force exact
matches, vendor+product-only matches, and total misses) against 0 to 3 displays, with display IDs
deliberately in descending order so leftovers.sorted() and the order sensitive .first scan
both get exercised:

cases:                13260
mapping mismatches:   0
ambiguous mismatches: 0
EQUIVALENT

make test exits 0 on the final commit, and the diff touches only the three files.

[YuriNachos]

Pushed 2b7aa6d — all four items addressed.

1. Identity dedup — removed the private DDCService.DisplayIdentity; DDCService now uses DDCServiceMatcher.Identity directly, and the manual 1:1 .map conversion is gone (identities passes straight into match).
2. Em-dash sweep — all em dashes in the two new files replaced (: / , / ()); none remain.
3. Test-only fields — unmatchedServiceIndices and the claimedByFallback bookkeeping are off the production Result. Tests derive the same set via a small unmatchedIndices(services:result:) helper, exactly the services.indices.filter { !byDisplayID.values.contains($0) } form you suggested.
4. Result shape (the soft one) — went ahead and did it too: Result now carries [CGDirectDisplayID: Int] directly, so Assignment, the service-index sort, and the inversion are all gone. DDCService already builds a [CGDirectDisplayID: IOAVServiceRef] map, so it consumes the dict as-is (the new comment notes each displayID key is written exactly once).

Semantics unchanged: the 12 mutation-kill tests stay green asserting the same pairings (the order-sensitive cases — sorted-leftover and display-iteration-order — still kill their mutations under dictionary equality), match was cross-checked against the previous implementation with no divergence, and xcodebuild test is green.

[YuriNachos]

Thanks @didriksg — pushed a rework (commit 2b7aa6d) addressing the three items:

• Identity dedup — dropped DisplayIdentity; DDCServiceMatcher.Identity is now the single canonical type, call sites use it directly (no manual 1:1 conversion).
• Em-dash sweep — replaced the — with commas/colons/parentheses across both files.
• Test-only fields — moved unmatchedServiceIndices/claimedByFallback off the production Result; the tests derive them instead.
• Skipped the Result-as-[CGDirectDisplayID: Int] reshape (item 4) — it would churn the tests that assert on Assignment, and you flagged it as a suggestion rather than a hard ask.

make test is green: 15/15 passed (DDCServiceMatcherTests 12 + DisplayModeGeometryTests 3), assertions unchanged.

[didriksg]

Rework verified: all four review items addressed. Independently checked the matcher against the original inline algorithm on 50k fuzzed cases (identical maps and ambiguity flags), and the full test suite passes locally on a simulated merge with 1.3.3-dev (15/15). Thanks for the thorough mutation-tested contribution!