Skip to content

MarkupExtension use interface instead of abstract class #306

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

MarkupExtension use interface instead of abstract class #306

wants to merge 3 commits into from

Conversation

h82258652
Copy link

@walterlv
Copy link
Contributor

That's a great improvement that we can make an existed class used like a MarkupExtension.

@Allatu
Copy link

Allatu commented Jan 31, 2019

Why u didn't use MarkupExtension now in all Classes? I think you need to check for IMarkupExtension everywhere. @h82258652

@vatsan-madhavan vatsan-madhavan added Enhancement Requested Product code improvement that does NOT require public API changes/additions * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) API suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 31, 2019
@vatsan-madhavan
Copy link
Member

This is a design change along with the addition of a public type. I would recommend opening an issue for this and discussing the design, so that we could ensure that this is the approach we want to take. It will also allow interested folks to chime in with their input.

Marking this as NO MERGE for now, until we have consensus on the approach.

@vatsan-madhavan vatsan-madhavan added this to the Future milestone Jan 31, 2019
dotMorten
dotMorten reviewed Feb 4, 2019
View reviewed changes
Copy link
Contributor

@dotMorten dotMorten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my $0.02. I think to make this non-breaking you need a new class in-between, and that's a lot of API to add that would need full design/review of the product owners. It's probably good to discuss the design and get buy-off in the issue first.

...icrosoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/XamlSetMarkupExtensionEventArgs.cs
public IServiceProvider ServiceProvider { get; private set; }

internal XamlSetMarkupExtensionEventArgs(XamlMember member,
MarkupExtension value, IServiceProvider serviceProvider, object targetObject)
IMarkupExtension value, IServiceProvider serviceProvider, object targetObject)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change

...icrosoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/XamlSetMarkupExtensionEventArgs.cs
base(member, value)
{
ServiceProvider = serviceProvider;
}

public MarkupExtension MarkupExtension { get { return Value as MarkupExtension; } }
public IMarkupExtension MarkupExtension { get { return Value as IMarkupExtension; } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change.
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).

...icrosoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/XamlSetMarkupExtensionEventArgs.cs
@@ -9,17 +9,17 @@ namespace System.Windows.Markup
public class XamlSetMarkupExtensionEventArgs : XamlSetValueEventArgs
{
public XamlSetMarkupExtensionEventArgs(XamlMember member,
MarkupExtension value, IServiceProvider serviceProvider) :
IMarkupExtension value, IServiceProvider serviceProvider) :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change

@grubioe grubioe added the PR metadata: Label to tag PRs, to facilitate with triage label May 28, 2019
Base automatically changed from master to main March 17, 2021 17:38
@ryalanms ryalanms requested a review from a team as a code owner March 17, 2021 17:38
@dipeshmsft dipeshmsft added API Review Requested and removed API suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 2, 2023
@ghost ghost assigned h82258652 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotMorten dotMorten dotMorten left review comments

@walterlv walterlv walterlv left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@h82258652 h82258652

Labels
API Review Requested Enhancement Requested Product code improvement that does NOT require public API changes/additions * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Milestone
Future
Development

Successfully merging this pull request may close these issues.
7 participants
@h82258652 @walterlv @Allatu @vatsan-madhavan @dotMorten @grubioe @dipeshmsft