pimhwang
/
ILSpy
mirror of https://github.com/icsharpcode/ILSpy.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

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
pull/4002/head
Christoph Wille 1 month ago
parent
03e5e5ece2
commit
487630e8de
4 changed files with 161 additions and 114 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 237
      ILSpy.Tests/Options/DisplayFontSizeTests.cs
  2. 3
      ILSpy/Options/DisplaySettingsPanel.axaml
  3. 5
      ILSpy/Options/DisplaySettingsPanel.axaml.cs
  4. 30
      ILSpy/Options/DisplaySettingsViewModel.cs

237
ILSpy.Tests/Options/DisplayFontSizeTests.cs
Unescape View File

@ -23,6 +23,7 @@ using System.Threading.Tasks; @@ -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; @@ -40,10 +41,12 @@ using NUnit.Framework;
namespace ICSharpCode.ILSpy.Tests;
/// <summary>
/// The options dialog edits the font size in points (like the Windows font dialogs and the
/// WPF host's FontSizeConverter), while <see cref="DisplaySettings.SelectedFontSize"/> keeps
/// The options dialog edits the font size in points (the unit Windows font dialogs use),
/// while <see cref="DisplaySettings.SelectedFontSize"/> keeps
/// storing device-independent pixels so persisted settings round-trip with ILSpy 9.x.
/// The pt/px conversion lives in <see cref="DisplaySettingsViewModel.SelectedFontSizePoints"/>.
/// 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.
/// </summary>
[TestFixture]
public class DisplayFontSizeTests
@ -59,36 +62,22 @@ 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 @@ -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<string?>();
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<string?>();
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 @@ -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<string?>();
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<string?>();
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<ComboBox> OpenDisplayPanelSizeBoxAsync(MainWindow window)
{
AppComposition.Current.GetExport<MainMenuCommandRegistry>()
.GetCommand(nameof(Resources._Options)).Execute(null);
var vm = (MainWindowViewModel)window.DataContext!;
var model = (OptionsPageModel)vm.DockWorkspace.Documents!.VisibleDockables!
.OfType<ContentTabPage>().First(t => t.Content is OptionsPageModel).Content!;
model.SelectedPage = model.Pages.OfType<DisplaySettingsViewModel>().Single();
TestCapture.Step("display-page-selected");
var panel = await window.WaitForComponent<DisplaySettingsPanel>();
var box = panel.FindControl<ComboBox>("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<MainWindow>();
window.Show();
var settings = AppComposition.Current.GetExport<SettingsService>();
var original = settings.DisplaySettings.SelectedFontSize;
try
{
settings.DisplaySettings.SelectedFontSize = 10.0 * 4 / 3;
AppComposition.Current.GetExport<MainMenuCommandRegistry>()
.GetCommand(nameof(Resources._Options)).Execute(null);
var vm = (MainWindowViewModel)window.DataContext!;
var model = (OptionsPageModel)vm.DockWorkspace.Documents!.VisibleDockables!
.OfType<ContentTabPage>().First(t => t.Content is OptionsPageModel).Content!;
model.SelectedPage = model.Pages.OfType<DisplaySettingsViewModel>().Single();
TestCapture.Step("display-page-selected");
var panel = await window.WaitForComponent<DisplaySettingsPanel>();
var box = panel.FindControl<ComboBox>("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<TextBox>()
.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<MainWindow>();
window.Show();
var settings = AppComposition.Current.GetExport<SettingsService>().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<TextBox>()
.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<DisplaySettingsPanel>()!
.GetVisualDescendants().OfType<CheckBox>().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");
}
}

3
ILSpy/Options/DisplaySettingsPanel.axaml
Unescape View File

@ -33,7 +33,8 @@ @@ -33,7 +33,8 @@
<ComboBox Grid.Row="0" Grid.Column="3" Width="100" x:Name="fontSizeComboBox"
IsEditable="True"
ItemsSource="{Binding FontSizes}"
Text="{Binding SelectedFontSizePoints, Mode=TwoWay}" />
Text="{Binding SelectedFontSizePoints, Mode=TwoWay}"
LostFocus="FontSizeBox_LostFocus" />
<Border Grid.Row="1" Grid.Column="1" Grid.ColumnSpan="3"
BorderBrush="{DynamicResource ILSpy.ChromeBorder}" BorderThickness="1" Padding="6" Margin="0,2,0,0">
<!-- FontFamily binds to a derived FontFamily property because Avalonia's

5
ILSpy/Options/DisplaySettingsPanel.axaml.cs
Unescape View File

@ -29,5 +29,10 @@ namespace ICSharpCode.ILSpy.Options.Panels @@ -29,5 +29,10 @@ namespace ICSharpCode.ILSpy.Options.Panels
}
void InitializeComponent() => AvaloniaXamlLoader.Load(this);
// Commit-on-focus-loss for the size box: rewrites the text from the stored (clamped)
// value, so e.g. a typed "3" doesn't keep showing while 6 pt is what got stored.
void FontSizeBox_LostFocus(object? sender, global::Avalonia.Interactivity.RoutedEventArgs e)
=> (DataContext as DisplaySettingsViewModel)?.CommitFontSizeText();
}
}

30
ILSpy/Options/DisplaySettingsViewModel.cs
Unescape View File

@ -57,23 +57,35 @@ namespace ICSharpCode.ILSpy.Options @@ -57,23 +57,35 @@ namespace ICSharpCode.ILSpy.Options
.OrderBy(n => n, System.StringComparer.OrdinalIgnoreCase)
.ToArray();
/// <summary>Point sizes offered in the size dropdown; same list as the WPF host.</summary>
/// <summary>Point sizes offered in the size dropdown.</summary>
public IReadOnlyList<int> FontSizes { get; } = Enumerable.Range(6, 24 - 6 + 1).ToArray();
/// <summary>
/// The font size as the user sees and edits it: points, like the Windows font dialogs.
/// <see cref="DisplaySettings.SelectedFontSize"/> itself stays in device-independent
/// pixels (1 pt = 4/3 px) so persisted settings round-trip with the WPF host; the
/// conversion happens only at this dialog boundary. Non-numeric input is ignored
/// (it is usually a transient typing state), numeric input is clamped to 6-72 pt.
/// The font size as the user sees and edits it: points, converted at 1 pt = 4/3 logical
/// pixels. That ratio is exact wherever Avalonia's logical unit is 1/96 inch (Windows
/// DIPs, X11 via Xft.dpi). On macOS the logical unit is a Cocoa point, so the number
/// shown here is not the size native Mac font dialogs would report for the same
/// rendering - accepted, because
/// <see cref="DisplaySettings.SelectedFontSize"/> staying in logical pixels with one
/// fixed conversion is what lets settings files round-trip with ILSpy 9.x on every host.
/// Unparsable and non-finite input is ignored, numeric input is clamped to 6-72 pt.
/// </summary>
public string SelectedFontSizePoints {
get => Settings == null
? string.Empty
: Math.Round(Settings.SelectedFontSize * 3 / 4).ToString(CultureInfo.CurrentCulture);
set {
// Ignoring unparsable input is load-bearing beyond typing UX: ComboBox re-publishes
// its still-empty Text when ItemsSource initializes before the Text binding has
// delivered a value, and that write must not clobber the stored size. Do not
// "improve" this into a fall-back-to-default.
if (Settings == null || !double.TryParse(value, NumberStyles.Float, CultureInfo.CurrentCulture, out double points))
return;
// TryParse accepts the culture's NaN symbol, and NaN falls straight through
// Math.Clamp; persisted, it would fail the editor's SelectedFontSize > 0 guard
// forever, so the editor would never apply a font size again.
if (!double.IsFinite(points))
return;
points = Math.Clamp(points, 6, 72);
// Suppress the PropertyChanged echo for this property: the binding would
// immediately rewrite the size box with the rounded value mid-keystroke.
@ -91,6 +103,12 @@ namespace ICSharpCode.ILSpy.Options @@ -91,6 +103,12 @@ namespace ICSharpCode.ILSpy.Options
bool updatingFontSizeFromText;
/// <summary>Re-publishes the size text from the stored value. Wired to the size box's
/// LostFocus: the echo suppression above means a typed "3" stored as the clamped 6 pt
/// would otherwise keep showing 3 - committing on focus loss resyncs the box, the same
/// way the NumericUpDown this ComboBox replaced committed its input.</summary>
public void CommitFontSizeText() => OnPropertyChanged(nameof(SelectedFontSizePoints));
/// <summary>
/// Derived FontFamily for the preview TextBlock. Avalonia's runtime binding pipeline
/// doesn't auto-coerce a <c>string</c> source to <see cref="FontFamily"/> (the implicit

Write Preview
Loading…
Cancel
Save