From 487630e8de5b72111bca6600d4fd301f22b0aaef Mon Sep 17 00:00:00 2001 From: Christoph Wille Date: Fri, 14 Aug 2026 14:49:12 +0200 Subject: [PATCH] Harden the point-size box against NaN, stale clamped text, and theme 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 --- ILSpy.Tests/Options/DisplayFontSizeTests.cs | 237 +++++++++++--------- ILSpy/Options/DisplaySettingsPanel.axaml | 3 +- ILSpy/Options/DisplaySettingsPanel.axaml.cs | 5 + ILSpy/Options/DisplaySettingsViewModel.cs | 30 ++- 4 files changed, 161 insertions(+), 114 deletions(-) diff --git a/ILSpy.Tests/Options/DisplayFontSizeTests.cs b/ILSpy.Tests/Options/DisplayFontSizeTests.cs index 5097184ef..a1d901abd 100644 --- a/ILSpy.Tests/Options/DisplayFontSizeTests.cs +++ b/ILSpy.Tests/Options/DisplayFontSizeTests.cs @@ -23,6 +23,7 @@ using System.Threading.Tasks; using Avalonia.Controls; using Avalonia.Headless.NUnit; using Avalonia.Threading; +using Avalonia.VisualTree; using AwesomeAssertions; @@ -40,10 +41,12 @@ using NUnit.Framework; namespace ICSharpCode.ILSpy.Tests; /// -/// The options dialog edits the font size in points (like the Windows font dialogs and the -/// WPF host's FontSizeConverter), while keeps +/// The options dialog edits the font size in points (the unit Windows font dialogs use), +/// while keeps /// storing device-independent pixels so persisted settings round-trip with ILSpy 9.x. /// The pt/px conversion lives in . +/// No per-test save/restore of the settings here: ResetAppState rebuilds the container and +/// settings file before every test, so each test starts from pristine defaults. /// [TestFixture] public class DisplayFontSizeTests @@ -59,36 +62,22 @@ public class DisplayFontSizeTests [AvaloniaTest] public void Default_Pixel_Size_Displays_As_10_Points() { + // Deliberately no arrange: this asserts on the genuine built-in default. var (page, settings) = CreatePage(); - var original = settings.SelectedFontSize; - try - { - settings.SelectedFontSize = 10.0 * 4 / 3; - page.SelectedFontSizePoints.Should().Be("10", - "the default 13.33 px must be presented as the 10 pt it actually is"); - } - finally - { - settings.SelectedFontSize = original; - } + settings.SelectedFontSize.Should().BeApproximately(10.0 * 4 / 3, 1e-9, + "precondition: the built-in default is 10 pt expressed in pixels"); + page.SelectedFontSizePoints.Should().Be("10", + "the default 13.33 px must be presented as the 10 pt it actually is"); } [AvaloniaTest] public void Typed_Point_Size_Is_Stored_As_Pixels() { var (page, settings) = CreatePage(); - var original = settings.SelectedFontSize; - try - { - page.SelectedFontSizePoints = "12"; - settings.SelectedFontSize.Should().BeApproximately(12.0 * 4 / 3, 1e-9, - "the stored value stays device-independent pixels for 9.x round-tripping"); - page.SelectedFontSizePoints.Should().Be("12"); - } - finally - { - settings.SelectedFontSize = original; - } + page.SelectedFontSizePoints = "12"; + settings.SelectedFontSize.Should().BeApproximately(12.0 * 4 / 3, 1e-9, + "the stored value stays device-independent pixels for 9.x round-tripping"); + page.SelectedFontSizePoints.Should().Be("12"); } [AvaloniaTest] @@ -97,21 +86,13 @@ public class DisplayFontSizeTests // Reset-to-defaults and LoadFromXml write SelectedFontSize directly; the dialog text // must follow via PropertyChanged on SelectedFontSizePoints. var (page, settings) = CreatePage(); - var original = settings.SelectedFontSize; - try - { - var notified = new List(); - page.PropertyChanged += (_, e) => notified.Add(e.PropertyName); - - settings.SelectedFontSize = 20; - - notified.Should().Contain(nameof(DisplaySettingsViewModel.SelectedFontSizePoints)); - page.SelectedFontSizePoints.Should().Be("15"); - } - finally - { - settings.SelectedFontSize = original; - } + var notified = new List(); + page.PropertyChanged += (_, e) => notified.Add(e.PropertyName); + + settings.SelectedFontSize = 20; + + notified.Should().Contain(nameof(DisplaySettingsViewModel.SelectedFontSizePoints)); + page.SelectedFontSizePoints.Should().Be("15"); } [AvaloniaTest] @@ -120,97 +101,139 @@ public class DisplayFontSizeTests // While the user is typing in the size box, the setter must not raise PropertyChanged // for SelectedFontSizePoints - the binding would rewrite the box mid-keystroke // (typing "10." would snap back to "10" before the fraction can be completed). + var (page, _) = CreatePage(); + var notified = new List(); + page.PropertyChanged += (_, e) => notified.Add(e.PropertyName); + + page.SelectedFontSizePoints = "14"; + + notified.Should().NotContain(nameof(DisplaySettingsViewModel.SelectedFontSizePoints)); + } + + [AvaloniaTest] + public void NonNumeric_And_Empty_Input_Are_Ignored() + { + // The empty-string case is load-bearing, not just typing UX: ComboBox re-publishes its + // still-empty Text when ItemsSource initializes before the Text binding has delivered a + // value. If the setter ever "helpfully" fell back to a default instead, every Options + // page load would silently reset the font size. var (page, settings) = CreatePage(); - var original = settings.SelectedFontSize; - try - { - var notified = new List(); - page.PropertyChanged += (_, e) => notified.Add(e.PropertyName); - - page.SelectedFontSizePoints = "14"; - - notified.Should().NotContain(nameof(DisplaySettingsViewModel.SelectedFontSizePoints)); - } - finally - { - settings.SelectedFontSize = original; - } + settings.SelectedFontSize = 16; + + page.SelectedFontSizePoints = "abc"; + settings.SelectedFontSize.Should().Be(16, + "transient garbage while typing must not move the stored size"); + + page.SelectedFontSizePoints = ""; + settings.SelectedFontSize.Should().Be(16, + "the empty Text published during ComboBox ItemsSource initialization must not clobber the setting"); } [AvaloniaTest] - public void NonNumeric_Input_Is_Ignored() + public void NaN_Input_Is_Ignored() { + // double.TryParse accepts the culture's NaN symbol and NaN falls through Math.Clamp. + // Persisted, it would fail DecompilerTextEditor's SelectedFontSize > 0 guard on every + // run, permanently disabling font-size application until the settings file is repaired. var (page, settings) = CreatePage(); - var original = settings.SelectedFontSize; - try - { - settings.SelectedFontSize = 16; - page.SelectedFontSizePoints = "abc"; - settings.SelectedFontSize.Should().Be(16, - "transient garbage while typing must not move the stored size"); - } - finally - { - settings.SelectedFontSize = original; - } + settings.SelectedFontSize = 16; + + page.SelectedFontSizePoints = "NaN"; + + settings.SelectedFontSize.Should().Be(16); + double.IsFinite(settings.SelectedFontSize).Should().BeTrue(); } [AvaloniaTest] public void Typed_Sizes_Are_Clamped_To_The_6_To_72_Point_Range() { var (page, settings) = CreatePage(); - var original = settings.SelectedFontSize; - try - { - page.SelectedFontSizePoints = "1"; - settings.SelectedFontSize.Should().BeApproximately(6.0 * 4 / 3, 1e-9); - - page.SelectedFontSizePoints = "500"; - settings.SelectedFontSize.Should().BeApproximately(72.0 * 4 / 3, 1e-9); - } - finally - { - settings.SelectedFontSize = original; - } + page.SelectedFontSizePoints = "1"; + settings.SelectedFontSize.Should().BeApproximately(6.0 * 4 / 3, 1e-9); + + page.SelectedFontSizePoints = "500"; + settings.SelectedFontSize.Should().BeApproximately(72.0 * 4 / 3, 1e-9); } [AvaloniaTest] - public void Size_List_Offers_6_Through_24_Points_Like_The_WPF_Host() + public void Size_List_Offers_6_Through_24_Points() { var (page, _) = CreatePage(); page.FontSizes.Should().Equal(Enumerable.Range(6, 24 - 6 + 1)); } + static async Task OpenDisplayPanelSizeBoxAsync(MainWindow window) + { + AppComposition.Current.GetExport() + .GetCommand(nameof(Resources._Options)).Execute(null); + var vm = (MainWindowViewModel)window.DataContext!; + var model = (OptionsPageModel)vm.DockWorkspace.Documents!.VisibleDockables! + .OfType().First(t => t.Content is OptionsPageModel).Content!; + model.SelectedPage = model.Pages.OfType().Single(); + TestCapture.Step("display-page-selected"); + + var panel = await window.WaitForComponent(); + var box = panel.FindControl("fontSizeComboBox"); + ((object?)box).Should().NotBeNull("the size box must be the named editable ComboBox"); + Dispatcher.UIThread.RunJobs(); + return box!; + } + [AvaloniaTest] public async Task Display_Panel_Size_Box_Is_An_Editable_ComboBox_Showing_Points() { var window = AppComposition.Current.GetExport(); window.Show(); - var settings = AppComposition.Current.GetExport(); - var original = settings.DisplaySettings.SelectedFontSize; - try - { - settings.DisplaySettings.SelectedFontSize = 10.0 * 4 / 3; - - AppComposition.Current.GetExport() - .GetCommand(nameof(Resources._Options)).Execute(null); - var vm = (MainWindowViewModel)window.DataContext!; - var model = (OptionsPageModel)vm.DockWorkspace.Documents!.VisibleDockables! - .OfType().First(t => t.Content is OptionsPageModel).Content!; - model.SelectedPage = model.Pages.OfType().Single(); - TestCapture.Step("display-page-selected"); - - var panel = await window.WaitForComponent(); - var box = panel.FindControl("fontSizeComboBox"); - ((object?)box).Should().NotBeNull("the size box must be the named editable ComboBox"); - Dispatcher.UIThread.RunJobs(); - - box!.IsEditable.Should().BeTrue("custom sizes must be typeable, like Notepad's font page"); - box.Text.Should().Be("10", "the box shows points, not device-independent pixels"); - } - finally - { - settings.DisplaySettings.SelectedFontSize = original; - } + + var box = await OpenDisplayPanelSizeBoxAsync(window); + + box.IsEditable.Should().BeTrue("custom sizes must be typeable, like Notepad's font page"); + box.Text.Should().Be("10", "the box shows points, not device-independent pixels"); + // IsEditable=true is only real if the applied ControlTheme materialized the editable + // text box part; assert the template realized it rather than trusting the property. + box.ApplyTemplate(); + Dispatcher.UIThread.RunJobs(); + box.GetVisualDescendants().OfType() + .Should().Contain(t => t.Name == "PART_EditableTextBox", + "the theme must realize the editable text box, or IsEditable is a no-op"); + } + + [AvaloniaTest] + public async Task Clamped_Value_Is_Written_Back_Into_The_Box_On_Focus_Loss() + { + // Typing "3" stores the clamped 6 pt, but the echo suppression leaves the box showing + // "3". Focus loss must resync the text from the stored value, like the NumericUpDown + // this ComboBox replaced did via CommitInput on LostFocus. + var window = AppComposition.Current.GetExport(); + window.Show(); + var settings = AppComposition.Current.GetExport().DisplaySettings; + + var box = await OpenDisplayPanelSizeBoxAsync(window); + + // A real focus change, not a synthetic RaiseEvent: LostFocus is typed + // (FocusChangedEventArgs), so hand-built RoutedEventArgs blow up any typed + // subscriber on the route - and a genuine focus move is what users do anyway. + // The editable ComboBox delegates focus to its template's text box, so focus that + // (ComboBox.Focus() itself returns false when IsEditable). + box.ApplyTemplate(); + Dispatcher.UIThread.RunJobs(); + var sizeEditor = box.GetVisualDescendants().OfType() + .First(t => t.Name == "PART_EditableTextBox"); + sizeEditor.Focus().Should().BeTrue("headless focus must land in the size box"); + Dispatcher.UIThread.RunJobs(); + + box.Text = "3"; + Dispatcher.UIThread.RunJobs(); + settings.SelectedFontSize.Should().BeApproximately(6.0 * 4 / 3, 1e-9, + "precondition: the typed 3 is stored clamped to 6 pt"); + TestCapture.Step("undersized-value-typed"); + + var elsewhere = box.FindAncestorOfType()! + .GetVisualDescendants().OfType().First(); + elsewhere.Focus().Should().BeTrue("headless focus must be able to leave the size box"); + Dispatcher.UIThread.RunJobs(); + TestCapture.Step("focus-left-size-box"); + + box.Text.Should().Be("6", "focus loss must replace the rejected text with the clamped value"); } } diff --git a/ILSpy/Options/DisplaySettingsPanel.axaml b/ILSpy/Options/DisplaySettingsPanel.axaml index 2b18e2b6c..ecf99f963 100644 --- a/ILSpy/Options/DisplaySettingsPanel.axaml +++ b/ILSpy/Options/DisplaySettingsPanel.axaml @@ -33,7 +33,8 @@ + Text="{Binding SelectedFontSizePoints, Mode=TwoWay}" + LostFocus="FontSizeBox_LostFocus" />