Introduce BehaviorBase and let FadeHeaderBehavior derive from this base #1010

GeertvanHorrik · 2017-03-06T19:34:24Z

dnfclas · 2017-03-06T19:34:28Z

@GeertvanHorrik,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

GeertvanHorrik · 2017-03-06T19:35:49Z

I've created the BehaviorBase and tested it with FadeHeaderBehavior. It works in the example and on my app that uses collapsed visibility by default.

Please review the code. If it looks ok, I'll fix the other HeaderBehavior classes as well.

dnfclas · 2017-03-06T19:45:54Z

@GeertvanHorrik, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

…ehaviorBase

GeertvanHorrik · 2017-03-06T19:49:52Z

It was so little work that I've done the other 2 behaviors as well. Ready for review & merge if all is good.

lukasf

I think I have found a few issues, please check my remarks...

lukasf · 2017-03-11T13:03:38Z

Microsoft.Toolkit.Uwp.UI.Animations/Behaviors/BehaviorBase.cs

+
+        private void HandleAttach()
+        {
+            if (_isAttached || _isAttached)


I guess this is supposed to be _isAttaching || _isAttached?

Good point, will fix.

lukasf · 2017-03-11T13:04:25Z

Microsoft.Toolkit.Uwp.UI.Animations/Behaviors/FadeHeaderBehavior.cs

            // Confirm that Windows.UI.Xaml.Hosting.ElementCompositionPreview is available (Windows 10 10586 or later).
            if (!ApiInformation.IsMethodPresent("Windows.UI.Xaml.Hosting.ElementCompositionPreview", nameof(ElementCompositionPreview.GetScrollViewerManipulationPropertySet)))
            {
-                return;


I think it would be better to return "true" here, otherwise the method will be called again and again on each SizeChanged.

Good point, will fix.

lukasf · 2017-03-11T13:05:02Z

Microsoft.Toolkit.Uwp.UI.Animations/Behaviors/QuickReturnHeaderBehavior.cs

            if (!ApiInformation.IsMethodPresent("Windows.UI.Xaml.Hosting.ElementCompositionPreview", nameof(ElementCompositionPreview.GetScrollViewerManipulationPropertySet)))
            {
-                return;
+                return false;


I think it would be better to return "true" here, otherwise the method will be called again and again on each SizeChanged.

lukasf · 2017-03-11T13:05:33Z

Microsoft.Toolkit.Uwp.UI.Animations/Behaviors/StickyHeaderBehavior.cs

            if (!ApiInformation.IsMethodPresent("Windows.UI.Xaml.Hosting.ElementCompositionPreview", nameof(ElementCompositionPreview.GetScrollViewerManipulationPropertySet)))
            {
-                return;
+                return false;


I think it would be better to return "true" here, otherwise the method will be called again and again on each SizeChanged.

- Don't allow retries of behavior subscription when the composition types are not present

GeertvanHorrik · 2017-03-11T13:57:20Z

@lukasf Thanks for the review, all should be fixed.

lukasf · 2017-03-12T19:56:14Z

@GeertvanHorrik Thanks, looks all good now!

deltakosh · 2017-03-13T16:09:20Z

Do you think we should change this file to inherit from your behaviorbase? cc @shenchauhan

shenchauhan · 2017-03-13T18:17:05Z

I don't think it should be added to the chain of derived types for Composition Behavior animations that we have. I think I need more convincing that this is the right approach for it. I need help in understanding how this is going to stop multiple instances? From what I can tell there is nothing stopping me from creating multiple instances. The BehaviourBase also takes type T which could be anything. However, the code looks for FrameworkElement when checking the AssociatedObject. Why not change T to FrameworkElement?

GeertvanHorrik · 2017-03-13T19:14:56Z

I kept that one intact because I didn't want this change to become too big or impact too many other behaviors.

It doesn't necessarily prevent you from creating multiple behavior instance (that should still be possible since it's a valid use-case). However, it does prevent the behavior from accidentally assign the animation multiple times (for example on the OnAttached method and the Loaded event). It also prevents you from calling the same code twice (which was happening in my code as soon as I called the HeaderElement dependency property).

I've added a type constraint T : UIElement to allow this behavior to be used even on UIElement class instances (it will just not support Loaded / Unloaded events). I don't mind changing this to FrameworkElement though.

lukasf · 2017-03-13T19:56:44Z

I agree with @shenchauhan that the type constraint should be FrameworkElement. The whole point of the new BehaviorBase is to auto retry the attachment on Loaded and on SizeChanged. Both are only supported on FrameworkElement, so using the BehaviorBase on UIElement will not give any benefits over Behavior.

But for all FrameworkElement targets, the base class makes a lot of sense. Probably all behaviors will need to wait at least for Loaded to get into action. So I am with @deltakosh that the CompositionBehaviorBase should be converted, then we have all Behaviors on the new base class. Plus, the type constraint for CompositionBehaviorBase should be changed to FrameworkElement as well. It does not work with UIElements, because it waits for Loaded to start the animation, so the type constraint was wrong to begin with.

GeertvanHorrik · 2017-03-13T20:00:45Z

@lukasf I don't agree. It also prevents a duplicate attach call while the behavior is already attaching. So when I call the HeaderElement property, it somehow re-invokes the attach method (and thus creates a double invocation). This behavior also takes care of that issue (hence the _isAttaching field).

lukasf · 2017-03-13T20:22:37Z

@GeertvanHorrik I am pretty sure that the Behavior base class makes sure that Attach is only called once, that is when the behaviors are assigned to the target Behaviors collection (and before the element is loaded into the UI tree). So my assumption is that the duplicate attach you are seeing is rather an internal issue of the BehaviorBase, where HandleAttach is called not only on Attach, but also on Loaded and SizeChanged. Probably either Loaded or SizeChaged is fired while you are attaching, so you get a second HandleAttach call. Have you checked in the debugger where the calls originate from?

GeertvanHorrik · 2017-03-13T20:26:47Z

@lukasf It happened before I introduced the BehaviorBase, that's why I created this in the first place. I can double check on the regular branch to see what was causing it (if I remember correctly, it was when calling the HeaderElement property), but I need to submit my app first now. I expect to have an answer in about 30 - 60 minutes.

GeertvanHorrik · 2017-03-13T20:46:44Z

GeertvanHorrik · 2017-03-13T20:48:08Z

As you can see, setting the HeaderElement inside the AssignFadeAnimation causes the PropertyChanged event to be invoked and it initiates a new AssignFadeAnimation call. However, this call is not required because we are still assigning the animation.

lukasf · 2017-03-14T20:15:13Z

Okay, so as I assumed, the second call does not come from Behavior.Attach but from a custom event handler. My point was that Behavior.Attach will never get called multiple times. It is the specific behavior implementation which causes this behavior. But I see now that a similar situation might occur with behaviors for UIElement, so using BehaviorBase might help even for UIElement.

As a side note, I am not sure if it is such a good idea to change the HeaderElement property on attach, as it is done in all Header Behaviors right now. HeaderElement is a public setter property, it might be data bound (only currently evaluating to zero). By assigning a new value, an existing binding assigned by user/dev would be removed/overridden. I'd think that controls usually should avoid changing public setter properties, unless it is required for the control to work (e.g. property is the "output" of the control). Would it perhaps be better to store the effectively used header (either from HeaderElement or implicit from ListViewBase) in a private field, just like scroll viewer?

deltakosh · 2017-03-14T20:15:50Z

Hello I would like to close on this one soon as we plan to code freeze on 3/21

lukasf · 2017-03-14T22:18:19Z

You can ignore my last comment then, it's not so important anyways. I'd still propose to also convert the last behavior to BehaviorBase, so we have them all on the same base class...

deltakosh · 2017-03-14T22:21:33Z

I agree

GeertvanHorrik · 2017-03-15T08:51:42Z

Just ping me if you want me to change the other behaviors as well.

deltakosh · 2017-03-15T17:54:02Z

Ping @shenchauhan who tought they must provide default implementation (even empty) instead of abstract method so that inheriting from it is straightforward

shenchauhan · 2017-03-16T16:48:11Z

@GeertvanHorrik I'm ok with adding it to the other behaviours for consistency purposes and as long as those abstract methods do something meaningful in the other behaviours too.

deltakosh · 2017-03-16T17:49:47Z

@GeertvanHorrik Do you mind to update all behaviors? I just would like to mark some abstract methods as virtual (and empty) so child classes won't be force to provide empty methods

GeertvanHorrik · 2017-03-16T18:05:57Z

I'll update all behaviors and make abstract methods virtual instead.

deltakosh · 2017-03-16T18:21:55Z

Lovely!

GeertvanHorrik · 2017-03-16T18:40:28Z

I've changed StartAsync to Start in some behaviors (since the calls are not being awaited anyway)
I've implemented the base CompositionBehaviorBase (and a non-generic one for backwards compatibility)
Whenever something was casted to a FrameworkElement, I have replaced the generic parameter to FrameworkElement so this casting and storing in a private field is no longer required.

GeertvanHorrik · 2017-03-16T18:44:36Z

I've noticed that Detach() was biting the IBehavior.Detach() method so have renamed these to Initialize and Uninitialize. If there are issues with this, ping me and I will change that.

GeertvanHorrik · 2017-03-16T18:49:08Z

Done, pushed the changes. Also fixed all warnings.

deltakosh · 2017-03-16T20:10:31Z

@shenchauhan do you mind reviewing the changes?

shenchauhan · 2017-03-20T21:56:30Z

reviewing now, just pulling PR. Should have an answer by EOD.

shenchauhan

Please try running the Sample app. It crashes for me when I try to view the animation behaviors. InitializeComponent fails. Failed to assign to property 'Microsoft.Toolkit.Uwp.UI.Animations.Behaviors.CompositionBehaviorBase`1<Windows.UI.Xaml.UIElement>.AutomaticallyStart'. [Line: 19 Position: 35]'

GeertvanHorrik · 2017-03-21T13:07:18Z

Investigating now.

GeertvanHorrik · 2017-03-21T13:10:15Z

I forgot the change the owner type when making the CompositionBehaviorBase generic. Fix incoming.

Introduce BehaviorBase and let FadeHeaderBehavior derive from this base

b71f75c

dnfclas added the cla-required label Mar 6, 2017

dnfclas added cla-signed and removed cla-required labels Mar 6, 2017

QuickReturnHeaderBehavior and StickyHeaderBehavior also derive from B…

9de574a

…ehaviorBase

Improve BehaviorBase::Detach method

df9a240

deltakosh approved these changes Mar 6, 2017

View reviewed changes

deltakosh added this to the v1.4 milestone Mar 8, 2017

deltakosh added the reviewers wanted label Mar 8, 2017

lukasf suggested changes Mar 11, 2017

View reviewed changes

- Fix usage of wrong field in BehaviorBase

fabc32e

- Don't allow retries of behavior subscription when the composition types are not present

deltakosh requested a review from shenchauhan March 13, 2017 22:12

All behaviors derive from BehaviorBase

a8de83f

shenchauhan requested changes Mar 20, 2017

View reviewed changes

Fix owner type of CompositionBehaviorBase

8a8ae01

shenchauhan approved these changes Mar 23, 2017

View reviewed changes

deltakosh merged commit e51557b into CommunityToolkit:dev Mar 23, 2017

Introduce BehaviorBase and let FadeHeaderBehavior derive from this base #1010

Introduce BehaviorBase and let FadeHeaderBehavior derive from this base #1010

Uh oh!

Conversation

GeertvanHorrik commented Mar 6, 2017

Uh oh!

dnfclas commented Mar 6, 2017

Uh oh!

GeertvanHorrik commented Mar 6, 2017

Uh oh!

dnfclas commented Mar 6, 2017

Uh oh!

GeertvanHorrik commented Mar 6, 2017

Uh oh!

lukasf left a comment

Choose a reason for hiding this comment

Uh oh!

lukasf Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

GeertvanHorrik Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

lukasf Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

GeertvanHorrik Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

lukasf Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

lukasf Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

GeertvanHorrik commented Mar 11, 2017

Uh oh!

lukasf commented Mar 12, 2017

Uh oh!

deltakosh commented Mar 13, 2017

Uh oh!

shenchauhan commented Mar 13, 2017

Uh oh!

GeertvanHorrik commented Mar 13, 2017

Uh oh!

lukasf commented Mar 13, 2017

Uh oh!

GeertvanHorrik commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukasf commented Mar 13, 2017

Uh oh!

GeertvanHorrik commented Mar 13, 2017

Uh oh!

GeertvanHorrik commented Mar 13, 2017

Uh oh!

GeertvanHorrik commented Mar 13, 2017

Uh oh!

lukasf commented Mar 14, 2017

Uh oh!

deltakosh commented Mar 14, 2017

Uh oh!

lukasf commented Mar 14, 2017

Uh oh!

deltakosh commented Mar 14, 2017

Uh oh!

GeertvanHorrik commented Mar 15, 2017

Uh oh!

deltakosh commented Mar 15, 2017

Uh oh!

shenchauhan commented Mar 16, 2017

Uh oh!

deltakosh commented Mar 16, 2017

Uh oh!

GeertvanHorrik commented Mar 16, 2017

Uh oh!

deltakosh commented Mar 16, 2017

Uh oh!

GeertvanHorrik commented Mar 16, 2017

Uh oh!

GeertvanHorrik commented Mar 13, 2017 •

edited

Loading