Minor API tweaks to the Animations package

[Sergio0694]

Following a conversation with @michael-hawker on Discord

PR Type

What kind of change does this PR introduce?

• Public API tweaks

Overview

This PR makes a couple of minor changes to the public API surface of Microsoft.Toolkit.Uwp.UI.Animations:

• Renamed AnimationSet.Ended to Completed. This is consistent with Timeline.Completed and CompositionScopedBatch.Completed.
• Removed the AnimationSet.INode interface. This was used originally to enforce compile time type safety to nodes being added to a set, but since now we're forced to inherit from DependencyObjectCollection anyway, it means we would no longer have this ability anyway, as that allows consumers to append objects of any type. Instead, I've removed the interface to simplify the public API surface, and added more helpful error messages to the exception being throw in case users pass in an invalid object.

Open questions

I'm thinking we might want to also rename AnimationStartBehavior and AnimationEndBehavior to AnimationStartedBehavior and AnimationCompletedBehavior? Thoughts? 🤔

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

[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 🙌

[michael-hawker]

Looks good to me, I like your idea about the behavior name too.

[marcelwgn]

I think we should also rename the AnimationBehaviors as you suggested to have consistent API naming. Having a Completed event raised by an AnimationEndBehavior is a bit confusing (how do "end" and "complete" differ?). Maybe we should call the end behavior "AnimationCompletionBehavior"?

[marcelwgn]

Maybe we should update the documentation here to "Raised whenever the current animation completes." to match the API wording.

[Sergio0694]

Right, makes perfect sense. Done in 9d4cdce, thanks! 😄

[Sergio0694]

Maybe we should call the end behavior "AnimationCompletionBehavior"?

I'm wondering keeping the same event name might be simpler and more intuitive to use?
As in, to have the general pattern "[SUBJECT][EVENT]Behavior".
Eg. consider cases where you might have different inconsistent naming otherwise, like how ItemClickBehavior would remain the same because "click" can be both a noun and a verb, but AnimationCompletion would be different than the event Completed.
With this pattern instead there would never be any potential confusion on the associated behavior for any event on any type - you'd already know its name would always just be "Behavior" next to whatever event name the behavior is reacting to.
Thoughts? 🙂

[marcelwgn]

I see that "Completion" and "Completed" is not the same and that this would be different to "ItemClickBehavior" where "Click" is also the event name. However I don't think that there will be an event called "Completion". To me "AnimationCompletionBehavior" sounds better than "AnimationCompletedBehavior" but both are fine to me, most importantly, it shouldn't be "AnimationEndBehavior" (which naming scheme wise is the same as "AnimationCompletionBehavior" since the event was named "Ended" not "End").

[michael-hawker]

Yeah, agree with @chingucoding that we shouldn't use 'End'. I think sticking with the names of the Events we're hooking into makes things clear to map and to understand, so I think that'd lead us to AnimationCompletedBehavior, eh?

[michael-hawker]

Actually, just thinking this is really a *TriggerBehavior like the EventTriggerBehavior and DataTriggerBehavior as they allow you to go spawn off *Actions.

So we probably want this to be AnimationCompletedTriggerBehavior and AnimationStartedTriggerBehavior.

Not sure if there's other things we've added in the Behaviors that don't follow this pattern... like thinking of my InvokeActionsActivity but that's a bit different...

[michael-hawker]

LGTM

[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]

@chingucoding if it looks good to you too - just approve, and we'll get someone to merge. 🙂 FYI @Kyaa-dost.

[marcelwgn]

Sure thing @michael-hawker, looks good to me!