Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Update the XML documentation to describe clamping toward black or white.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes Color.GetDimmerColor so extreme colors clamp directionally instead of falling back to gray values that may brighten the color.
Changes:
- Removes incorrect gray fallback returns.
- Updates tests for clamping and monotonic dimming.
- Requires updating the method documentation to reflect the new behavior.
| File | Summary |
|---|---|
Tests/UnitTestsParallelizable/Drawing/Color/ColorClassTests.DarkLightAwareness.cs |
Adds regression and directional monotonicity tests. |
Terminal.Gui/Drawing/Color/Color.cs |
Corrects dimming behavior; documentation still describes the removed gray fallbacks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // A color already at the extreme for the given direction is clamped by the math below: reducing | ||
| // the lightness of a near-black color lands on black, and increasing a near-white one lands on | ||
| // white. Substituting a named gray here would move the color the other way - dimming #101014 | ||
| // returned DarkGray (#767676), which is brighter than what it was asked to dim. |
GetDimmerColor substituted a named gray when the color had no room left for the step: DarkGray when reducing lightness, Gray when increasing it. On a dark theme DarkGray is brighter than what it replaced, so the method brightened what it was asked to dim - #101014 came back as #767676. ShadowView builds a transparent shadow by dimming the cells it covers, so on a dark ground the shadow came out lighter than the screen it fell on and lifted the dark grey text inside it. The workaround already in ShadowView - "if the BG is DarkGray, GetDimmerColor gave up" - is evidence of the same defect. A color with no room left is now returned unchanged. Clamping to the end of the range instead would sink it into the ground it is drawn against, which is where text drawn in it stops being readable, and that is the other half of what the named gray was standing in for. Scheme.DeriveAccent asked for the wrong direction on a light background - it passed isDark, which washes a near-white background further toward white - and only produced a darker accent because the Gray fallback caught it. It now asks for the dark direction on purpose. Tests: the two cases asserting the named grays assert the color is returned unchanged; two theories hold the contract that dimming never moves a color the wrong way; one holds that a dimmed foreground stays off its dimmed ground. The two transparent-shadow driver-output cases record that a shadow now dims what it covers rather than hiding it.
5ad2dee to
cb1a25b
Compare
|
Thanks for the fix. I think the PR merge block is incorrect and pointing to a comment that was addressed already. However I found one case you may want to address before merging: ShadowView calls GetDimmerColor(0.9) for the background under a transparent shadow. With the new guard, a mid-gray background such as #808080 is returned unchanged because the full step would cross black. That means the shadow may no longer darken its background. The updated shadow tests use white backgrounds, so they don’t cover this case. Could you add a test with a mid or dark background and adjust the behavior so the shadow still darkens it without making covered text disappear? |

Problem
Color.GetDimmerColorsubstitutes a named gray when the color has no room left for the step:On a dark theme
DarkGray(#767676) is brighter than the color it replaces, so the method brightens what it was asked to dim. Measured against 2.5.0:ShadowViewbuilds a transparent shadow by dimming the cells it covers, so on a dark ground the shadow comes out lighter than the screen it falls on, and dark grey text inside the shadow is lifted rather than sunk. The workaround already inShadowViewis evidence of the same defect:Fix
A color with no room left for the step is returned unchanged.
Clamping to the end of the range was the other candidate, and it costs readability: a near-black foreground would land on black and a near-black ground with it, so text under a shadow disappears into what it is drawn against. The named gray was standing in for two things at once — "do not return the same color" and "keep it visible" — and returning the color unchanged is the one of those that never lies about the direction.
Scheme.DeriveAccentThe accent derivation asked for the wrong direction on a light background:
Its own summary says the background is "shifted slightly brighter (on dark) or dimmer (on light)", but passing
isDark: falseasksGetDimmerColorto wash a near-white background further toward white. It only produced a darker accent because theGrayfallback caught it. It now asks for the dark direction on purpose, so it no longer depends on the fallback this PR removes.Tests
GetDimmerColor_VeryDarkInput_DarkBackground_*and..._VeryLightInput_LightBackground_*now assert the color is returned unchanged.ShadowTests.TransparentShadow_*_Draws_Transparent_At_Driver_Outputrecord that a cell under a transparent shadow keeps its glyph on a darkened ground (\x1b[100m->\x1b[40m, and a black foreground stays\x1b[30minstead of becoming\x1b[90m). If hiding what a shadow covers is the intended behaviour rather than dimming it, say so and I will withdraw that half.Tests/UnitTestsParallelizable(17634) andTests/UnitTests.NonParallelizable(32) pass.Tests/UnitTests.Legacyfails to start on my machine both with and without this change, so it is untouched by it.