-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Define Value property of Setter as XAML content property #745
Define Value property of Setter as XAML content property #745
Conversation
@thomasclaudiushuber I'd just like to warn you that this PR might not make it in until after September, as the acceptance criteria state that behavior-changing PRs such as this one will not be considered until .NET Core 3.0 is out the door. Nevertheless, good work! |
@wjk it's not changing behavior though is it? (Unless you consider something that used to give an exception no longer does) |
Thank you! I asked about this on StackOverflow back in 2009, just over 10 years ago today. |
it'll change a public api surface/contract; require update to docs, examples; likely needs updates to samples; definitely needs additions to tests. i can see reasonable framing of this change as (just) a bug-fix. but... i'm going to split the difference and call it a feature-ish bug-fix ;-). it's simple enough that i have a hard time not hitting the merge button... marking it as future + no-merge (for now). |
I'm sure we'll all forgive you if you accidentally clicked the wrong button... :-) #JustDoIt |
@wjk Thanks for the warning. But I think it's not a behavior-changing PR. It's a PR with a hint for the XAML processor, which doesn't change the existing behavior. It just brings in the possibility to omit the property element in XAML. @vatsan-madhavan Except adding the attribute to the class in the docs, there's no need to update docs and samples, because using the property element <Setter.Value> is still a valid option as before. The only difference with this PR is that this property element is now optional in XAML. And just because it is optional doesn't mean it's wrong using it. @dotMorten :-D Yes! Minor correction in your quote:
It's more than ok for me to wait till September, because it might be a good idea to stay consistent across all the PRs to not accept behavioral changes. Like @dotMorten I think this is not a behavioral change, that's why I created this PR (and also to start a little discussion like we have it now). But it seems we don't agree all if this is a behavioral change or not, which means not merging now is totally understandable and acceptable (Not merging is always acceptable! It's the free choice of the repo owner). And September is not too far away. @drewnoakes waited 10 years and 18 days for this, I guess 3-4 additional months are acceptable. On the other side it's such a simple win, and I don't see any side effects. |
Love the change, at some point. And when the change is allowed, can both markup compiling and xaml tooling/designers (VS, more?) handle the difference without additional work? P.S. what are the terms people have been using to distinguish those two flavors of (I said: netcoreapp wpf and netfx wpf, but that is my TFM centric view of things now) |
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.
Nice, @thomasclaudiushuber! I've been wanting to submit a PR for this for months, but you reacted faster than me when the Setter class became available in the repo 😉
@@ -24,6 +24,7 @@ namespace System.Windows | |||
/// </summary> | |||
[XamlSetMarkupExtensionAttribute("ReceiveMarkupExtension")] | |||
[XamlSetTypeConverterAttribute("ReceiveTypeConverter")] | |||
[ContentProperty("Value")] |
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.
Nitpicking: you might want to use nameof
here.
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.
Yes, thought about using nameof, but used on purpose a string for now, as it's like this in all other places I saw. I don't know if it's a good idea. But I guess we should be able to use a Roslyn analyzer later to get rid of all hardcoded strings and use for example nameof on every ContentPropertyAttribute in the whole WPF source code.
Curious about @rrelyea's comment here:
If that's the case, wouldn't that be the final nail in the coffin for any future enhancements for WPF on .NET Core? (considering .NET Framework is now in maintenance mode) |
WinForms gave up compatibility in favor of cleaning up the framework, so if WPF doesn't do it something is really going wrong. If anything you should consider compatibility between WinUI and WPF since that is the path forward (and has been acknowledged in some of the issues in WPF as well). I think that comment is probably just outdated now, maybe he was referring to 3.0 or 3.1 release. |
Any updates on this? 😊 |
I do not think the compatibility with NetFX argument is valid. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Thank you @thomasclaudiushuber for the suggestion and PR. We are sorry for the long delay. |
Thank you @anjali-wpf , fantastic news! |
Thank you @thomasclaudiushuber for your contribution |
Fixes issue #84