feat(display): per-output ICC color profiles and color temperature - #3388
Conversation
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| log.Info("gamma: output returned, re-establishing controls") | ||
| m.controlsInitialized = true | ||
| }) | ||
| if m.controlsInitialized { |
There was a problem hiding this comment.
This drops the recovery path for monitor sleep/disconnect. When the last output goes away, the remove handler sets m.controlsInitialized = false (line 270). With the old code the returning output ran addOutputControl and then re-marked controlsInitialized = true so the following gamma_size drove the reapply. Now the if m.controlsInitialized guard is false in exactly that case, so nothing is created and gamma/ICC stays dead until the user manually toggles night light.
Keep the previous behaviour: always post addOutputControl, and set controlsInitialized = true when it was cleared.
| case !m.controlsInitialized: | ||
| log.Debugf("applyGamma skipped: controls not initialized") | ||
| return | ||
| case m.lastAppliedTemp == temp && m.lastAppliedGamma == gamma: |
There was a problem hiding this comment.
This manager-level dedupe doesn't include contrast, and it short-circuits before the per-output loop, so two existing paths break:
SetAdjustments(gamma, contrast)with only contrast changed →syncControls→triggerUpdate→applyCurrentTemp→applyGamma(temp)with the same temp and gamma → early return. The contrast change is never written to any output.handleResume(line 1119) resets everyout.lastTemp = 0to force a resend after suspend (Gamma settings not applied after wake from sleep [with patch] #1235), then callsapplyCurrentTemp. Temp and gamma are unchanged across suspend, so this guard returns before the outputs are touched and the forced resend never happens.
Either include contrast in the comparison and have those paths reset m.lastAppliedTemp, or drop this guard and rely on the existing per-output rampCurrent dedupe, which already covers temp/gamma/contrast.
| } else { | ||
| log.Debugf("gamma_size: output %d not found in m.outputs", outputID) | ||
| } | ||
| m.lastAppliedTemp = 0 |
There was a problem hiding this comment.
Dropping out.lastTemp = 0 here (and the matching one in the send-failure path below, line ~936) breaks recovery after a gamma control failure. recreateOutputControl reuses the same outputState, so lastTemp/lastGamma/lastContrast survive the failure. When the new control's gamma_size arrives, failed and rampSize are reset but rampCurrent(temp, gamma, contrast) still returns true, so the output is skipped and the recreated control never receives a ramp — the display stays uncorrected.
m.lastAppliedTemp = 0 only bypasses the manager-level guard; it doesn't clear the per-output state. Keep out.lastTemp = 0 in both places.
| sunrise := time.Date(now.Year(), now.Month(), now.Day(), | ||
| config.ManualSunrise.Hour(), config.ManualSunrise.Minute(), config.ManualSunrise.Second(), 0, now.Location()) | ||
| sunset := time.Date(now.Year(), now.Month(), now.Day(), | ||
| config.ManualSunset.Hour(), config.ManualSunset.Minute(), config.ManualSunset.Second(), 0, now.Location()) |
There was a problem hiding this comment.
This PR reverts three unrelated scheduler fixes that exist on the base commit:
- The
if !sunset.After(sunrise) { sunset = sunset.Add(24 * time.Hour) }adjustment right below this line is gone, so a manual schedule whose night start is past midnight (e.g. sunrise 07:00, sunset 00:30) now produces aSunsetbeforeSunriseon the same day. activeCycle/shiftTimeswere deleted along with their call sites ingetSunPositionNormal,getDeadlineNormalandupdateStateFromSchedule, so early-morning hours no longer map back to yesterday's cycle — the temperature andisDayare wrong between midnight and dawn.- In the location-missing branch below (line 553),
m.schedule = sunSchedule{}was removed, so stale times from a previous config keep driving applies.
None of these are related to ICC; they should be restored.
| // non-ICC outputs get identity (no color shift). | ||
| if !enabled { | ||
| m.applyGamma(neutralTemp) | ||
| m.applyGamma(high) |
There was a problem hiding this comment.
high is the user-configurable HighTemp (SetTemperature, valid down to 1000K), not a neutral point — the removed neutralTemp was a fixed 6500. With night light disabled and HighTemp set to e.g. 5000, every output now gets a permanent 5000K ramp written to it instead of neutral. Use a fixed 6500 here (or DefaultConfig().HighTemp).
Related: syncControls (line 1292) still calls destroyControls() whenever needsControls() is false, so toggling night light off tears the controls down and drops the ICC ramps anyway — which defeats the "ICC applies while night light is off" goal of this PR. needsControls() probably needs to account for configured ICC profiles / per-output temps.
| if err != nil { | ||
| return "", err | ||
| } | ||
| return filepath.Join(configDir, "niri", "dms", "wayland.json"), nil |
There was a problem hiding this comment.
niri is hardcoded, but this dir is compositor-specific in the rest of the codebase — commands_setup.go:275 picks niri/dms, hypr/dms or mango/dms based on the running compositor. On Hyprland/mango/sway this writes ICC and per-output temp config into a niri directory that the rest of DMS never looks at. commands_icc.go:70 has the same hardcoding for the ICC profile dir.
Also worth using utils.XDGConfigHome() here rather than os.UserConfigDir(), so it matches the path resolution used everywhere else in core.
| strOff := binary.BigEndian.Uint32(data[recordStart+8 : recordStart+12]) | ||
| // String offset is absolute from profile start | ||
| strStart := entry.offset + strOff | ||
| strEnd := strStart + strLen |
There was a problem hiding this comment.
Both desc branches can panic on a malformed profile, which takes down the dms daemon (ApplyICC parses a user-picked file in-process, and dms icc list parses every file in the config dir).
strOffandstrLencome straight from the file.strStartis never bounds-checked, andstrEnd := strStart + strLenis uncheckeduint32arithmetic. IfstrStart > len(data)the clamp setsstrEnd = len(data), leavingstrStart > strEnd→data[strStart:strEnd]panics. Same for thedescpath at line 266-267 ifstrLenis large enough to wrapstrEndbelowstrStart.recordStart+12(line 297-299) is only guarded byentry.size < 16, so a tag withsize == 16at the end of the buffer reads past the slice.
Validate strStart <= strEnd <= len(data) (and recordStart+12 <= len(data)) before slicing, and return an error instead.
|
|
||
| onSliderValueChanged: function(newValue) { | ||
| colorTempRow.editing = true | ||
| tempLabel.text = newValue + "K" |
There was a problem hiding this comment.
Assigning to tempLabel.text imperatively destroys its binding permanently. After the first drag the label is a static string: colorTempRow.editing = false no longer restores it, and it stops tracking ICCService.outputTemps[outputName] for any later change (including one made from the CLI or another surface).
The binding on line 537 already handles both cases via colorTempRow.editing/tempSlider.value, so this line just needs to go.
| tempLabel.text = newValue + "K" | |
| colorTempRow.editing = true |
| colorTempRow.editing = true | ||
| tempLabel.text = newValue + "K" |
There was a problem hiding this comment.
Correcting the suggestion in my comment above — it should replace both lines, not just line 571 (otherwise editing = true ends up duplicated):
| colorTempRow.editing = true | |
| tempLabel.text = newValue + "K" | |
| colorTempRow.editing = true |
| } | ||
| } | ||
|
|
||
| DisplayConfirmationModal { |
There was a problem hiding this comment.
This PR deletes the identifyConfigured property, the MonitorIdentifyOverlay Loader that followed this block, and the id: monitorCanvas it depends on (line 560). That removes the working "identify monitors" overlay, which is unrelated to ICC — looks like an accidental revert. Same for the Theme.spacingXXS → 2 changes here and in OutputCard.qml, which swap Theme tokens back for hardcoded values.
Claude reviewSolid feature work, but it reverts several unrelated fixes in the gamma stack and the display settings UI, and the new dedupe breaks two existing paths — those need to be sorted before merge.
Checked: full diff against the merge base, gamma manager lifecycle/dedupe paths, ICC parser bounds handling, IPC handlers, and the new QML service and Display Config rows. Model: claude-opus-5. |
Rebuild of AvengeMedia#3388 on current master: the previous branch was cut from an older base and reverted unrelated fixes in the gamma stack and the display settings UI. ICC profiles: - core/internal/icc: ICC v2/v4 parser (TRC, LUT8/16 and parametric curves, vcgt, description/version/color space) plus gamma ramp generation, with synthetic coverage and optional vendor-profile tests (ICC_TEST_DIR). - wayland manager: load iccProfiles from the compositor's DMS config dir, build the ramp from the profile when one is set (falling back to the temperature ramp when it is missing or unparsable), and re-apply on output hotplug and after resume. - ICC ramps apply while the night light schedule is disabled; outputs without a profile keep an identity ramp. - desc/mluc offsets are validated before slicing, so a malformed profile returns an error instead of panicking the daemon. Per-output temperature: - outputTemps in the same config; SetOutputTemp accepts 1000-10000K and the Display Config slider exposes 3000-10000K, which matters for displays calibrated at a higher white point (e.g. a 7000K profile). Gamma control lifecycle: - needsControls() now keeps the controls alive for configured ICC profiles and per-output temperatures, so toggling the night light off no longer destroys the controls and drops the ramps. - Re-apply paths clear the per-output dedup state instead of adding a manager-level guard, which would have suppressed contrast-only writes and the post-resume forced resend (AvengeMedia#1235), and left a recreated control without a ramp. - Monitor sleep/disconnect still re-establishes controls when an output comes back. IPC / CLI / UI: - wayland.icc.{getStatus,apply,remove,listOutputs,setTemp,getTemps} - dms icc list | info <file> | apply <output> <file> | remove <output> | status - Display Config output card gains a Color Profile row (browse/apply/remove with description, version, color space and active state) and a per-output color temperature slider, backed by Services/ICCService.qml. Config path: - DMSConfigDir() follows the compositor layout (niri/dms, hypr/dms, mango/dms) via the shared compositor detection and utils.XDGConfigHome(), replacing the hardcoded niri path in both the manager and `dms icc`. Tests: TZ=UTC go test ./... (60 packages ok), including red/green coverage for the icc parser bounds checks and needsControls().
d45ea29 to
1c5cae1
Compare
|
Rebased onto current master ( Reverted upstream fixes (restored)
New-code fixes
One small addition beyond the review: Verification: Diff vs master: 13 files, +2406/−13. The 13 removed lines are |
The per-output temperature override was reachable from the Display Config slider and from `wayland.icc.setTemp` over IPC, but not from the CLI. - `dms icc set-temp <output> <kelvin>` (alias `setTemp`) sets a per-output override, 1000-10000K, and 0 clears it so the output follows the night light schedule again. Range checking matches the daemon so an invalid value fails in the CLI instead of over IPC. - `dms icc status` gains a Temp column (the override in K, or `schedule`), so an override can be verified without opening the settings UI. Verified against a running daemon: `dms icc status` reports 7000K for the three overridden outputs and `schedule` for the remaining one.
…e white point The per-output temperature was documented as the white point a profile was produced at, but an output with a profile ignored it entirely: the ramp was generated from the profile and the temperature only applied to outputs without one (or as the fallback when a profile failed to parse). A display calibrated at 7000K therefore got the profile ramp, and enabling the night light had no effect on it at all. - `ProfileRampWithTemp` composes the profile ramp with the ratio between the target temperature ramp and the reference ramp, so the temperature is the white point the profile describes and the night light shifts relative to it. - `applyGamma` passes the night light temperature as the target, and `noTempTarget` when the schedule is disabled, which leaves profiled outputs at their reference white point and drives plain outputs with the neutral ramp. - The re-apply sentinel stays 0, so it cannot collide with an applied temperature. Covered by TestProfileRampWithTemp (reference == target is a no-op, warmer and cooler targets move the expected channels). Also: `dms icc set-temp` help text describes the reference-white-point semantics.
Restoring the upstream "output returned, re-establishing controls" branch made the registry handler establish the gamma controls before the startup post runs, and that post returned early on `m.controlsInitialized`, so the configured ICC profiles and per-output temperatures were never attached: every output reported "(none)" and the night light ran with defaults until the user re-applied a profile by hand. - `initializeControlsAndICC` (the startup post) loads the configuration first and only creates the controls when they are missing. - The loading is split into `loadConfiguredICC` / `applyConfiguredICCForOutput` so it can also run per output. - A hotplugged output now gets its configured profile and temperature as soon as its name is known (name handler) or when its control is created, which also makes the "re-applies on hotplug" claim in the description true. - After attaching, the output's dedup state is cleared so the ramp is written. Covered by TestManager_LoadConfiguredICCWhenControlsAlreadyExist and TestManager_AttachConfiguredICCForNamedOutput.
The per-output value was treated as the white point a profile was produced at, which meant a display with a profile never changed when the user set a temperature: the profile ramp was applied as measured and the value only mattered when a night light target existed. Per-display temperatures were therefore unusable on profiled outputs while the night light was disabled, which is the normal state for users who want each monitor at its own value. - `effectiveTempTarget` decides per output: a per-output override wins over the night light schedule, the schedule applies when there is no override, and `noTempTarget` means neither. `0` is "no override", not 0K. - `applyGamma` composes the override on top of the profile ramp, treating the profile as measured at 6500K (D65), and drives outputs without a profile with the same target so both paths agree. - `dms icc set-temp` help text and the Display Config slider description go back to describing an independent per-output temperature. Covered by TestEffectiveTempTarget (override wins, schedule fallback, none) and the updated TestProfileRampWithTemp (7000K cools, 5000K warms relative to the 6500K reference).
|
Post-review updates now on the branch (
Tests added/updated:
|
The profile row only showed the description, so there was no way to tell which profile a display actually has applied (whitepoint, curve type, file provenance) without leaving the settings page. - `ICCStatus` carries the descriptive metadata: class, tone-curve kind (plus gamma or table size), vcgt channels/entries, white point chromaticity with a derived CCT and standard illuminant name, file size and mtime. - `internal/icc` gains `WhitePointXY`, `WhitePointCCT` (McCamy), `WhitePointName` (D50/D65) and `TRCKind`, all covered by tests. - Display Config's Color Profile row gets an info button that opens `ICCProfileInfoModal` with those fields and the profile path, alongside the existing browse/remove actions. - `dms icc info` reports the white point as name/xy/CCT instead of raw XYZ.
|
/claude review |
| } | ||
|
|
||
| tagCount := binary.BigEndian.Uint32(data[128:132]) | ||
| tagTableEnd := uint32(132) + tagCount*12 |
There was a problem hiding this comment.
tagCount comes straight from the file and tagCount*12 is unchecked uint32 arithmetic, so the bounds check can be bypassed by wrapping. With tagCount == 0x15555556, tagCount*12 wraps to 8, tagTableEnd becomes 140, and the check passes for any profile ≥ 140 bytes. Line 138 then does make([]tagEntry, 357913942) (~4 GB) and the loop below panics with slice bounds out of range on the first entry.
That matters because applyConfiguredICCForOutput → icc.ParseFile runs on the wayland actor goroutine (m.post from loadConfiguredICC/attachConfiguredICC), and waylandActor has no recover() — unlike the IPC path, which is covered by routeRequestRecovered. A malformed profile persisted in wayland.json therefore takes the whole dms daemon down at startup.
Compute the tag table end in int (or check tagCount against (len(data)-132)/12) before allocating.
| tagTableEnd := uint32(132) + tagCount*12 | |
| if tagCount > uint32((len(data)-132)/12) { | |
| return nil, fmt.Errorf("icc: profile data too short for %d tag entries", tagCount) | |
| } | |
| tagTableEnd := 132 + int(tagCount)*12 | |
| if len(data) < tagTableEnd { |
| tagTableEnd := uint32(132) + tagCount*12 | ||
| if uint32(len(data)) < tagTableEnd { |
There was a problem hiding this comment.
Correcting the suggestion in my comment above — it has to replace lines 132-133, otherwise the original if is left behind:
| tagTableEnd := uint32(132) + tagCount*12 | |
| if uint32(len(data)) < tagTableEnd { | |
| if tagCount > uint32((len(data)-132)/12) { | |
| return nil, fmt.Errorf("icc: profile data too short for %d tag entries", tagCount) | |
| } | |
| tagTableEnd := 132 + int(tagCount)*12 | |
| if len(data) < tagTableEnd { |
| id: tempSlider | ||
| width: parent.width | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| minimum: 3000 | ||
| maximum: 10000 | ||
| step: 100 | ||
| value: colorTempRow.currentTemp === 0 ? 7000 : colorTempRow.currentTemp | ||
| showValue: true | ||
| unit: "K" | ||
|
|
||
| onSliderValueChanged: function (newValue) { |
There was a problem hiding this comment.
DankSlider has wheelEnabled: true by default, and its wheel handler assigns value and emits sliderValueChanged without ever emitting sliderDragFinished. So scrolling over this slider:
- never calls
ICCService.setOutputTemp, so the temperature change is silently dropped; - leaves
colorTempRow.editing = truepermanently, freezingtempLabelon the slider value; - imperatively overwrites
tempSlider.value, destroying thecolorTempRow.currentTempbinding so the slider stops trackingICCService.outputTempsafterwards.
Every other drag-finished-only slider in the settings UI (SettingsSliderRow.qml:149, SettingsSliderCard.qml:135, SystemMonitorVariantCard.qml:379) sets wheelEnabled: false for exactly this reason.
| id: tempSlider | |
| width: parent.width | |
| anchors.verticalCenter: parent.verticalCenter | |
| minimum: 3000 | |
| maximum: 10000 | |
| step: 100 | |
| value: colorTempRow.currentTemp === 0 ? 7000 : colorTempRow.currentTemp | |
| showValue: true | |
| unit: "K" | |
| onSliderValueChanged: function (newValue) { | |
| id: tempSlider | |
| width: parent.width | |
| anchors.verticalCenter: parent.verticalCenter | |
| minimum: 3000 | |
| maximum: 10000 | |
| step: 100 | |
| value: colorTempRow.currentTemp === 0 ? 7000 : colorTempRow.currentTemp | |
| showValue: true | |
| unit: "K" | |
| wheelEnabled: false | |
| onSliderValueChanged: function (newValue) { |
| NightTime: times.Night, | ||
| IsDay: isDay, | ||
| SunPosition: pos, | ||
| ICCProfiles: m.GetICCStatus(), |
There was a problem hiding this comment.
GetICCStatus()/GetOutputTemps() read outputState.iccPath, iccProfile and outputTemp by ranging m.outputs, but those fields are only ever written on the wayland actor goroutine (loadConfiguredICC/attachConfiguredICC and the m.post bodies of ApplyICC/RemoveICC/SetOutputTemp). These getters are called off-actor:
updateStateFromSchedulehere runs on the scheduler goroutine (line 777) and on the IPC caller goroutine (lines 1510/1539/1631);handleICCGetStatus/handleICCGetTemps/handleICCListOutputsrun on the IPC connection goroutine.
So dms icc status (or the settings page polling) concurrent with a hotplug that runs attachConfiguredICC is an unsynchronised read/write of iccProfile — a data race go test -race will report, and a torn pointer read here dereferences in applyGamma. Before this PR updateStateFromSchedule only touched mutex-guarded config/schedule state.
Either move these fields behind a mutex, or have the actor publish a snapshot (e.g. store the status maps under stateMutex when it mutates them) and have the getters read that.
Separately, GetICCStatus does an os.Stat per profiled output, and it now runs on every updateStateFromSchedule — including every 25 K animation step during a sunrise/sunset transition.
| if (data.iccProfiles) | ||
| status = data.iccProfiles |
There was a problem hiding this comment.
State.ICCProfiles is tagged json:"iccProfiles,omitempty" (types.go:69) and GetICCStatus() returns an empty map once no output has a profile, so the key is omitted from the pushed state entirely. This truthiness guard then skips the assignment and status keeps the last non-empty value.
Result: dms icc remove DP-1 (or removing the last profile from any other surface) pushes a gamma state update with no iccProfiles, and the Display Config card keeps showing the removed profile with a green "active" dot indefinitely. The UI-initiated path only works because removeICC() follows up with requestStatus().
Check for undefined instead of truthiness so an empty/absent map clears the cache:
| if (data.iccProfiles) | |
| status = data.iccProfiles | |
| if (data.iccProfiles !== undefined) | |
| status = data.iccProfiles || ({}) |
| ColorSpace string `json:"colorSpace"` // e.g., "RGB" | ||
| HasVCGT bool `json:"hasVCGT"` // has video card gamma table |
There was a problem hiding this comment.
Correcting the suggestion I left on ICCService.qml:37 — my !== undefined version does not fix it, because omitempty drops the key entirely, so it is still undefined. The fix belongs here: without omitempty an empty map serialises as {} and the QML guard (if (data.iccProfiles)) sees a truthy object, so ICCService.status is cleared when the last profile is removed via dms icc remove.
| ColorSpace string `json:"colorSpace"` // e.g., "RGB" | |
| HasVCGT bool `json:"hasVCGT"` // has video card gamma table | |
| ICCProfiles map[string]*ICCStatus `json:"iccProfiles"` // outputName -> status | |
| OutputTemps map[string]int `json:"outputTemps"` // outputName -> current temp |
| } | ||
|
|
||
| // ListOutputs returns a list of all output names for ICC assignment. | ||
| func (m *Manager) ListOutputs() []string { |
There was a problem hiding this comment.
m.outputNames is stored in the wl_output name handler but never deleted in the SetGlobalRemoveHandler at line 225-258 (which does m.outputs.Delete(foundID)). Since this is the first code that enumerates that map, every disconnected monitor stays in it for the life of the daemon: wayland.icc.listOutputs and getStatus.outputs keep reporting monitors that are gone, so dms icc status grows a stale row per unplug/monitor-sleep cycle.
Add m.outputNames.Delete(foundID) next to m.outputs.Delete(foundID). That also closes the narrower window where a rebound wl_output reuses a released object ID and addOutputControl (line 443) attaches the previous monitor's profile from the stale name.
|
/claude review |
| if targetTemp <= 0 || targetTemp == baseTemp { | ||
| return ramp, nil | ||
| } | ||
|
|
||
| base := GenerateGammaRamp(size, baseTemp, gamma, contrast) | ||
| target := GenerateGammaRamp(size, targetTemp, gamma, contrast) | ||
|
|
||
| for i := range ramp.Red { | ||
| ramp.Red[i] = scaleRampValue(ramp.Red[i], base.Red[i], target.Red[i]) | ||
| ramp.Green[i] = scaleRampValue(ramp.Green[i], base.Green[i], target.Green[i]) | ||
| ramp.Blue[i] = scaleRampValue(ramp.Blue[i], base.Blue[i], target.Blue[i]) | ||
| } |
There was a problem hiding this comment.
The user's global gamma/contrast adjustments are silently dropped on any output with an ICC profile.
Two paths:
targetTemp <= 0 || targetTemp == baseTempreturns the raw vcgt ramp, with nogamma/contrastapplied at all. That's the common case: night light off (applyGamma(noTempTarget)→targetTemp == -1), or a per-output temp of exactly 6500.- The scaling path cancels them.
base[i] = pow(applyContrast(t, contrast), 1/gamma) * 65535andtarget[i] = pow(applyContrast(t, contrast) * wp_target, 1/gamma) * 65535, sotarget[i]/base[i] == wp_target^(1/gamma)— a per-channel constant.contrastcancels exactly, andgammaonly survives as an exponent on the temperature tint, never as a ramp adjustment.
Meanwhile needsControls() returns true for Gamma != 1.0 || Contrast != 1.0 and the non-ICC branch in applyGamma (manager.go:1011) does apply them, so with SessionData.displayGamma/displayContrast set to anything but 1.0 the profiled monitor visibly diverges from the rest and the sliders do nothing on it.
Apply gamma/contrast to the vcgt ramp itself (feed applyContrast(t, contrast) into the vcgt resample and raise the result to 1/gamma) before composing the temperature ratio, rather than relying on the ratio to carry them.
Claude reviewOne new finding; everything from the previous rounds looks addressed.
Checked: net diff against the merge base. ICC parser bounds and overflow handling (tag table, desc/mluc, curv, vcgt), ramp composition and the per-output dedup, actor-goroutine vs mutex discipline for the config and the published ICC snapshot, config load/save round trip and the night-light ownership split, control lifecycle across startup/hotplug/sleep/resume, the IPC handlers and the |
|
/claude review |
| m.publishICCState() | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
recreateOutputControl (line 549) still bails on !enabled, but this PR makes gamma controls exist while the night light is off (needsControls() now covers ICCProfiles/OutputTemps). Every other lifecycle path was switched to needsControls() — schedulerLoop, handleResume, syncControls, the registry handler — this one was not.
Failure: night light off, DP-1 has an ICC profile. The compositor sends failed on DP-1's gamma control (another client grabbed it, or the compositor dropped it). The failed handler sets out.failed = true, destroys the control and schedules the retry; the retry calls recreateOutputControl, which returns immediately because enabled is false. Nothing else clears failed for an existing outputState (gamma_size only arrives from a new control, controlStateFor only runs while !controlsInitialized), so applyGamma's case out.failed: continue skips that output for the life of the daemon — the calibration is silently gone until the user restarts DMS or toggles night light on. Same for a per-output temp override.
Use the same predicate as the rest of the lifecycle:
| func (m *Manager) recreateOutputControl(out *outputState) error { | |
| switch { | |
| case m.connectionDead.Load(): | |
| return nil | |
| case !m.needsControls() || !m.controlsInitialized: | |
| return nil |
(the m.configMutex.RLock()/enabled block above becomes unused and can go.)
| id: tempSlider | ||
| width: parent.width | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| minimum: 3000 |
There was a problem hiding this comment.
The per-output override can be set from the UI but never cleared from it. 0 is the "no override, follow the schedule" sentinel (SetOutputTemp, effectiveTempTarget, the Default branches in tempLabel and the label above it), but this slider's range starts at 3000 and setOutputTemp is only ever called from onSliderDragFinished, so the UI cannot produce 0.
Failure: on a default install the row reads "Default". The user drags the slider once — say to 6500, just to look at it. wayland.json now has outputTemps: {"DP-1": 6500} permanently, needsControls() is permanently true so gamma controls are created and held on an install that never wanted them, and the row can never show "Default" again. The only way back is dms icc set-temp DP-1 0 from a terminal — the ICC row right above has a remove button for exactly this, the temperature row has none.
Either add a reset affordance next to the label (calling ICCService.setOutputTemp(root.outputName, 0), mirroring iccRemoveButton), or make the bottom of the slider range mean "off".
Claude reviewTwo findings; everything from the previous rounds looks addressed (gamma/contrast on profiled outputs, CLI/IPC validation, night light ownership, name-map pruning, config map cloning, ICC-only state pushes).
Checked: net diff against merge base |
|
Claude review failed: https://github.com/AvengeMedia/DankMaterialShell/actions/runs/34981564013 |
…clearing a per-output temp
|
/claude review |
…erride Follow-up on d55d3d1, which fixed both points without tests: - TestManager_RecreateOutputControlFollowsNeedsControls pins the predicate: with the night light off and nothing configured there is nothing to recreate, and with a configured profile or a per-output temperature the control has to be recreated. Both configured cases return nil on 9226251. - TestManager_SetOutputTempZeroClearsTheOverride pins what the reset button in Display Config does end to end: the stored value and the published entry both disappear, so the row reads "Default" and the output follows the schedule. Also keeps the reset button's slot instead of revealing it on demand. The temperature row shares its column with the slider below it, so the height change between "Default" (no button) and an override moved the slider 14 px under the pointer while it was being dragged; the button is now always laid out and only its opacity and enabled state follow the override, which also lets the label width drop the conditional.
|
Both findings were already fixed on the branch in Regression tests for both fixes
One layout detail on the reset button The button is laid out unconditionally now, with Verification: |
| enabled: colorTempRow.currentTemp !== 0 | ||
| opacity: colorTempRow.currentTemp !== 0 ? 1 : 0 | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| onClicked: ICCService.setOutputTemp(root.outputName, 0) |
There was a problem hiding this comment.
The reset button clears the override but leaves the slider handle where it was dragged.
DankSlider.updateValueFromPosition assigns value = newValue on press/drag (dank-qml-common DankSlider.qml:56), which permanently destroys the declarative binding on line 595. After one drag tempSlider.value is a static number, so:
- pressing this button sets the override to 0 — the label flips to "Default", the icon/label lose the highlight and the button fades out — while the handle stays at the old temperature, which is exactly the state the button exists to leave;
- the slider also stops following
ICCService.outputTemps, so a laterdms icc set-temp DP-1 …(or a change from another surface) is not reflected.
SettingsSliderRow.qml:128 has the same problem and solves it by assigning slider.value in the reset handler. Re-establishing the binding fixes both cases:
| onClicked: ICCService.setOutputTemp(root.outputName, 0) | |
| onClicked: { | |
| ICCService.setOutputTemp(root.outputName, 0) | |
| tempSlider.value = Qt.binding(() => colorTempRow.currentTemp === 0 ? 7000 : colorTempRow.currentTemp) | |
| } |
| count := int(binary.BigEndian.Uint16(data[entry.offset+14 : entry.offset+16])) | ||
| entrySize := int(binary.BigEndian.Uint16(data[entry.offset+16 : entry.offset+18])) | ||
|
|
||
| if channels < 1 || count < 1 || entrySize < 1 { |
There was a problem hiding this comment.
channels < 3 is accepted here, but the loop below only fills ch < channels && ch < 3 while Red/Green/Blue are all allocated at count entries. A vcgt with channels == 1 (legal in the vcgt spec — one curve meant for all three channels) therefore returns a table whose green and blue arrays are entirely zero.
Nothing downstream catches that: ApplyICC only checks ColorSpace == "RGB" and HasVCGT, and ProfileRampWithTemp → resampleVCGT happily returns 0 for every position, so the daemon writes a ramp that blanks the green and blue channels — the display goes red and the path is persisted in wayland.json, so it comes back after a restart. The recovery is dms icc remove <output> from a terminal on an unreadable screen.
Either replicate channel 0 into green/blue when channels == 1, or reject anything other than 3 so the profile is refused with an error the way a non-RGB one is.
Claude reviewTwo findings, both minor; the previous rounds look addressed (gamma/contrast on profiled outputs,
Checked: net diff against merge base |
|
/claude review |
| // Without controls there are no outputs to write a ramp for, which is | ||
| // why the slider and `dms icc set-temp` used to do nothing until a | ||
| // restart on an install that had never enabled the night light. | ||
| m.ensureOutputControls() |
There was a problem hiding this comment.
The ICC paths create gamma controls but nothing ever releases them again, so needsControls() and controlsInitialized fall out of sync permanently.
syncControls() — the only caller of destroyControls() — is reached from SetAdjustments and SetEnabled only. ApplyICC (1610) and this call create controls when needsControls() becomes true, but the inverse transition has no path:
RemoveICCon the last profiled output, orSetOutputTemp(out, 0)on the last override, deletes the config entry soneedsControls()is false again, yet the controls stay up and keep holdingzwlr_gamma_control_v1for every output for the life of the daemon. Before this PR the control was always released once nothing needed it, so another gamma client (gammastep, wlsunset,wl-gammarelay) could take over; now it can't until DMS restarts.- Clearing an override on an install that never had controls is worse: this
ensureOutputControls()runs unconditionally, sodms icc set-temp DP-1 0(or the new reset button) creates the controls in order to clear the one thing that justified them, and then keeps them.
Both paths should end with m.syncControls() (or skip ensureOutputControls() when there is nothing left to apply), so the controls are torn down when needsControls() goes false.
| return | ||
| if (data.iccProfiles) | ||
| status = data.iccProfiles | ||
| if (data.outputTemps) |
There was a problem hiding this comment.
outputNames is never refreshed on a hotplug, so a monitor connected after the shell started gets no ICC row and no temperature slider.
outputNames is only assigned in requestStatus() (line 57), and requestStatus() only runs on capabilitiesReceived, on connect, at Component.onCompleted, and in the applyICC/removeICC/setOutputTemp callbacks. Nothing calls it when the set of outputs changes. The pushed state doesn't help either: the daemon publishes iccProfiles/outputTemps here but not the output list (State has no outputs field — ListOutputs() is only served by wayland.icc.getStatus/listOutputs).
Both new rows in OutputCard.qml gate on ICCService.outputNames.indexOf(root.outputName) !== -1 (lines 421 and 515), so for a display docked mid-session the Color Profile row and the Color Temp slider stay hidden until the shell restarts, or until the user happens to apply/remove a profile on some other output. The daemon side already does the right thing — attachConfiguredICC runs on the new wl_output's name event — it is only the UI that can't see it.
Either add the output list to the published state (publishICCState already runs on every hotplug via attachConfiguredICC/removeOutputByRegistryName) and read it here, or call requestStatus() from this handler when the pushed state names an output that isn't in outputNames yet.
Claude reviewTwo findings; the previous rounds look addressed (the vcgt channel count, the temp slider staying in sync after a drag,
Checked: net diff against merge base |
|
/claude review |
| if err != nil { | ||
| log.Fatalf("Failed to read ICC file: %v", err) | ||
| } | ||
| if err := os.WriteFile(destPath, data, 0644); err != nil { |
There was a problem hiding this comment.
destPath is keyed only on filepath.Base(filePath), so this silently overwrites an existing profile in the config dir that has the same file name.
dms icc apply DP-1 ~/cal/dp1/display.icc then dms icc apply DP-2 ~/cal/dp2/display.icc both resolve to <config>/dms/icc/display.icc: the second copy replaces the first, and wayland.json now has both outputs pointing at that one path. The daemon keeps the profile it already parsed in memory, so nothing looks wrong until the next start, when DP-1 is driven with DP-2's vcgt — a calibrated display quietly gets the wrong ramp, and the original file is gone.
copied is false in that case too, so even the rollback in dropCopiedProfile won't restore it.
Worth either erroring out when destPath exists with different content, or disambiguating the destination name (e.g. suffix on collision) before writing.
Claude reviewPrevious rounds look addressed; one new finding, in the CLI copy path.
Checked: net diff against merge base |
|
/claude review |
Claude reviewNo issues found. The findings from the previous rounds all look addressed. Checked: net diff against merge base. The four newest commits in detail — |
2 participants
Description
Adds per-output ICC color profile support and per-output color temperature to the night light / gamma stack.
ICC profiles
core/internal/iccpackage: ICC v2/v4 parser (TRC curves, LUT8/LUT16 and parametric curves,vcgt, description/version/color space metadata) with unit tests.iccProfilesandoutputTempsfrom the DMS config directory of the running compositor (<config>/<compositor>/dms/wayland.json, for example~/.config/niri/dms/wayland.jsonon niri), builds gamma ramps from the parsed profile per output, falls back to temperature ramps when a profile is missing or unparsable, and re-applies after resume.Per-output temperature
outputTempsin the same config;SetOutputTempaccepts 1000-10000K and the Display Config slider exposes the full 3000-10000K range.0= no override). It is composed on top of an ICC profile ramp, which is treated as measured at 6500K (D65); outputs without a profile are driven with the same value directly.dms icc set-temp <output> <kelvin>sets it from the CLI anddms icc statusreports it.IPC / CLI
wayland.icc.getStatus,wayland.icc.apply,wayland.icc.remove,wayland.icc.listOutputs,wayland.icc.setTemp,wayland.icc.getTemps.dms icc list | info <file> | apply <output> <file> | remove <output> | status | set-temp <output> <kelvin>.UI
quickshell/Services/ICCService.qml.Included fix
LoadConfig()now takes its fallback values fromDefaultConfig()and coversContrast.Validate()rejectsContrast == 0, so night light configs written before that field existed aborted manager construction and took gamma/ICC down entirely (wayland manager not initialized).Type of change
Related issues
Screenshots / video
Display Config output cards with the new Color Profile row (browse / apply / remove, showing an applied profile with version, color space and active state) and the per-output Color Temp slider set to 7000K on the calibrated display:
Checklist
I18n.tr()with translator context, reusing existing terms where possiblemake fmt, added/updated tests,make testpasses (TZ=UTC go test ./...-> 51 ok, 0 fail), andgo mod tidyis cleanmake lint-qmlwith no new warnings