Grid Shorthand Syntax Implementation #6
Conversation
|
|
||
| namespace System.Windows.Controls | ||
| { | ||
| internal sealed class ColumnDefinitionCollectionConverter : TypeConverter |
There was a problem hiding this comment.
I wonder if it makes sense to have create base type converter for code shared between Rows and Columns
There was a problem hiding this comment.
They don't really have any properties in common, the code would have to be split anyway even if it was a single converter. And I would rather have two standalone converters than one relying on type descriptor to figure out what the converter should produce.
There was a problem hiding this comment.
I agree with @miloush here, there is very less commonality between the two properties and we would need to bifurcate the code to handle the properties.
|
So you seem to have decided to create a new collection on set but copy the same instances of definitions to it. Why is that necessary compared to just using the collection directly? Edit: I see. the collection now has two identities, one with owner which does all the magic like before and one without which works just like an ordinary collection of definitions. You technically keep the magic one internal and the public one trivial. This takes some time to figure out and while it is clever, I don't find it to be a very good design. |
| _data ??= new ExtendedData(); | ||
| _data.RowDefinitions ??= new RowDefinitionCollection(this); | ||
| _data.RowDefinitions.Clear(); | ||
| if (value == null) |
| } | ||
| foreach (RowDefinition rowDef in value) | ||
| { | ||
| if (rowDef.Parent != null) |
There was a problem hiding this comment.
this throws when the definition is already the correct owner/parent
e.g. grid.ColumnDefinitions = grid.ColumnDefinitions would throw and shouldn't
There was a problem hiding this comment.
Ah except you clear everything first and then readd it.
| { | ||
| _data ??= new ExtendedData(); | ||
| _data.RowDefinitions ??= new RowDefinitionCollection(this); | ||
| _data.RowDefinitions.Clear(); |
There was a problem hiding this comment.
Do you need to remove the definitions from the parent?
There was a problem hiding this comment.
Nevermind, Clear() does that internally.
| @@ -5116,10 +5116,10 @@ public partial class Grid : System.Windows.Controls.Panel, System.Windows.Markup | |||
| public static readonly System.Windows.DependencyProperty ShowGridLinesProperty; | |||
| public Grid() { } | |||
| [System.ComponentModel.DesignerSerializationVisibilityAttribute(System.ComponentModel.DesignerSerializationVisibility.Content)] | |||
| public System.Windows.Controls.ColumnDefinitionCollection ColumnDefinitions { get { throw null; } } | |||
| public System.Windows.Controls.ColumnDefinitionCollection ColumnDefinitions { get { throw null; } set { } } | |||
|
As I commented on one of the other approaches, these converters will happily convert rows/columns that have other properties set and lose the values. This might be undesirable. I wonder whether we should just not support converting to string at all. |
|
So if I understand correctly, this will not work as expected: var collection = grid2.ColumnDefinitions = grid1.ColumnDefinitions;
collection.Add(def);@etvorun I don't see how in any model where setter manages ownership this could work, any ideas? |
|
Also moving in this direction, I don't see a reason why the collections couldn't have a public parameterless constructor. |
src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/ColumnDefinition.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/ColumnDefinition.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/Grid.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/Grid.cs
Outdated
Show resolved
Hide resolved
| @@ -272,6 +272,7 @@ public bool ShowGridLines | |||
| /// <summary> | |||
| /// Returns a ColumnDefinitionCollection of column definitions. | |||
| /// </summary> | |||
| [TypeConverter(typeof(ColumnDefinitionCollectionConverter))] | |||
There was a problem hiding this comment.
Any reason why type converter is applied to ColumnDefinitions property rather than class ColumnDefinitionCollection?
There was a problem hiding this comment.
I would find that a higher risk, as it would affect uses outside of the Grid (and would get returned by TypeDescriptor). The main reason I would be reluctant to do so is because the converter does not really support converting the whole collection. It supports only quite a narrow albeit common case where properties like MinWidth, MaxWidth, Offset or SharedSizeGroup are not set.
There was a problem hiding this comment.
There is also a discussion that for that reason, the converter might only support conversion from string, not to string.
(And type converter applied to a property was an important distinguishing factor for the XAML parser to allow manipulating collections using attribute, though with public setter we have departed from that requirement.)
src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/Grid.cs
Outdated
Show resolved
Hide resolved
I would expect an exception to be thrown in this case. Similar to border1.Child = border2.Child |
|
At that time the setter was creating a copy of the given collection, so it wouldn't have thrown. I believe the PR has now evolved to use the collection directly and throw as you expect. |
…initionCollection 2, Adding CanConvertTo method in RowDefinitionCollectionConverter and ColumnDefinitionCollectionConverter to resolve serialization error.
No description provided.