Repository navigation
Refactor to allow better modularization of the translator. - #337
Conversation
Previously most of the translation code had to be in the LottieToWinCompTranslator class and that was making it difficult to maintain. The goal with this refactor is to allow translation code to be implemented in static methods grouped for convenience into static classes, and to avoid having to pass more than one context to a translation method. The state that is needed for translation is now in various "contexts". The LottieToWinCompTranslator is now just a static class that creates the top-level context and tells it to translate. Previously we had a "TranslationContext" that was the context for translating a layer, and a "ShapeContext" that was the context for translating a shape withing a shape layer. Now we have: TranslationContext - context for stuff that is global to a particular Lottie file's translation. CompositionContext - context for a list of layers. There is a CompositionContext for the root of the Lottie as well as one for each PreComp layer. LayerContext - context for stuff that is layer dependent. There are strongly-typed subclasses of this for each layer type. ShapeContext - as before - context for translating a shape. These contexts form a hierarchy: ShapeContext contains a LayerContext which contains a CompositionContext which contains a TranslationContext. Contexts inherit state from their containing context. Through the magic of implicit conversions, a context can always be passed to a method requiring a context type higher in the hierarchy, e.g. a LayerContext can be passed to a method that requires a TranslationContext.
| /// <summary> | ||
| /// Static methods for animating Windows Composition objects. | ||
| /// </summary> | ||
| static class Animate |
There was a problem hiding this comment.
Animate [](start = 17, length = 7)
Nit: could this be renamed "Animations"? That would match the naming convention you use for the other files in this directory a bit better. #Closed
There was a problem hiding this comment.
This was done to make the callsites easier to read. For most cases you end up with code that looks something like this:
Animate.Vector2(context, startPoint, result, nameof(result.StartPoint));
... so as you're reading it the naming make it clear what the intention is with out making the name really long.
It doesn't work for the "Start*" methods though. I'll focus on renaming those so they fit the pattern better.
In reply to: 476545075 [](ancestors = 476545075)
|
|
||
| namespace Microsoft.Toolkit.Uwp.UI.Lottie.LottieToWinComp | ||
| { | ||
| static class ExtensionMethods |
There was a problem hiding this comment.
ExtensionMethods [](start = 17, length = 16)
It's not totally clear what "ExtensionMethods" are. Based on the the methods here, maybe Descriptions or ObjectDescriptions would be a better name? Add a summary comment regardless. #Closed
There was a problem hiding this comment.
It's a style used by most extension methods in C#. We don't care so much about the name of the class because they're called like instance methods (e.g. myCompObject.SetName(...)), and the name of the class make it obvious that there is extension method magic in here.
Seeing as it bugged you, maybe I can call it IDescribableExtensionMethods and make everyone happy.
In reply to: 476546424 [](ancestors = 476546424)
| <Compile Include="$(MSBuildThisFileDirectory)ThemePropertyBindings.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)Transforms.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)TranslationContext.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)Rectangles.cs" /> |
There was a problem hiding this comment.
Rectangles [](start = 49, length = 10)
Alphabetize (same for ConvertTo below). #Closed
|
|
||
| namespace Microsoft.Toolkit.Uwp.UI.Lottie.LottieToWinComp | ||
| { | ||
| static class Shapes |
There was a problem hiding this comment.
Shapes [](start = 17, length = 6)
Add summary comment? #Closed
Previously most of the translation code had to be in the LottieToWinCompTranslator class and that was making it difficult to maintain.
The goal with this refactor is to allow translation code to be implemented in static methods grouped for convenience into static classes, and to avoid having to pass more than one context to a translation method.
The state that is needed for translation is now in various "contexts". The LottieToWinCompTranslator is now just a static class that creates the top-level context and tells it to translate.
Previously we had a "TranslationContext" that was the context for translating a layer, and a "ShapeContext" that was the context for translating a shape withing a shape layer.
Now we have:
TranslationContext - context for stuff that is global to a particular Lottie file's translation.
CompositionContext - context for a list of layers. There is a CompositionContext for the root of the Lottie as well as one for each PreComp layer.
LayerContext - context for stuff that is layer dependent. There are strongly-typed subclasses of this for each layer type.
ShapeContext - as before - context for translating a shape.
These contexts form a hierarchy: ShapeContext contains a LayerContext which contains a CompositionContext which contains a TranslationContext.
Contexts inherit state from their containing context.
Through the magic of implicit conversions, a context can always be passed to a method requiring a context type higher in the hierarchy, e.g. a LayerContext can be passed to a method that requires a TranslationContext.