Improved MarqueeText code quality

components/MarqueeText/src/MarqueeText.Events.cs

@@ -41,14 +41,16 @@ private void MarqueeText_Unloaded(object sender, RoutedEventArgs e)
 
     private void Container_SizeChanged(object sender, SizeChangedEventArgs e)
     {
-        if (_marqueeContainer is not null)
+        if (_marqueeContainer is null)
         {
-            // Clip the marquee within its bounds
-            _marqueeContainer.Clip = new RectangleGeometry
-            {
-                Rect = new Rect(0, 0, e.NewSize.Width, e.NewSize.Height)
-            };
+            return;
         }
+
+        // Clip the marquee within its bounds
+        _marqueeContainer.Clip = new RectangleGeometry
+        {
+            Rect = new Rect(0, 0, e.NewSize.Width, e.NewSize.Height)
+        };
 
         // The marquee should run when the size changes in case the text gets cutoff
         StartMarquee();

components/MarqueeText/src/MarqueeText.cs

@@ -2,7 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-
 namespace CommunityToolkit.Labs.WinUI.MarqueeTextRns;
 
 /// <summary>
@@ -68,6 +67,7 @@ protected override void OnApplyTemplate()
     {
         base.OnApplyTemplate();
 
+        // Explicit casting throws early when parts are missing from the template
         _marqueeContainer = (Panel)GetTemplateChild(MarqueeContainerPartName);
         _segment1 = (FrameworkElement)GetTemplateChild(Segment1PartName);
         _segment2 = (FrameworkElement)GetTemplateChild(Segment2PartName);
@@ -112,6 +112,7 @@ private static string GetVisualStateName(MarqueeBehavior behavior)
     /// <summary>
     /// Begins the Marquee animation if not running.
     /// </summary>
+    /// <exception cref="InvalidOperationException">Thrown when template parts are not supplied.</exception>
     public void StartMarquee()
     {
         bool initial = _isActive;
@@ -128,18 +129,20 @@ public void StartMarquee()
     /// <summary>
     /// Stops the Marquee animation.
     /// </summary>
+    /// <exception cref="InvalidOperationException">Thrown when template parts are not supplied.</exception>
     public void StopMarquee()
     {
         StopMarquee(_isActive);
     }
 
-    private void StopMarquee(bool stopping)
+    private void StopMarquee(bool initialState)
     {
+        // Set _isActive and update the animation to match
         _isActive = false;
         bool playing = UpdateAnimation(false);
 
         // Invoke MarqueeStopped if Marquee is not playing and was before
-        if (!playing && stopping)
+        if (!playing && initialState)
         {
             MarqueeStopped?.Invoke(this, EventArgs.Empty);
         }
@@ -149,9 +152,13 @@ private void StopMarquee(bool stopping)
     /// Updates the animation to match the current control state.
     /// </summary>
     /// <param name="resume">True if animation should resume from its current position, false if it should restart.</param>
-    /// <returns>True if the Animation is now playing</returns>
+    /// <exception cref="InvalidOperationException">Thrown when template parts are not supplied.</exception>
+    /// <returns>True if the Animation is now playing.</returns>
     private bool UpdateAnimation(bool resume = true)
     {
+        // Crucial template parts are missing!
+        // This can happen during initialization of certain properties.
+        // Gracefully return when this happens. Proper checks for these template parts happen in OnApplyTemplate.
         if (_marqueeContainer is null ||
             _marqueeTransform is null ||
             _segment1 is null ||
@@ -160,55 +167,68 @@ _segment1 is null ||
             return false;
         }
 
+        // The marquee is stopped.
+        // Update the animation to the stopped position.
         if (!_isActive)
         {
             VisualStateManager.GoToState(this, MarqueeStoppedState, false);
 
             return false;
         }
 
-        // Get the size (width horizontal, height if vertical) of the
-        // contain and segment.
-        // Also track the property to adjust based on the orientation.
+        // Get the size of the container and segment, based on the orientation.
+        // Also track the property to adjust, also based on the orientation.
         double containerSize;
         double segmentSize;
         double value;
-        string property;
+        string targetProperty;
 
         if (IsDirectionHorizontal)
         {
+            // The direction is horizontal, so the sizes, value, and properties
+            // are defined by width and X coordinates.
             containerSize = _marqueeContainer.ActualWidth;
             segmentSize = _segment1.ActualWidth;
             value = _marqueeTransform.X;
-            property = "(TranslateTransform.X)";
+            targetProperty = "(TranslateTransform.X)";
         }
         else
         {
+            // The direction is vertical, so the sizes, value, and properties
+            // are defined by height and Y coordinates.
             containerSize = _marqueeContainer.ActualHeight;
             segmentSize = _segment1.ActualHeight;
             value = _marqueeTransform.Y;
-            property = "(TranslateTransform.Y)";
+            targetProperty = "(TranslateTransform.Y)";
         }
 
         if (IsLooping && segmentSize < containerSize)
         {
-            // If the text segment is smaller than the area provided,
-            // it does not need to run in looping mode.
+            // If the marquee is in looping mode and the segment is smaller
+            // than the container, then the animation does not not need to play.
+
+            // NOTE: Use resume as initial because _isActive is updated before
+            // calling update animation. If _isActive were passed, it would allow for
+            // MarqueeStopped to be invoked when the marquee was already stopped.
             StopMarquee(resume);
             _segment2.Visibility = Visibility.Collapsed;
+
             return false;
         }
 
-        // The start position is offset 100% if ticker
+        // The start position is offset 100% if in ticker mode
         // Otherwise it's 0
         double start = IsTicker ? containerSize : 0;
-        // The end is when the end of the text reaches the border if bounding
+
+        // The end is when the end of the text reaches the border if in bouncing mode
         // Otherwise it is when the first set of text is 100% out of view
         double end = IsBouncing ? containerSize - segmentSize : -segmentSize;
 
-        // The distance is used for calculating the duration and the progress if resuming
+        // The distance is used for calculating the duration and the previous
+        // animation progress if resuming
         double distance = Math.Abs(start - end);
 
+        // If the distance is zero, don't play an animation
         if (distance is 0)
         {
             return false;
@@ -220,27 +240,51 @@ _segment1 is null ||
             (start, end) = (end, start);
         }
 
-        // The second segment of text should be hidden if the marquee is not in looping mode.
+        // The second segment of text should be hidden if the marquee is not in looping mode
         _segment2.Visibility = IsLooping ? Visibility.Visible : Visibility.Collapsed;
 
+        // Calculate the animation duration by dividing the distance by the speed
         TimeSpan duration = TimeSpan.FromSeconds(distance / Speed);
 
+        // Unbind events from the old storyboard
         if (_marqueeStoryboard is not null)
         {
             _marqueeStoryboard.Completed -= StoryBoard_Completed;
         }
 
-        _marqueeStoryboard = new Storyboard
+        // Create new storyboard and animation
+        _marqueeStoryboard = CreateMarqueeStoryboardAnimation(start, end, duration, targetProperty);
+
+        // Bind the storyboard completed event
+        _marqueeStoryboard.Completed += StoryBoard_Completed;
+
+        // Set the visual state to active and begin the animation
+        VisualStateManager.GoToState(this, MarqueeActiveState, true);
+        _marqueeStoryboard.Begin();
+
+        // If resuming, seek the animation so the text resumes from its current position.
+        if (resume)
+        {
+            double progress = Math.Abs(start - value) / distance;
+            _marqueeStoryboard.Seek(TimeSpan.FromTicks((long)(duration.Ticks * progress)));
+        }
+
+        return true;
+    }
+
+    private Storyboard CreateMarqueeStoryboardAnimation(double start, double end, TimeSpan duration, string targetProperty)
+    {   
+        // Initialize the new storyboard
+        var marqueeStoryboard = new Storyboard
         {
             Duration = duration,
             RepeatBehavior = RepeatBehavior,
 #if !HAS_UNO
             AutoReverse = IsBouncing,
 #endif
         };
-
-        _marqueeStoryboard.Completed += StoryBoard_Completed;
-
+
+        // Create a new double animation, moving from [start] to [end] positions in [duration] time.
         var animation = new DoubleAnimationUsingKeyFrames
         {
             Duration = duration,
@@ -249,6 +293,8 @@ _segment1 is null ||
             AutoReverse = IsBouncing,
 #endif
         };
+
+        // Create the key frames
         var frame1 = new DiscreteDoubleKeyFrame
         {
             KeyTime = KeyTime.FromTimeSpan(TimeSpan.Zero),
@@ -260,23 +306,18 @@ _segment1 is null ||
             Value = end,
         };
 
+        // Add the key frames to the animation
         animation.KeyFrames.Add(frame1);
         animation.KeyFrames.Add(frame2);
-        _marqueeStoryboard.Children.Add(animation);
-        Storyboard.SetTarget(animation, _marqueeTransform);
-        Storyboard.SetTargetProperty(animation, property);
-
-        VisualStateManager.GoToState(this, MarqueeActiveState, true);
-        _marqueeStoryboard.Begin();
 
-        if (resume)
-        {
-            // Seek the animation so the text is in the same position.
-            double progress = Math.Abs(start - value) / distance;
-            _marqueeStoryboard.Seek(TimeSpan.FromTicks((long)(duration.Ticks * progress)));
-        }
+        // Add the double animation to the storyboard
+        marqueeStoryboard.Children.Add(animation);
+
+        // Set the storyboard target and target property
+        Storyboard.SetTarget(animation, _marqueeTransform);
+        Storyboard.SetTargetProperty(animation, targetProperty);
 
-        return true;
+        return marqueeStoryboard;
     }
 }