Skip to content

first color/style change not rendered until a second change occurs#77

Merged
Zeljko-Predjeskovic merged 1 commit into
developfrom
fix/pin_didnt_color
Jul 17, 2026
Merged

first color/style change not rendered until a second change occurs#77
Zeljko-Predjeskovic merged 1 commit into
developfrom
fix/pin_didnt_color

Conversation

@Zeljko-Predjeskovic

Copy link
Copy Markdown
Collaborator

Fix: first color/style change not rendered until a second change occurs

Problem

Recoloring an element (e.g. a pin) with the color tool was inconsistent — the first color change often had no visible effect, and only a second change (or an intervening pan/zoom) made it show. The element model updated immediately; only the rendered output lagged.

Cause

Each SvgVisualElement caches its fill/stroke brush per renderer and rebuilds it only when its attribute-change token changes. The token starts as Guid.Empty and begins tracking only after the first render, so an element enters its first render still holding Empty. SetAttributeChangeToken treated Empty as "initial, don't dispose," which swallowed the first real change — the stale old-color brush kept being drawn until a second change flipped the token again.

Fix

Always rebuild the cached pens/brushes when the token differs. Disposing a freshly-created entry is a no-op (its brushes are null), so the Guid.Empty special-case is unnecessary and was the bug.

Single-line change in Svg/Basic Shapes/SvgVisualElement.cs. Applies to all visual elements, not just pins.

@Zeljko-Predjeskovic

Zeljko-Predjeskovic commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Review: the fix is correct, minimal, and low-risk ✅

I traced the full token lifecycle in SvgVisualElement.cs and validated the change.

Root cause — confirmed accurate

  1. An element is created with token Guid.Empty; token tracking stays disabled until the first render.
  2. First render creates the cache entry (token Empty) and lazily builds the brush via ??=.
  3. The first color change bumps the element token to X.
  4. Old code: the cache entry's token was still Empty, so SetAttributeChangeToken(X) hit the == Guid.Empty branch and set the token to X without disposing — the stale (old-color) brush kept being drawn. This is exactly the swallowed first change.
  5. Only the second change disposed + rebuilt, hence "the color only changes on the second attempt."

Removing the Empty special-case fixes it.

No regression risk

  • No perf hit on the genuine first render: tracking is off until the first GetOrCreateRenderCacheEntry call, so the real first render still has Empty == Empty → no dispose, fresh build only.
  • Disposing a fresh entry is a no-op — every brush/pen is null-guarded in RenderCacheEntry.Dispose, so the removed guard was never protecting anything.
  • The change lives in the core Svg library, so it applies to all visual elements, with no public API change.
  • Existing render-cache tests are unaffected: Svg.Tests.Win/SvgRendererTests.cs mocks RenderCacheEntry without CallBase, so they only assert SetAttributeChangeToken is invoked with the right token — the changed body doesn't run there.

Test coverage — solid, one small gap

RenderCacheColorTests is a faithful reproduction: it reuses the same renderer across two OnDraw calls (the cache persists on the canvas' _svgRenderer), renders black → changes fill → re-renders, and asserts red pixels appear. Against the old code redAfter would be 0, so it genuinely fails pre-fix.

  • Gap (minor): only fill is covered; the first-time stroke color change uses the same Dispose() path but has no dedicated test.

Comment thread Svg/Basic Shapes/SvgVisualElement.cs
@Zeljko-Predjeskovic
Zeljko-Predjeskovic merged commit 4b45e2a into develop Jul 17, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant