Add app-mark icon to the app template and fix FontIconSource NaN crash#855
Conversation
The `dotnet new reactorapp` template now gives its title bar a small
app-mark icon: a Segoe Fluent Icons CircleRing glyph (U+EA3A) as a
placeholder, with a comment pointing authors at a bundled asset. That is
the only template change -- the interactive greeting, Mica backdrop, and
extended title bar were already there on main.
Adding that icon surfaced a framework bug. ResolveIconSource(IconData) for
FontIconData forced FontSize = double.NaN and FontFamily = null! on the
FontIconSource. A FontIconSource with a NaN FontSize is rejected by
TitleBar.set_IconSource with ArgumentException ("Value does not fall within
the expected range" / E_INVALIDARG), so any FontIcon without an explicit
size placed in an IconSource slot (TitleBar, TabView, ...) rendered as an
error panel instead of an icon.
Fix (src/Reactor/Core/V1Protocol/IconResolver.cs): a new
CreateFontIconSource(fi) helper mirrors the working IconElement path and
only assigns FontFamily/FontSize when specified.
Test: selftest regression PrivMount_TitleBarFontIconNoSize resolves a
no-size FontIcon and assigns it to a real TitleBar.IconSource, asserting no
ArgumentException.
Fixes microsoft#854
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
284e6fd to
29441ff
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the default dotnet new reactorapp WinUI template by adding a modern title-bar app-mark icon, and fixes a Reactor framework bug that caused FontIconSource creation to crash when FontSize was left unspecified (surfaced by the new template icon).
Changes:
- Template: add a placeholder Segoe Fluent Icons glyph (
\uEA3ACircleRing) as the title-bar app-mark icon, with a comment pointing to a future asset-based icon. - Framework: fix
IconResolver.ResolveIconSource(FontIconData)soFontFamily/FontSizeare only assigned when specified (avoidsFontSize = NaN/ null assignment patterns thatTitleBar.IconSourcerejects). - Selftest: add a regression check that assigns a no-size
FontIcon-derivedIconSourceto a realTitleBar.IconSourceand asserts it doesn’t throw.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/Templates/templates/WinUIApp-CSharp/App.cs | Adds a placeholder title-bar app-mark icon to the scaffolded app template. |
| src/Reactor/Core/V1Protocol/IconResolver.cs | Fixes FontIconSource construction to avoid invalid default assignments (prevents TitleBar.IconSource crash). |
| tests/Reactor.AppTests.Host/SelfTest/Fixtures/ReconcilerBigCoverageFixtures.cs | Adds a selftest regression to ensure a sizeless FontIcon resolves and can be assigned to TitleBar.IconSource. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try | ||
| { | ||
| _ = new WinXC.TitleBar { IconSource = barFontIconNoSize }; | ||
| titleBarAcceptedFontIcon = true; |
azchohfi
left a comment
There was a problem hiding this comment.
Automated PR review — app-mark icon + FontIconSource NaN fix
Ran the repo's multi-dimensional pr-review skill (security, correctness, api-ergonomics, alternative-solution, test-coverage, docs-and-samples, packaging) plus a cross-model check (GPT-5.5, high reasoning) over origin/main...HEAD.
Verdict: looks good to merge. The fix is clean and the right shape — extracting CreateFontIconSource to mirror the existing CreateFontIcon and only assigning FontFamily/FontSize when specified is exactly how the IconElement path already works, so the two projections are now consistent. No security, correctness, API, packaging, or alternative-solution concerns surfaced.
Good call adding a real-TitleBar regression selftest that asserts the assignment no longer throws — that's a non-vacuous guard (it fails if FontSize = double.NaN is restored). Cross-check note: the fixture doesn't force a layout/render pass, so it relies on TitleBar.set_IconSource validating synchronously at assignment (which matches the app mount path and the reported failure) — worth keeping in mind if that WinUI behavior ever changes.
Only two minor, non-blocking notes (inline below):
- M1 (medium, test-coverage): the new test only isolates the missing-
FontSizehalf of the change; the null-FontFamilybranch isn't exercised on its own. - L1 (low, docs): the template comment names glyph
U+EA3Aas "CircleRing", which independent sources dispute.
Summary: Critical: 0 High: 0 Medium: 1 Low: 1
| // via TitleBar(...).Icon(FontIcon("\uEA3A", "Segoe Fluent Icons")). | ||
| // Assign to a real TitleBar on the UI thread to lock the fix in. | ||
| var barFontIconNoSize = IconResolver.ResolveIconSource( | ||
| new FontIconData("\uEA3A", "Segoe Fluent Icons")); |
There was a problem hiding this comment.
M1 (medium, test-coverage): This regression case pins the FontSize-unset path, but it passes a non-null FontFamily ("Segoe Fluent Icons"), so the other half of the change — the if (fi.FontFamily is not null) branch in CreateFontIconSource — is never isolated. The pre-fix code also set FontFamily = fi.FontFamily is null ? null! : ..., so a null family was a distinct (and also potentially control-rejected) path.
Consider an adjacent check with a null family, e.g. new FontIconData("\uEA3A", fontFamily: null, fontSize: 18) assigned to a fresh TitleBar, so a future regression in the FontFamily branch can't slip through. Not a blocker.
| var (name, setName) = UseState("World"); | ||
|
|
||
| var titleBar = TitleBar("Company.ReactorApp1").Flex(shrink: 0); | ||
| // App-mark icon in the title bar: a Segoe Fluent Icons glyph (U+EA3A, |
There was a problem hiding this comment.
L1 (low, docs): The comment labels U+EA3A as "CircleRing", but that glyph name doesn't hold up — the Segoe Fluent Icons mapping for U+EA3A is reported elsewhere as something else (e.g. "ContactInfo"/"Movies" depending on source), so the name is at best ambiguous. Since the code only depends on the raw codepoint, safest to either drop the name and call it a "placeholder glyph", or confirm it against the official Segoe Fluent Icons list. Purely cosmetic.
…omment - Selftest: assert ResolveIconSource returns a real FontIconSource (not just that TitleBar assignment doesn't throw), and add a null-FontFamily case so the FontFamily branch of CreateFontIconSource is exercised in isolation. - Template: drop the disputed 'CircleRing' name for U+EA3A; call it a placeholder glyph. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review! Addressed the nits in fcf221a:
Verified: |
Summary
Gives the app scaffolded by
dotnet new reactorappa title-bar app-mark icon — the one bit of modern-title-bar polish the template was missing — and fixes a framework bug in icon-source resolution that adding the icon surfaced.The template already shipped a Mica backdrop, an extended/modern title bar, and an interactive
Hello, {name}!greeting. This PR leaves all of that untouched and only adds the icon.Linked issue
Fixes #854
What changed
Template —
tools/Templates/templates/WinUIApp-CSharp/App.cs(one line + a comment)Add a placeholder app-mark icon to the title bar: a Segoe Fluent Icons CircleRing glyph (
\uEA3A). A comment points authors at.Icon("ms-appx:///Assets/AppIcon.ico")for when they add a real asset.Framework fix —
src/Reactor/Core/V1Protocol/IconResolver.csAdding that icon hit a runtime crash. Reactor caught the exception and painted it into the window instead of rendering the app — so "just add a title-bar icon" didn't actually work before this fix.
ResolveIconSource(IconData)built theFontIconSourcefor the icon-source slot with an object initializer that always setFontSize = fi.FontSize ?? double.NaN(andFontFamily = null!when unspecified). AFontIconSourcewhoseFontSizeisNaNis rejected byTitleBar.set_IconSourcewithArgumentException: Value does not fall within the expected range(E_INVALIDARG). So anyFontIconplaced in anIconSourceslot (TitleBar,TabView, …) without an explicit size threw. The parallelIconElementpath (CreateFontIcon) was already correct — it only assignsFontFamily/FontSizewhen specified — which is why the bug stayed hidden (existing icon tests always passed an explicit size and never assigned the result to a real control).CreateFontIconSource(fi)helper that mirrorsCreateFontIcon—Glyphalways,FontFamily/FontSizeonly when specified — leaving them at their control defaults otherwise.Test
PrivMount_TitleBarFontIconNoSize(ReconcilerBigCoverageFixtures.cs): resolve a no-sizeFontIconand assign it to a realTitleBar.IconSource, asserting noArgumentException. Fails without the fix, passes with it.Test plan
dotnet build Reactor.slnx— clean.dotnet run --project tests\Reactor.AppTests.Host -p:Platform=x64 -- --self-test --filter "PrivateMountHotPaths"→ok PrivMount_TitleBarFontIconNoSize,# Total failures: 0.Risk / breaking changes
FontIcons that specified a size are unchanged; ones that omitted a size now correctly inherit the control's default size instead of throwing. No regression for callers that already worked.