Feature/implicit animation binding

[Sergio0694]

Closes #3842

PR Type

What kind of change does this PR introduce?

• Feature

What is the current behavior?

Updating properties of implicit animations through bindings doesn't actually change the animations.

What is the new behavior?

Implicit animations now listen to real-time changes in their properties.

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

Open questions (hence draft status)

• Do we also want to listen for changes in individual properties in each key frame?
• Do we have to inherit from DependencyObjectCollection for the ImplicitAnimationSet class in order for XAML bindings/properties to work correctly, or is implementing the same public APIs enough to make the XAML compiler/runtime happy? If I could do that, I could fix the isssue with removed items not being unsubscribed (nice to have).

UPDATE on 2): yes, we have to inherit from DependencyObjectCollection or old bindings break down, sadly.

[ghost]

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[Sergio0694]

Switching this to ready for review, here's more info on the status of this PR 😄

I've added more XML docs to ImplicitAnimationSet in 8d6815b, documenting all the new features introduced in this PR as well as some remarks with respect to unsupported scenarios. We can expand on the supported features and possibly enable more scenarios in the future, but for now this PR adds support for real-time changes for all the main animation properties through both bindings and x:Bind, which should unblock developers in most cases 🎉

I've put together a small sample app to validate the changes in this PR, here's the code in case anyone wants to give it a try too:

XAML

<Page
    x:Class="ImplicitAnimationsBindingTest.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:ImplicitAnimationsBindingTest"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    xmlns:animations="using:Microsoft.Toolkit.Uwp.UI.Animations"
    mc:Ignorable="d">
    <Page.DataContext>
        <local:MainPageViewModel x:Name="ViewModel"/>
    </Page.DataContext>

    <Grid RowSpacing="20" Margin="40">
        <Grid.RowDefinitions>
            <RowDefinition Height="Auto"/>
            <RowDefinition Height="Auto"/>
            <RowDefinition Height="*"/>
        </Grid.RowDefinitions>
        <TextBox PlaceholderText="From" Text="{x:Bind ViewModel.From, Mode=TwoWay}"/>
        <TextBox Grid.Row="1" PlaceholderText="To" Text="{x:Bind ViewModel.To, Mode=TwoWay}"/>
        <Rectangle Grid.Row="2" x:Name="MyRectangle" Height="200" Width="200" Fill="Green" HorizontalAlignment="Center" VerticalAlignment="Center">
            <animations:Implicit.ShowAnimations>
                <animations:TranslationAnimation From="{Binding From, Mode=OneWay}" To="0" Duration="0:0:0.5"/>
            </animations:Implicit.ShowAnimations>
            <animations:Implicit.HideAnimations>
                <animations:TranslationAnimation From="0" To="{Binding To, Mode=OneWay}" Duration="0:0:0.5"/>
            </animations:Implicit.HideAnimations>
        </Rectangle>
        <Button Grid.Row="2" Content="CLICK ME" Click="Button_Click" VerticalAlignment="Bottom" HorizontalAlignment="Left"/>
    </Grid>
</Page>

C#

using Microsoft.Toolkit.Mvvm.ComponentModel;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;

namespace ImplicitAnimationsBindingTest
{
    public sealed partial class MainPage : Page
    {
        public MainPage()
        {
            this.InitializeComponent();
        }

        private void Button_Click(object sender, RoutedEventArgs e)
        {
            if (MyRectangle.Visibility == Visibility.Visible)
            {
                MyRectangle.Visibility = Visibility.Collapsed;
            }
            else
            {
                MyRectangle.Visibility = Visibility.Visible;
            }
        }
    }

    public class MainPageViewModel : ObservableObject
    {
        private string from = "0";

        public string From
        {
            get => from;
            set => SetProperty(ref from, value);
        }

        private string to = "0";

        public string To
        {
            get => to;
            set => SetProperty(ref to, value);
        }
    }
}

To validate the changes, simply set some new values in either of the two TextBox controls and then show/hide the rectangle again. You'll see the implicit show/hide animations being updated in real time and the new animation being played.

Added to the 7.0.1 milestone! 🚀

[michael-hawker]

Hmm... Is this only if Binding is used or for everything now? As in 6.1.1 (and like we enabled for the Explicit ones) you could define these as resources and apply...

[Sergio0694]

Nope, it wasn't supported in 7.0 either. The reason is that AnimationSet can either contain a parent reference (if you assing one directly to an item), or no parent references at all (if it's in a resource dictionary), but in that case in order to start the animation you need to explicitly pass the target UIElement to animate (which our behaviors do automatically). An implicit set instead has to have a parent reference so that it can use it to recreate the animations whenever it needs to be updated (which was already happening if you removed/added an animation from a set). If it was shared in a resource dictionary then it wouldn't be able to do that. To be clear, due to how the code is currently setup, you can technically share an implicit set in a resource dictionary, but then real-time updates would only work for the last element that the set was applied to. In theory if you never make changes to a set in a dictionary, then it should be fine 🤔

[michael-hawker]

@Sergio0694 I was talking about 6.1.1 allows for Implicit animations to be resourced, but you would never have bindings in those scenarios, they're just static.

So I think you're saying it should still allow for that and be fine?

[Sergio0694]

Yup, should still work just fine 🙂

Made some changes in 7105101 to make this clearer - basically sharing an implicit set will work, but only the last element the set was attached to will actually receive real-time changes. As in, if you share a set and then use a binding to modify the animations, all elements except the last one the animation set was attached to will just keep using the initial values of the animations. If you don't use bindings instead but just hardcode the animations as before, then you wouldn't notice anything weird going on.

[azchohfi]

Do we accept/support anything other than ImplicitAnimation in this collection? I beliebe we do, so we should do a safe cast here.

[Sergio0694]

We don't we only support implicit animations here that are defined through the IImplicitTimeline interface. In practice we also technically allow users to just add whatever DependencyObject too since we're forced to use DependencyObjectCollection, which doesn't let us restrict the type of contained items, but we document not to do that so I think a runtime crash in that case would be acceptable. Though now that you made me think about this, and also to reply to your other question below, what we could do would be to remove the internal interface and just add the event to the public IImplicitTimeline interface.

Pro: users writing custom impliciti animations could support real-time updates as well
Cons: it's a breaking change (adding a member to a public interface)

Otherwise I could just make this a safe cast and just bail if it's not that interface, and we could document that if consumers write their own custom implicit animations, real-time updates will no longer be supported. This would avoid a breaking change, but it's also less limited (and possibly less intuitive for users?). I personally think taking a breaking change here could be acceptable as I don't think anyone would've added custom animations already after less than 2 weeks since the release? 🤔

Thoughts?

[azchohfi]

I'm ok with this "breaking change" of adding the member to the public interface, but we shouldn't do this on a non-major release, and unfortunately it is already public. @michael-hawker what do you think? Should we keep the new interface and just make this a safe cast?

[michael-hawker]

Is there a reason we couldn't add the new event to the IImplicitTimeline interface itself? Then we get both the pro and the con, eh? ImplicitAnimation is the only implementer, right?

I'm fine with either approach, even though 7.0.1 is a patch, we're still trying to fix things that we discovered in 7.0.0. Like I'll delist 7.0.0 once we ship 7.0.1.

[Sergio0694]

We could, the thing is that IImplicitTimeline is a public interface which is supported for custom animations in a set, and adding members to public interfaces is a breaking change. As in, if someone is using 7.0 and had a custom animation they defined, with a new member in that interface updating to 7.0.1 would be breaking for them, as they'd have to add that event first to be able to compile.

So it's really just a matter of whether it's fine to take this minor breaking change now. Reasonably speaking, I'd be surprised if anyone had already defined custom animations this past week since 7.0 was released. Also technically fixing that would just be a matter of adding that empty event, so there's that too 🙂

[Sergio0694]

Update: I made the breaking change in 4bc18c7 👍

[michael-hawker]

@azchohfi we good here now with Sergio's responses?

[ghost]

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[michael-hawker]

@msftbot merge once @azchohfi approves.

[ghost]

Hello @michael-hawker!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I'll only merge this pull request if it's approved by @azchohfi

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".