Add monochrome palette detection and fallback palette resolution#145
Conversation
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com> Agent-Logs-Url: https://github.com/jongalloway/DiagramForge/sessions/27f13f43-5376-482e-8812-a2b0189deafb
There was a problem hiding this comment.
Pull request overview
Adds shared infrastructure to detect achromatic/monochrome palettes and derive a usable fallback palette, preventing “invisible” node fills and improving robustness of color derivation utilities.
Changes:
- Added
ColorUtils.IsAchromatic/ColorUtils.IsPaletteMonochromeand hardenedColorUtils.Vibrant()for achromatic inputs. - Added
ThemePaletteResolver.ResolveEffectivePalette(theme)to return either the theme palette (when usable) or an 8-color fallback derived from gradient stops or hue rotation. - Added unit tests covering the new utilities and fallback behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/DiagramForge.Tests/Models/ThemePaletteResolverTests.cs | Adds tests for ResolveEffectivePalette behavior across monochrome and gradient-stop fallback scenarios. |
| tests/DiagramForge.Tests/Models/ColorUtilsTests.cs | Adds tests for IsAchromatic, IsPaletteMonochrome, and the new achromatic behavior in Vibrant(). |
| src/DiagramForge/Models/ThemePaletteResolver.cs | Implements ResolveEffectivePalette plus internal helpers for gradient sampling and hue-rotation palette derivation. |
| src/DiagramForge/Models/ColorUtils.cs | Implements achromatic/monochrome detection and modifies Vibrant() to handle achromatic inputs explicitly. |
Comments suppressed due to low confidence (1)
tests/DiagramForge.Tests/Models/ColorUtilsTests.cs:813
- Test name doesn’t match the assertion:
IsPaletteMonochrome_MatchesBackground_NoBackground_ReturnsFalseassertsTrue. Rename the test (e.g., “…ReturnsTrue”) or change the assertion so the name accurately describes the expected behavior.
public void IsPaletteMonochrome_MatchesBackground_NoBackground_ReturnsFalse()
{
// Same chromatic entries, but without a background to compare against —
// they are the same color, so still "all same color" → true
var palette = new[] { "#2563EB", "#2563EB", "#2563EB" };
Assert.True(ColorUtils.IsPaletteMonochrome(palette, backgroundColor: null));
}
| /// <param name="hex">Hex color string.</param> | ||
| /// <param name="saturationThreshold"> | ||
| /// Saturation threshold 0–1; colors with saturation below this value are considered | ||
| /// achromatic. Defaults to 0.08. | ||
| /// </param> | ||
| public static bool IsAchromatic(string hex, double saturationThreshold = 0.08) | ||
| { | ||
| var (rRaw, gRaw, bRaw) = ParseHex(hex); | ||
| double r = rRaw / 255d; | ||
| double g = gRaw / 255d; | ||
| double b = bRaw / 255d; | ||
| double max = Math.Max(r, Math.Max(g, b)); | ||
| double min = Math.Min(r, Math.Min(g, b)); | ||
| double delta = max - min; | ||
| double lightness = (max + min) / 2; | ||
| double denominator = 1 - Math.Abs(2 * lightness - 1); | ||
| // When delta is near zero the color is already achromatic; guard the denominator | ||
| // (which is also ~0 at lightness=0 or lightness=1) to avoid division by zero. | ||
| double saturation = delta < 0.0001 || denominator < 0.0001 | ||
| ? 0 | ||
| : delta / denominator; | ||
| return saturation < saturationThreshold; |
There was a problem hiding this comment.
The XML doc for saturationThreshold says it is a 0–1 value, but the method currently accepts any double without clamping/validation. Either clamp saturationThreshold into [0,1] (similar to Blend/WithOpacity) or update the docs to reflect the actual behavior to avoid surprising results for negative/large thresholds.
| private static IReadOnlyList<string> BuildPaletteFromHueRotation( | ||
| string accentColor, string secondaryColor, int count) | ||
| { | ||
| bool isLight = ColorUtils.IsLight(accentColor); | ||
| double hueStep = 360.0 / count; | ||
| var result = new List<string>(count); | ||
|
|
||
| for (int i = 0; i < count; i++) | ||
| { | ||
| string baseColor = i % 2 == 0 ? accentColor : secondaryColor; | ||
| double rotation = Math.Floor(i / 2.0) * hueStep; | ||
| result.Add(ColorUtils.RotateHue(baseColor, rotation, isLight)); | ||
| } |
There was a problem hiding this comment.
BuildPaletteFromHueRotation passes ColorUtils.IsLight(accentColor) as the isLightBackground argument to ColorUtils.RotateHue. That parameter controls clamping based on the canvas/background, not whether the accent color itself is light. This can skew the generated palette (too dark/light) depending on theme background. Consider deriving isLightBackground from theme.BackgroundColor (and passing it down) or adding an explicit parameter so callers can provide the correct background context.
| // Build fallback from gradient stops when they are available and meaningful. | ||
| if (theme.UseBorderGradients && theme.BorderGradientStops is { Count: > 1 }) | ||
| return BuildPaletteFromGradientStops(theme.BorderGradientStops, DefaultPaletteSize); | ||
|
|
||
| // Fall back to hue-rotation derivation from the theme's semantic colors. | ||
| return BuildPaletteFromHueRotation(theme.AccentColor, theme.SecondaryColor, DefaultPaletteSize); |
There was a problem hiding this comment.
ResolveEffectivePalette can still return a monochrome palette if BorderGradientStops are present but themselves monochrome/achromatic (e.g., two identical stops, or all-gray stops). Since the method’s contract is to return a palette suitable for direct fill use, consider validating the gradient-derived palette with IsPaletteMonochrome (optionally against theme.BackgroundColor) and falling back to the hue-rotation path when the gradient result is still monochrome.
| /// <item> | ||
| /// If <see cref="Theme.UseBorderGradients"/> is <see langword="true"/> and | ||
| /// <see cref="Theme.BorderGradientStops"/> contains more than one stop, the stops | ||
| /// are interpolated/cycled to produce <see cref="DefaultPaletteSize"/> entries. |
There was a problem hiding this comment.
The remarks mention gradient stops are “interpolated/cycled”, but BuildPaletteFromGradientStops only linearly interpolates across the stop range and does not cycle/repeat stops. Please update the comment to match the actual behavior (or implement cycling if that was intended).
| /// are interpolated/cycled to produce <see cref="DefaultPaletteSize"/> entries. | |
| /// are linearly interpolated to produce <see cref="DefaultPaletteSize"/> entries. |
| public void IsAchromatic_CustomThreshold_LowSaturationColor_ReturnsTrue() | ||
| { | ||
| // A muted blue — below the default threshold but above an even lower custom one | ||
| // Test that the threshold parameter is respected | ||
| Assert.False(ColorUtils.IsAchromatic("#4F81BD", saturationThreshold: 0.01)); | ||
| } |
There was a problem hiding this comment.
Test name doesn’t match the assertion: IsAchromatic_CustomThreshold_LowSaturationColor_ReturnsTrue asserts False. Rename the test (e.g., “…ReturnsFalse”) or adjust the expectation so the name accurately reflects the behavior being validated.
This issue also appears on line 807 of the same file.
| if (palette.All(c => string.Equals(c, palette[0], StringComparison.OrdinalIgnoreCase))) | ||
| return true; | ||
|
|
||
| // All entries match the background (nodes would be invisible against the canvas). | ||
| if (backgroundColor is not null && | ||
| palette.All(c => string.Equals(c, backgroundColor, StringComparison.OrdinalIgnoreCase))) | ||
| return true; |
There was a problem hiding this comment.
IsPaletteMonochrome compares colors using string.Equals, which treats equivalent hex encodings as different (e.g. #F00 vs #FF0000, or #ABC vs #AABBCC). Because ColorUtils explicitly supports shorthand formats, this can incorrectly return false for palettes that are effectively monochrome or match the background. Consider normalizing colors (e.g., via ParseHex/ParseHexWithAlpha or ExpandShorthand) before doing “all same” / “matches background” comparisons, and compare by RGB(A) values rather than raw strings.
| if (palette.All(c => string.Equals(c, palette[0], StringComparison.OrdinalIgnoreCase))) | |
| return true; | |
| // All entries match the background (nodes would be invisible against the canvas). | |
| if (backgroundColor is not null && | |
| palette.All(c => string.Equals(c, backgroundColor, StringComparison.OrdinalIgnoreCase))) | |
| return true; | |
| { | |
| var referenceColor = ParseHexWithAlpha(palette[0]); | |
| if (palette.All(c => ParseHexWithAlpha(c) == referenceColor)) | |
| return true; | |
| } | |
| // All entries match the background (nodes would be invisible against the canvas). | |
| if (backgroundColor is not null) | |
| { | |
| var background = ParseHexWithAlpha(backgroundColor); | |
| if (palette.All(c => ParseHexWithAlpha(c) == background)) | |
| return true; | |
| } |
|
@copilot apply changes based on the comments in this thread |
…ackground, validate gradient palette, fix test names Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com> Agent-Logs-Url: https://github.com/jongalloway/DiagramForge/sessions/4b46141f-444c-46cc-9873-d40f91adb1a3
Applied all six review comments in commit
|
Themes with all-achromatic
NodePaletteentries (e.g. Prism's all-#FFFFFFpalette) had no shared infrastructure to detect the condition or substitute meaningful colors—leading to invisible fills, hue-scoring no-ops, and silentVibrant()failures.ColorUtilsadditionsIsAchromatic(hex, saturationThreshold = 0.08)— computes HSL saturation and returnstruebelow the threshold; guards against division-by-zero at extreme lightness values.IsPaletteMonochrome(palette, backgroundColor?)— returnstruewhen all entries are achromatic, all are the same color, or all match the background (invisible nodes). Returnsfalsefor empty palettes.Vibrant()hardening — detects achromatic input viaIsAchromaticand applies an explicit lightness shift (light → −0.35, dark → +0.35, bounded to [0.25, 0.75]) instead of silently running the hue-amplification formula on colorless input.ThemePaletteResolveradditionResolveEffectivePalette(theme)— pure function returning a usable 8-entry fill palette. ReturnsNodePaletteunchanged when it's chromatic; otherwise falls back to:BorderGradientStopsto 8 entries (whenUseBorderGradients && BorderGradientStops.Count > 1)AccentColor/SecondaryColoracross 8 evenly-spaced positionsThese are additive utilities only—no existing rendering paths are changed; per-diagram wiring is handled in follow-on sub-issues.
⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.