Skip to content

Fix #3996: edit the display font size in points, not pixels - #3998

Merged
christophwille merged 2 commits into
masterfrom
fontsize-points-3996
Aug 14, 2026
Merged

christophwille merged 2 commits into
masterfrom
fontsize-points-3996

Conversation

@christophwille

Copy link
Copy Markdown
Member

Fixes #3996.

The options dialog bound DisplaySettings.SelectedFontSize (device-independent pixels) straight into a NumericUpDown, so a fresh profile showed 13.333... instead of 10, the increment stepped by 0.75 pt, and the 6-72 bounds were pixels (4.5-54 pt). The WPF host presented the value in points via FontSizeConverter; this restores that behavior on Avalonia, matching the Windows font dialogs (Notepad et al.).

What changed

  • ILSpy/Options/DisplaySettingsViewModel.cs - new SelectedFontSizePoints string proxy that shows Math.Round(px * 3 / 4) and stores pt * 4 / 3, mirroring the WPF FontSizeConverter. Non-numeric input is ignored (usually a transient typing state); numeric input is clamped to 6-72 pt. External changes to SelectedFontSize (reset-to-defaults, settings load) refresh the text via PropertyChanged, while writes originating from the size box suppress the echo so typing isn't clobbered mid-keystroke. Also adds FontSizes (6-24), the same dropdown list the WPF host offered.
  • ILSpy/Options/DisplaySettingsPanel.axaml - replaces the pixel-unit NumericUpDown with a "Size" label (existing Resources.Size string) plus an editable ComboBox (IsEditable="True", Text bound to the points proxy, items 6-24), i.e. the WPF panel's shape on Avalonia 12. The preview TextBlock still binds the raw pixel value, so it renders the true size.
  • ILSpy.Tests/Options/DisplayFontSizeTests.cs - new tests (written red-first): default 13.33 px displays as "10", typed "12" stores 16 px, external pixel change raises PropertyChanged and shows the right points, dialog-originated writes don't echo, garbage input is ignored, clamping at both ends, WPF-parity size list, and a headless UI test verifying the panel's size box is the editable ComboBox showing "10".

DisplaySettings.SelectedFontSize and its persistence are untouched, so settings files written by ILSpy 9.x still round-trip; EditorZoom and the live editor wire-up are unaffected.

Full ILSpy.Tests suite: 1178 passed, 0 failed, 4 skipped.

🤖 Generated with Claude Code

The options dialog bound DisplaySettings.SelectedFontSize (device-independent
pixels) straight into a NumericUpDown, so a fresh profile showed 13.33 and the
6-72 bounds were pixels. The WPF host presented points via FontSizeConverter;
this restores that behavior on Avalonia with an editable size ComboBox (like
the Windows font dialogs) backed by a pt/px proxy on the viewmodel. The stored
value stays pixels so settings files keep round-tripping with ILSpy 9.x.

Assisted-by: Claude:claude-fable-5:Claude Code
@christophwille

Copy link
Copy Markdown
Member Author

Code review

Verified locally: ILSpy/ILSpy.csproj builds clean on the branch (0 warnings), the Simple theme's ComboBox template does ship a PART_EditableTextBox (checked the compiled Avalonia.Themes.Simple 12.1.1 resource strings), so IsEditable="True" is really typeable and not just a no-op property; the Text two-way + ItemsSource wiring round-trips correctly through Avalonia's ComboBox.TextChanged/UpdateInputTextFromSelection (dropdown pick -> SelectedItem -> SetCurrentValue(TextProperty, ...) -> binding write-back). The initialization order is also safe: ComboBox.OnPropertyChanged for ItemsSourceProperty re-publishes Text (which is string.Empty before the Text binding lands), and the setter's "ignore non-numeric" rule is what keeps that from wiping the setting -- worth a comment, because that guard is load-bearing, not just typing UX.

Four things below.


1. Math.Clamp(NaN, 6, 72) returns NaN, and NaN gets persisted -- medium

ILSpy/Options/DisplaySettingsViewModel.cs:75-77

if (Settings == null || !double.TryParse(value, NumberStyles.Float, CultureInfo.CurrentCulture, out double points))
    return;
points = Math.Clamp(points, 6, 72);

double.TryParse with NumberStyles.Float accepts the culture's NaN symbol on .NET Core 3.0+ (verified: "NaN" and "nan" both parse, Math.Clamp(NaN, 6, 72) == NaN -- Clamp compares with </>, both false for NaN, so it falls through to return value). Infinity is fine (+inf -> 72, -inf -> 6, "1e400" -> 72); only NaN slips through.

Scenario: user types or pastes NaN into the size box.

  • DisplaySettings.SelectedFontSize = double.NaN.
  • SaveToXml writes FontSize="NaN"; LoadFromXml's (double?)section.Attribute("FontSize") reads it straight back, so it survives restart.
  • DecompilerTextEditor.ApplyFontSettings guards with if (displaySettings.SelectedFontSize > 0) -- false for NaN -- so from then on the editor never applies a font size again, on this run or any future one.
  • The preview TextBlock binding is rejected by Avalonia's TextElement.FontSizeProperty validate func (fontSize > 0 && !IsNaN && !IsInfinity), which surfaces as a logged binding error rather than a crash.
  • Recovery requires Reset-to-defaults or hand-editing ILSpy.xml.

The NumericUpDown this replaces could not produce a non-finite value. One line fixes it:

if (!double.IsFinite(points))
    return;

(The old WPF FontSizeConverter.ConvertBack had the same hole, so this isn't a WPF-parity regression -- but it is a regression against what ships today.)


2. macOS: the 4/3 factor is a Windows-DIP assumption -- medium

ILSpy/Options/DisplaySettingsViewModel.cs:63-73

The conversion is correct on Windows and Linux, and wrong-by-convention on macOS.

  • On Windows, a WPF/Avalonia device-independent pixel is defined as 1/96 in, and the Windows font dialogs report points where pt = DIP * 3/4. 4/3 is exactly right.
  • On Linux/X11, Avalonia derives RenderScaling from Xft.dpi/96, so the logical unit is again ~1/96 in and GTK-style point sizes line up.
  • On macOS, Avalonia.Native's RenderScaling is the NSWindow/NSScreen backingScaleFactor and DesktopScaling => 1.0 (decompiled Avalonia.Native 12.1.1). That makes one Avalonia logical unit identical to one Cocoa point -- and a Cocoa point is exactly the number every native macOS font UI puts in its "size" field (NSFontPanel, Xcode's editor font, VS Code's editor.fontSize).

Concretely on a Mac: Xcode's default SF Mono at "11" and ILSpy at SelectedFontSize = 11 render identically, but after this PR ILSpy's box would show 8 for that size. Conversely a Mac user who types 11 to match Xcode gets 14.67 logical units -- ~33% larger than every other app at the same stated number.

So on macOS this change makes the displayed number less comparable to the rest of the desktop than the raw pixel value was. Nothing crashes and nothing round-trips wrong, but the doc comment's premise ("points, like the Windows font dialogs") is a Windows premise living in a cross-platform view model, and #3996's complaint ("a fresh profile shows 13.333") has a different right answer per platform. Options, roughly in order of laziness:

  1. Ship as-is and accept the macOS mismatch (defensible -- the stored value still round-trips, and this matches the 9.x host).
  2. Label the unit in the UI (Size (pt):) so the number isn't silently compared against native Mac apps.
  3. Apply 4/3 only where the platform's logical unit is 1/96 in (i.e. skip it on OperatingSystem.IsMacOS()), which then makes the stored value platform-dependent -- probably not worth it.

Worth an explicit decision either way; right now the comment asserts a universal that only holds on two of the three targets.


3. A clamped value never makes it back into the box -- low/medium

ILSpy/Options/DisplaySettingsViewModel.cs:78-88

The echo suppression is unconditional, and there is no commit-on-lost-focus normalization, so whenever the typed text and the stored value disagree the dialog keeps lying:

  • Type 3 and tab away -> SelectedFontSize becomes 8 px (6 pt), the box still reads 3, and the preview under it renders 6 pt. Reopening the Options page is the only way to resync.
  • Same for 500 -> stored 96 px, box still reads 500.

NumericUpDown handled this: OnLostFocus -> CommitInput(forceTextUpdate: true) rewrote the text from the clamped value.

The suppression is only needed while the round-trip text is unchanged, so making it conditional fixes both without a focus handler:

var text = points.ToString(CultureInfo.CurrentCulture);
updatingFontSizeFromText = text == value;   // only suppress when the box already shows the truth
try { Settings.SelectedFontSize = points * 4 / 3; }
finally { updatingFontSizeFromText = false; }

4. The headless test doesn't actually exercise the editable box -- low

ILSpy.Tests/Options/DisplayFontSizeTests.cs:206-210

box!.IsEditable.Should().BeTrue(...);
box.Text.Should().Be("10", ...);

Both are plain styled-property reads on the ComboBox; they pass whether or not the applied ControlTheme actually realizes an editable text box. (I confirmed by hand that the Simple theme does -- but the test wouldn't have caught it if it didn't, which is the interesting failure this test is meant to guard.) box.ApplyTemplate() plus asserting a TextBox named PART_EditableTextBox exists in the visual tree would make the assertion mean what its "custom sizes must be typeable" message claims.

Two smaller notes on the same file:

  • Default_Pixel_Size_Displays_As_10_Points sets SelectedFontSize = 10.0 * 4 / 3 itself rather than reading the actual default, so it doesn't test the default. ResetAppStateAttribute rebuilds the MEF container per test, so a fresh DisplaySettings is already there -- the assignment (and the try/finally restores throughout the fixture) can just go.
  • NonNumeric_Input_Is_Ignored is the test that pins down the load-bearing behaviour from the intro paragraph (empty Text published during ItemsSource initialization must not clobber the setting). Worth saying so in the comment, otherwise someone will "improve" the setter into falling back to a default like the WPF converter did (return 11.0 * 4 / 3) and silently reset user font sizes on every page load.

Cosmetic

ILSpy/Options/DisplaySettingsPanel.axaml:30 -- Resources.Size is "Size:" (with colon) while the neighbouring Resources.Font is "Font" (without), so the row renders Font [ ] Size: [ ]. The WPF panel had the same asymmetry, so this is parity, not a regression -- just noting it since the label is new here.

@christophwille christophwille left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings from the review inline below (summary is in the comment above).

Comment thread ILSpy/Options/DisplaySettingsViewModel.cs
Comment thread ILSpy/Options/DisplaySettingsViewModel.cs Outdated
Comment thread ILSpy/Options/DisplaySettingsViewModel.cs
Comment thread ILSpy.Tests/Options/DisplayFontSizeTests.cs Outdated
christophwille added a commit that referenced this pull request Aug 14, 2026
…drift

Review follow-ups on #3998: reject non-finite parses (NaN slips through
Math.Clamp and, once persisted, permanently fails the editor's
SelectedFontSize > 0 guard), commit the clamped value back into the box on
focus loss (the echo suppression otherwise leaves a typed "3" on screen while
6 pt is stored), and assert the theme actually realizes PART_EditableTextBox
instead of trusting the IsEditable property. The 4/3 pt/px ratio is documented
as the WPF-host convention it is - exact on Windows/X11, deliberately not the
Cocoa-point number on macOS - rather than a universal.

Assisted-by: Claude:claude-fable-5:Claude Code
@christophwille

Copy link
Copy Markdown
Member Author

All four addressed in 1bcb33b; replies with details on each thread. Short version: NaN is rejected via double.IsFinite before the clamp; the macOS 4/3 question is decided as "keep the WPF-host convention, document it honestly" (stored value stays host-independent); the stale-clamped-text issue is fixed with NumericUpDown-style commit-on-LostFocus instead of conditional echo suppression (the conditional variant re-breaks the 10. and leading-1 typing cases); and the headless test now asserts PART_EditableTextBox is realized, tests the genuine default, and pins the load-bearing empty-Text guard. The cosmetic Size:/Font label asymmetry is left as WPF parity. Options tests all green (10/10 new + 10/10 OptionsTabTests).

christophwille added a commit that referenced this pull request Aug 14, 2026
…drift

Review follow-ups on #3998: reject non-finite parses (NaN slips through
Math.Clamp and, once persisted, permanently fails the editor's
SelectedFontSize > 0 guard), commit the clamped value back into the box on
focus loss (the echo suppression otherwise leaves a typed "3" on screen while
6 pt is stored), and assert the theme actually realizes PART_EditableTextBox
instead of trusting the IsEditable property. The 4/3 pt/px ratio is documented
as the WPF-host convention it is - exact on Windows/X11, deliberately not the
Cocoa-point number on macOS - rather than a universal.

Assisted-by: Claude:claude-fable-5:Claude Code
@christophwille

Copy link
Copy Markdown
Member Author

The Ubuntu leg failed on the new Clamped_Value_Is_Written_Back_Into_The_Box_On_Focus_Loss test: it raised LostFocus synthetically with a plain RoutedEventArgs, but the event is typed (FocusChangedEventArgs), so the first typed subscriber on the route - present on the Linux headless run, absent on Windows - hit an InvalidCastException in the handler adapter. Fixed by performing a real focus traversal instead: focus the box's PART_EditableTextBox, then focus a checkbox elsewhere on the panel, which raises the genuine event and exercises the actual commit path. (ComboBox.Focus() itself returns false when IsEditable - focus is delegated to the inner text box - which the test now documents.) Squashed into the hardening commit per branch convention, now 9fbebb8.

Comment thread ILSpy/Options/DisplaySettingsViewModel.cs Outdated
…drift

Review follow-ups on #3998: reject non-finite parses (NaN slips through
Math.Clamp and, once persisted, permanently fails the editor's
SelectedFontSize > 0 guard), commit the clamped value back into the box on
focus loss (the echo suppression otherwise leaves a typed "3" on screen while
6 pt is stored), and assert the theme actually realizes PART_EditableTextBox
instead of trusting the IsEditable property. The 4/3 pt/px ratio is documented
as the WPF-host convention it is - exact on Windows/X11, deliberately not the
Cocoa-point number on macOS - rather than a universal.

Assisted-by: Claude:claude-fable-5:Claude Code
@christophwille
christophwille merged commit 487630e into master Aug 14, 2026
15 checks passed
@christophwille
christophwille deleted the fontsize-points-3996 branch August 14, 2026 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Options dialog edits the font size in pixels, not points

2 participants