Skip to content
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

Changes of visibility to get a better adaptation for extensions. #37

Closed
wants to merge 1 commit into from

Conversation

MartinEgli
Copy link

@MartinEgli MartinEgli commented Jun 19, 2019

Description of Change

Triggers only support events with the structure (object, EventArgs). I also like to use events with the structure (object DependencyPropertyCahngedEventArgs). Currently this is not possible. I can also imagine other connections to delegate, IObservable, Func<> or Action<>. To support more types, it would help to change some methods from internal to public. So you can create your own EventTriggerBase classes.

Examples are in my branch Behaviors.Extensions

API Changes

Class Behavior
Changed:

  • internal event EventHandler AssociatedObjectChanged => public event EventHandler AssociatedObjectChanged

Class NameResolver

  • internal sealed class NameResolver => public sealed class NameResolver
  • internal sealed class NameResolvedEventArgs : EventArgs => public sealed class NameResolvedEventArgs : EventArgs

Class TriggerBase

  • internal TriggerBase(Type associatedObjectTypeConstraint) => public TriggerBase(Type associatedObjectTypeConstraint)

Behavioral Changes

Can create your own EventTriggerBase classes for other types of events in your own project.

@msftclas
Copy link

msftclas commented Jun 19, 2019

CLA assistant check
All CLA requirements met.

@MartinEgli
Copy link
Author

How can I trigger a review of my change?

@pedrolamas
Copy link
Collaborator

Triggers only support events with the structure (object, EventArgs)

I haven't checked this, but shouldn't you be able to just cast from EventArgs to the correct type?

@MartinEgli
Copy link
Author

MartinEgli commented Aug 11, 2019

This is exactly the problem. I want to make other structures like (object, DependencyPropertyChangedEvent), action or similar available. To do this, the desired changes had to be made to the base so that you could extend it yourself. I made an example of this in my fork.

@MartinEgli
Copy link
Author

Is there any chance that the changes will be accepted? I don't want to create my own Behaviors project.

@@ -47,7 +47,7 @@ public abstract class Behavior :
private Type associatedType;
private DependencyObject associatedObject;

internal event EventHandler AssociatedObjectChanged;
public event EventHandler AssociatedObjectChanged;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would you need this to be public? If you need to know when the associated object changed, you can just override the OnAttached and OnDettaching methods.

Copy link
Author

@MartinEgli MartinEgli Aug 15, 2019

Choose a reason for hiding this comment

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

An example of its use can be found in https://github.com/MartinEgli/XamlBehaviorsWpf/tree/Behaviors.Extensions/src/Behaviors.Extensions . The extension is in a separate project. A class similar to EventTriggerBase should be created that inherits from TriggerBase and the class should behave similarly. Access to internal elements is required to create a class DependencyPropertyChangedEventTriggerBase.

The class DependencyPropertyChangedEventTriggerBase inherits from TriggerBase. With the method OnAttached newBehavior.AssociatedObjectChanged += this.OnBehaviorHostChanged becomes identical to the class EventTriggerBase. To enable access to AssociatedObjectChanged the visibility must be changed to public. The same for behavior.AssociatedObjectChanged -= this.OnBehaviorHostChanged.

https://github.com/MartinEgli/XamlBehaviorsWpf/blob/Behaviors.Extensions/src/Behaviors.Extensions/DependencyPropertyChangedEventTriggerBase.cs#L259

https://github.com/MartinEgli/XamlBehaviorsWpf/blob/Behaviors.Extensions/src/Behaviors.Extensions/DependencyPropertyChangedEventTriggerBase.cs#L301

@@ -11,7 +11,7 @@ namespace Microsoft.Xaml.Behaviors
/// <summary>
/// Provides data about which objects were affected when resolving a name change.
/// </summary>
internal sealed class NameResolvedEventArgs : EventArgs
public sealed class NameResolvedEventArgs : EventArgs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still struggle to understand why you would need to make this public...

Copy link
Author

@MartinEgli MartinEgli Aug 15, 2019

Choose a reason for hiding this comment

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

An example of its use can be found in https://github.com/MartinEgli/XamlBehaviorsWpf/tree/Behaviors.Extensions/src/Behaviors.Extensions . The extension is in a separate project. A class similar to EventTriggerBase should be created that inherits from TriggerBase and the class should behave similarly. Access to internal elements is required to create a class DependencyPropertyChangedEventTriggerBase.

The class DependencyPropertyChangedEventTriggerBase also needs access to the NameResolvedEventArgs.

https://github.com/MartinEgli/XamlBehaviorsWpf/blob/Behaviors.Extensions/src/Behaviors.Extensions/DependencyPropertyChangedEventTriggerBase.cs#L416

@@ -37,7 +37,7 @@ public NameResolvedEventArgs(object oldObject, object newObject)
/// Helper class to handle the logic of resolving a TargetName into a Target element
/// based on the context provided by a host element.
/// </summary>
internal sealed class NameResolver
public sealed class NameResolver
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, why public?

Copy link
Author

@MartinEgli MartinEgli Aug 15, 2019

Choose a reason for hiding this comment

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

An example of its use can be found in https://github.com/MartinEgli/XamlBehaviorsWpf/tree/Behaviors.Extensions/src/Behaviors.Extensions . The extension is in a separate project. A class similar to EventTriggerBase should be created that inherits from TriggerBase and the class should behave similarly. Access to internal elements is required to create a class DependencyPropertyChangedEventTriggerBase.

The class DependencyPropertyChangedEventTriggerBase also needs access to the NameResolvers.

https://github.com/MartinEgli/XamlBehaviorsWpf/blob/Behaviors.Extensions/src/Behaviors.Extensions/DependencyPropertyChangedEventTriggerBase.cs#L55

https://github.com/MartinEgli/XamlBehaviorsWpf/blob/Behaviors.Extensions/src/Behaviors.Extensions/DependencyPropertyChangedEventTriggerBase.cs#L184

@pedrolamas
Copy link
Collaborator

@MartinEgli Not evaluating the merits on the PR itself, but given the scenario you mentioned, it seems easier to just use a PropertyChangedTrigger and monitor the IsEnabled property for changes instead of creating a new EventTrigger class.

@MartinEgli
Copy link
Author

MartinEgli commented Aug 15, 2019

You're right. The problem in the example could be solved differently. My example is only a use case of different kinds of events. The goal is to make delegations, actions, functions, IObservables available.

@brianlagunas
Copy link
Collaborator

I am closing this PR at this time, as this is not something we want to do. In the future, please open an issue to discuss your idea, before spending your valuable time putting together a PR. Thank you fort he time you put into this, even though it was not accepted. Your effort is appreciated.

@MartinEgli
Copy link
Author

MartinEgli commented Oct 10, 2019

Too bad that this is not implemented. The community of this project is very slow here. Do I really have to do a fork project for these little changes?
PS: Gitter chat of XamlBehaviors is also dead or not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants