dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.07k stars 1.17k forks source link

Make MarkupExtension an interface #170

Open Allatu opened 5 years ago

Allatu commented 5 years ago

I believe that making MarkupExtension an interface enables developers to work with (Static/Dynamic)Resources more flexibly. In my case it has hurt the creation of ResourceDictionaries with simple types like Double (e.g. font sizes or the like) or fonts in general.

The implementation of the class is already really close to that of an interface so it should not be too much work to make the change.

Example:

<TypeOfIMarkupExtension x:Key="GlobalFontSize" Value="{Binding BindingToMyClass}"/>

<TextBlock FontSize="{StaticResource GlobalFontSize}"/>
h82258652 commented 5 years ago

agree. MarkupExtension is not inherbit DependencyObject, use interface will better than abstract class.

dotMorten commented 5 years ago

I'm curious how you'd justify the breaking changes this would create? Aren't breaking API changes basically dead on arrival? Is there another way this could be accomplished without breaking anything? (otherwise I doubt this issue would get much support from the WPF team, and anyone else outside who relies on things not breaking on them).

From your PR it looks like the breaking change is isolated to XamlSetMarkupExtensionEventArgs.cs. Perhaps injecting a base class between this class and XamlSetValueEventArgs that adds the interface version, and leave XamlSetMarkupExtensionEventArgs alone (apart from now inheriting from the intermediate class)

Allatu commented 5 years ago

First, I thouth this is a new Implementation, and their is no offical Release yet. Then it is not a breaking change. Second I would just keep the MarkupExtension Class, but use everywhere the IMarkupExtension interface. @dotMorten Other ways to accomplish that, is to inherit MarkupExtension from DependencyObject, but I think that is the worst way.

stevenbrix commented 5 years ago

@YannikZ12, you are right that it isn't technically a breaking change, but we want to ensure that migration from .NET Framework to .NET Core is as easy as possible for everyone. In that regard, we are considering anything that would change the runtime behavior and/or compile time behavior between .NET Framework and .NET Core a breaking change. In theory, developers should be able to just retarget their applications for .NET Core and not need to change anything. I haven't fully thought about the implications of this change, are you sure that we can make that guarantee here?

Allatu commented 5 years ago

Hello @stevenbrix, my colleague @Dabbel created a Fork and made some Changes to ensure no Breaking change for any Developer.

https://github.com/Dabbel/wpf/commit/17798b374435d70e8dc416bcc5231c0f525b5477

He couldn't Test it because a missing .net Core 3.0-preview4 Version, maybe you can Test it and give feedback.

stevenbrix commented 5 years ago

He couldn't Test it because a missing .net Core 3.0-preview4 Version

Can you elaborate more on this? We'd love to make sure that everyone has the ability to test and validate changes themselves. I know our documentation is currently a bit lacking in this regard, but any information you can share about what does/doesn't work for you at the moment would really help!

Allatu commented 5 years ago

@stevenbrix Yeah at the offical .net Core 3.0 Page is only preview3 available, but in the last Version you referenced to Preview4 so he couldn't test it

Dabbel commented 5 years ago

Only .Net Core 3.0-preview3 can be download from https://dotnet.microsoft.com/download/dotnet-core/3.0

Severity Code Description Project File Line Suppression State Error NU1605 Detected package downgrade: Microsoft.NETCore.Platforms from 3.0.0-preview4.19181.2 to 3.0.0-preview3.19128.7. Reference the package directly from the project to select a different version. System.Xaml -> Microsoft.Private.Winforms 4.8.0-preview4.19205.5 -> Microsoft.NETCore.Platforms (>= 3.0.0-preview4.19181.2) System.Xaml -> Microsoft.NETCore.Platforms (>= 3.0.0-preview3.19128.7) System.Xaml Microsoft.DotNet.Wpf\src\System.Xaml\System.Xaml.csproj 1

stevenbrix commented 5 years ago

Ahh I see. If you download the daily build from https://github.com/dotnet/core-sdk are you then able to test?

Dabbel commented 5 years ago

With the daily build i can build the solution.