-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[X|C] only use a single GridLengthTypeConverter #29376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR clarifies which GridLengthTypeConverter to use by introducing a new public API in Microsoft.Maui.GridLength and updating converter references across multiple projects.
- Added new public API entries for Microsoft.Maui.GridLength.GridLengthTypeConverter in PublicAPI.Unshipped.txt files for different platforms.
- Updated GridLength conversion logic to support string inputs and revised related type converter references in RowDefinition and ColumnDefinition converters.
- Added a new unit test (Maui29334.xaml.cs) to cover the updated conversion behavior.
Reviewed Changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt | Added public API entries for the new GridLengthTypeConverter. |
| src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt | Added corresponding public API entries for Windows. |
| src/Core/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt | Added corresponding public API entries for Tizen. |
| src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt | Added corresponding public API entries for Mac Catalyst. |
| src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt | Added corresponding public API entries for iOS. |
| src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt | Added corresponding public API entries for Android. |
| src/Core/src/Primitives/GridLength.cs | Made GridLengthTypeConverter public and extended conversion logic to support strings. |
| src/Controls/tests/Xaml.UnitTests/Issues/Maui29334.xaml.cs | Added a unit test to verify the new conversion behavior. |
| src/Controls/src/Core/RowDefinitionCollectionTypeConverter.cs, ColumnDefinitionCollectionTypeConverter.cs, RowDefinition.cs, ColumnDefinition.cs | Updated TypeConverter references to use the fully qualified new GridLengthTypeConverter. |
| src/Controls/src/Core/GridLengthTypeConverter.cs | Marked the existing GridLengthTypeConverter as obsolete. |
| src/Controls/src/Build.Tasks/XamlCache.cs | Commented out an import reference for the old grid length converter. |
Files not reviewed (3)
- src/Controls/tests/Xaml.UnitTests/Issues/Maui29334.xaml: Language not supported
- src/Core/src/PublicAPI/net/PublicAPI.Unshipped.txt: Language not supported
- src/Core/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (1)
src/Core/src/Primitives/GridLength.cs:132
- The GridLengthTypeConverter now supports string conversion; please update or ensure that the XML docs and public API documentation explain the accepted string formats (e.g., 'auto', '', and numeric with optional '' suffix).
string strValue => strValue.Trim().ToLowerInvariant() switch
| @@ -1,5 +1,11 @@ | |||
| #nullable enable | |||
| Microsoft.Maui.ElementHandlerExtensions | |||
| Microsoft.Maui.GridLength.GridLengthTypeConverter | |||
Copilot
AI
May 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of the public API for GridLengthTypeConverter may affect downstream consumers; please confirm that the related public documentation in the /docs/ folders is updated accordingly to reflect the new API usage.
5330431 to
7e95b27
Compare
| { module.ImportReference(this, ("Microsoft.Maui", "Microsoft.Maui.Converters", "FlexAlignSelfTypeConverter")), typeof(EnumTypeConverter<Layouts.FlexAlignSelf>) }, | ||
| { module.ImportReference(this, ("Microsoft.Maui", "Microsoft.Maui.Converters", "FlexWrapTypeConverter")), typeof(EnumTypeConverter<Layouts.FlexWrap>) }, | ||
| { module.ImportReference(this, ("Microsoft.Maui", "Microsoft.Maui.Converters", "FlexBasisTypeConverter")), typeof(FlexBasisTypeConverter) }, | ||
| // { module.ImportReference(this, ("Microsoft.Maui", "Microsoft.Maui", "GridLength.GridLengthTypeConverter")), typeof(GridLengthTypeConverter) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be fixed (ImportReference should accept nested types) but of no importance as GridLengthTypeConverters are almost never use directly, but through collections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending changes in the foldable project:
C:\a\_work\1\s\src\Controls\Foldable\src\TwoPaneView.cs(165,47): error CS0618: 'GridLengthTypeConverter' is obsolete: 'Microsoft.Maui.Controls.GridLengthTypeConverter is obsolete. Use Microsoft.Maui.GridLengthConverter instead.' [C:\a\_work\1\s\src\Controls\Foldable\src\Controls.Foldable.csproj::TargetFramework=net9.0-ios18.0]
14 Error(s)
| public static bool operator !=(GridLength left, GridLength right) => !(left == right); | ||
|
|
||
| private sealed class GridLengthTypeConverter : TypeConverter | ||
| public sealed class GridLengthTypeConverter : TypeConverter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as this is becoming public, I wonder if it should level up in the Converters namespace instead of staying a nested type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a good idea!
|
Are we adding new api in a Service release? Should this target net10 ? |
|
@rmarinho the added api is harmless, but this obsolete a type. so if we decide to merge this in a -sr, the deprecation should be delayed to net10 |
| public static bool operator !=(GridLength left, GridLength right) => !(left == right); | ||
|
|
||
| private sealed class GridLengthTypeConverter : TypeConverter | ||
| public sealed class GridLengthTypeConverter : TypeConverter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a good idea!
| /// Gets the calculated width (in wide mode) or height (in tall mode) of pane 1, or sets the GridLength value of pane 1. | ||
| /// </summary> | ||
| [System.ComponentModel.TypeConverter(typeof(GridLengthTypeConverter))] | ||
| [System.ComponentModel.TypeConverter(typeof(GridLength.GridLengthTypeConverter))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes will be something that users will have to do? What if we were to keep the controls one and inherit from the Core one? Or something. Looking at the code it seems that no one is using it in Core?
Is there a way we can avoid API changes? #apiregret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about promoting the nested class to a normal class. that will limit the amount of change users might have to do.
also it is more aligned with other converters.
let's do that
e6f7536 to
502a26d
Compare
|
I have a rebase to net10 ready if we choose to defer this change |
502a26d to
57538eb
Compare
57538eb to
fde81ae
Compare
Description of Change
when we have multiple GridLengthTypeConverter in the codebase, it can be confusing to know which one to use.
Issues Fixed
Fixes #29334