Add monochrome palette fallback infrastructure#152
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds shared infrastructure to ensure monochrome themes (notably Prism) can reliably produce chromatic fallback palettes for repeated visual elements, and wires that into ring-color generation so hue-distance selection doesn’t degenerate on achromatic inputs.
Changes:
- Added a count-aware
ThemePaletteResolver.ResolveEffectivePalette(theme, desiredCount)overload to size fallback palettes for repeated elements. - Added
ColorUtils.Vibrant(hex, achromaticFallback, amplify)to substitute a chromatic fallback when the source color is achromatic while preserving alpha. - Routed
BuildRingColorsthrough the effective palette and updated unit tests + Prism target snapshot to reflect improved Prism ring coloring.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/DiagramForge/Models/ThemePaletteResolver.cs |
Adds count-aware effective palette resolution and uses it to make BuildRingColors robust for monochrome themes. |
src/DiagramForge/Models/ColorUtils.cs |
Introduces a Vibrant overload that swaps in a chromatic fallback for achromatic inputs while preserving alpha. |
tests/DiagramForge.Tests/Models/ThemePaletteResolverTests.cs |
Adds tests covering Prism fallback sizing and Prism ring-color chromatic fallback behavior. |
tests/DiagramForge.Tests/Models/ColorUtilsTests.cs |
Adds a test validating achromatic Vibrant fallback uses the fallback hue and preserves alpha. |
tests/DiagramForge.E2ETests/Fixtures/conceptual-target-prism.expected.svg |
Updates expected Prism target SVG output due to new chromatic ring candidate selection. |
Comments suppressed due to low confidence (1)
src/DiagramForge/Models/ThemePaletteResolver.cs:169
- In BuildRingColors,
ResolveEffectivePalette(...)andisThemePaletteMonochromeare computed before theringCount == 1early-return. For the single-ring case this does unnecessary palette work and allocations. Consider moving theringCount == 1guard (and any dependent variables) before resolving the chromatic palette.
var chromaticPalette = ResolveEffectivePalette(theme, Math.Max(DefaultPaletteSize, ringCount * 2));
bool isThemePaletteMonochrome = theme.NodePalette is { Count: > 0 }
&& ColorUtils.IsPaletteMonochrome(theme.NodePalette, theme.BackgroundColor);
var colors = new List<string>(ringCount)
{
outerColor,
};
if (ringCount == 1)
return [.. colors];
| /// <summary> | ||
| /// Returns a chromatic palette sized for the caller's repeated-element needs. | ||
| /// </summary> | ||
| /// <param name="theme">Source theme.</param> | ||
| /// <param name="desiredCount">Number of entries to produce for fallback palettes.</param> | ||
| public static IReadOnlyList<string> ResolveEffectivePalette(Theme theme, int desiredCount) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(theme); | ||
| if (desiredCount < 1) | ||
| throw new ArgumentOutOfRangeException(nameof(desiredCount), desiredCount, "desiredCount must be at least 1."); |
There was a problem hiding this comment.
The new ResolveEffectivePalette(Theme, int desiredCount) XML summary implies the returned palette is sized to desiredCount, but when the theme has a non-monochrome NodePalette the method returns it unchanged (which may be shorter than desiredCount). Please clarify the contract in the XML docs (e.g., that desiredCount only applies to fallback palette generation) and document the ArgumentOutOfRangeException thrown when desiredCount < 1.
|
@copilot apply changes based on the comments in this thread |
…xceptions Agent-Logs-Url: https://github.com/jongalloway/DiagramForge/sessions/0bc8aea5-ed48-4f4c-963a-7b5bd06c180e Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Done in commit |
Closes #150.
Summary
ThemePaletteResolver.ResolveEffectivePaletteoverload so callers can request a chromatic fallback palette sized for repeated elementsColorUtils.Vibrantoverload that substitutes a chromatic fallback when the source color is achromatic while preserving alphaBuildRingColorsthrough the shared effective palette so Prism-style monochrome node palettes still produce chromatic ring candidatesValidation
dotnet test d:\Users\Jon\Documents\GitHub\DiagramForge\tests\DiagramForge.Tests\DiagramForge.Tests.csproj --filter "FullyQualifiedName~ThemePaletteResolverTests"dotnet test d:\Users\Jon\Documents\GitHub\DiagramForge\tests\DiagramForge.Tests\DiagramForge.Tests.csproj --filter "FullyQualifiedName~BuildRingColors_PrismTheme_UsesChromaticInnerRingFallback|FullyQualifiedName~ResolveEffectivePalette_PrismTheme_WithRequestedCount_ReturnsChromaticFallbackOfRequestedSize|FullyQualifiedName~Vibrant_AchromaticInput_WithChromaticFallback_UsesFallbackHueAndPreservesAlpha"Note: in this environment the runner executed the full
DiagramForge.Testsassembly instead of honoring the narrow filter, and all tests passed.