NumericUpDown control changes

[puppetsw]

What does the pull request do?

Attempts to issue #5318 fix by adding a property for decimal places to the control.

What is the current behavior?

When the NumericUpDown control is at a value of 5.6 and Increment is 0.01, if adjusted up, it will appear as 0.570000000000000001 instead of 0.57

What is the updated/expected behavior with this PR?

The value should be 0.57

How was the solution implemented (if it's not obvious)?

Changing the backing type for NumericUpDown to decimal breaks contracts.

Checklist

• Added unit tests (if possible)?
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/Documentation with user documentation

Breaking changes

Changing the backing type for NumericUpDown to decimal breaks contracts.

Obsoletions / Deprecations

CultureInfo property is obsolete @AvaloniaUI/core
Users should use NumberFormat instead.

Fixed issues

Fixes #5318

[maxkatz6]

Thanks for the contribution!
Am I right, that with this implementation if won't apply rounding for values that were manually inputed or came from binding? So, everything but increment/decrement.
I had quick look, and it seems that ConvertTextToValue method suits well to have rounding:

Avalonia/src/Avalonia.Controls/NumericUpDown/NumericUpDown.cs

Line 607 in 2956f79

[danwalmsley]

@maxkatz6 probably we have something in MathUtils class that can deal with this kind of stuff?

[maxkatz6]

@danwalmsley I don't see anything that can help us in https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Utilities/MathUtilities.cs

But I am not sure, do we need to Round here or Truncate?

[robloo]

Internally, the backing type should probably be decimal. Then Value is just a double conversion of the internal decimal for backwards compatiblity. A new DecimalValue should be added. The only reason this wasn't done in the NumberBox in WinUI is because C++ has no decimal type.

Note: Switching to decimal is especially important for currency input scenarios where precision is mandatory in the math. This shouldn't just be "hidden" using rounding techniques.

[puppetsw]

Thanks for the contribution!
Am I right, that with this implementation if won't apply rounding for values that were manually inputed or came from binding? So, everything but increment/decrement.
I had quick look, and it seems that ConvertTextToValue method suits well to have rounding:

Avalonia/src/Avalonia.Controls/NumericUpDown/NumericUpDown.cs

Line 607 in 2956f79

Yes, that was how I approached it. But it makes more sense to change the backing type to decimal considering currency input.

I'm happy to make the changes for decimal backing type. or Adding a new DecimalValue.

Sorry I'm new to this, should I close out this PR?

[robloo]

A few other comments:

• If it was me, I wouldn't close the PR. I would just revert commits and add changes as needed. It's good to keep all the discussion like this in one place.
• Generally, rounding should be handled by the number formatting itself. I would never add a DecimalPlaces property like some old controls might have. Instead, the ToString and parsing methods should be using a NumberFormatInfo that can be defined by the developer as a property. Otherwise, it should default to the current UI culture one. This is an oversimplification of a much larger discussion that was had over on the WinUI repo (still unresolved). Bottom line, Hex, Currency, Decimal, Integer, etc formats all need to be easily supported. This at first seems out of scope for this discussion, but is very much related to decimal place precision as it relates to overall formatting.

Edit

It seems NumericUpDown has most of the pieces for doing formatting correctly already: CultureInfo and NumberStyles are already properties. The issue is CultureInfo should instead be a NumberFormatInfo. Doing this means you can then customize the NumberFormatInfo controlling the decimal places: https://docs.microsoft.com/en-us/dotnet/api/system.globalization.numberformatinfo.numberdecimaldigits?view=net-5.0. It would be possible to add a new NumberFormat property and use that in code. Then keep CultureInfo for backwards compatiblity but use it to directly set the NumberFormat property.

The decimal backing type change plus adding the NumberFormat (type NumberFormatInfo) property would cover all bases here I think.

[maxkatz6]

@puppetsw technically switching from double to decimal is a breaking changes, that's why build is failing. You need to follow this tutorial to allow that https://stu.dev/check-for-breaking-changes-with-apicompat/ .

[puppetsw]

I've made type changes to the NumericUpDown class and event class. Added a NumberFormat property and have also set it through the CultureInfo to keep backwards compatibility. Marked CultureInfo as Obsolete.

I also added a new Clamp method to the MathUtilities.

[robloo]

Marked CultureInfo as Obsolete.

Core team doesn't really like anyone marking things as obsolete unless you run in past them first. @danwalmsley will need to buy in on this.

[pr8x]

What's wrong with System.Math.Clamp(Decimal, Decimal, Decimal) ? https://docs.microsoft.com/en-us/dotnet/api/system.math.clamp?view=net-5.0#System_Math_Clamp_System_Decimal_System_Decimal_System_Decimal_

[maxkatz6]

It's not available in netstandard 2.0

[grokys]

I've not really been following this PR - are we agreed that it's a good idea to break the API and change the values from double to decimal?

[maxkatz6]

@grokys personally I agree with that. Also it seems to be binary breaking change only, since both xaml and c# implicitly convert from decimal to double and in other way.

[grokys]

Ok, I'm fine with accepting your judgement on that ;) Looks good to me, but I'll leave the final review to you.

[maxkatz6]

Updated samples and ApiCompat. Thanks!