search

microsoft / XamlBehaviorsWpf

Home for WPF XAML Behaviors on GitHub.
MIT License
840 stars 138 forks source link

`TriggerBase<T>` doesn't implement `CloneCurrentValueCore`, so `Clone()` is shallow. #146

Closed Nintynuts closed 8 months ago

Nintynuts commented 9 months ago

Describe the bug When using an attached property to add behaviours or triggers to a style (because the style instance can't be shared) the Clone() method on Freezable is documented to indicate it's a deep copy, however when doing using it Actions is empty. This is because Clone's common logic only copies non-read-only dependency properties (which excludes Actions since it's a collection).

It's probable that other read-only collection properties on other triggers and behaviours also need this fix.

It would be nice if the attached properties to copy triggers and behaviours from a style were included in this package.

To Reproduce Steps to reproduce the behavior:

  1. Create or install a package including an attached property to apply triggers from a style (for example)
  2. Add a trigger to a style using this attached property, with an action
  3. Execute the trigger
  4. The actions will not happen, because they weren't cloned

Expected behavior The actions should be cloned and executed when triggered.

Desktop:

brianlagunas commented 8 months ago

I am not seeing how this is an issue with the WPF Behaviors, but rather how the WPF framework works in general. Thee are no mechanisms in this project that handles the copying of actions/triggers. What is your suggested change in order to support this?

Nintynuts commented 8 months ago

The mechanism to copy the behaviours/actions/triggers etc is Freezable.Clone which TriggerBase<T> inherits. I think this can be fixed by implementing CloneCurrentValueCore on the TriggerBase<T> class, which is there to be overridden and isn't. You're right that this project alone doesn't support copying them in a useful way, but to make them work with styles there are lots of projects (including the one I referenced) which implement an attached property to copy them onto instances, and it doesn't work because the implementation doesn't fulfil the Clone contract properly.

Ideally this project would include a mechansim to add behaviours and triggers to styles, but that's a step beyond what I'm asking and would require this fixing first to support it.

Nintynuts commented 8 months ago

Also please re-open, this isn't resolved. I'm not sure why you don't see it, but it definitely is an issue with WPF Behaviours.

brianlagunas commented 8 months ago

This isn't a bug. It's a feature request. The use-case needs to be more clearly defined or demonstrated. A focused sample would help with this. Based on my current understanding of this request, I'm not confident this is something that would be added.

Nintynuts commented 8 months ago

The classes implemented by this project don't fulfil the contracts they inherit. Seems pretty simple to me that it's wrong and should be fixed.

I thought I provided a pretty straight forward use-case, but to elaborate: you want to add triggers/behaviours to a style (because it's used by an ItemsControl or similar) and to do that you have to use an attached property mechanism that many people have implemented (like the one I referenced), which makes a deep copy of the trigger/behaviour and adds it to the instance the style is applied to using .Clone() (inherited from Freezable), which says it does a deep copy, and in this case doesn't.

Adding the attached properties to this project to be able to do that using just this project is a feature request, but that wasn't the main point of this issue.