🏆 Live Soccer TV: CommunityToolkit/Lottie Windows/pull/

simeoncran · 2020-07-23T15:05:45Z

This is a rewrite of the Scrubber to support markers. It also is a much nicer implementation of the
scrubber that reuses more of the XAML Slider functionality, and reduces the amount of copied XAML from
the default Slider style (from generic.xaml).

As I was adding this change, it became apparent that the way the XAML slider draws its track would
not work well with markers at 0.0 or 1.0 because the track ends up extending before and after the
0.0 and 1.0 positions which would make the markers look like they were incorrectly placed. So now
we do all of the drawing (track, decrease rectangle, and thumb) using Composition.

We also now mimic the mouse-over behavior of Slider so that the control feels a little more engaging.
This required hooking Composition into the VisualStateManager, which is achieved by replacing the
default VisualStateManager with our own implementation.

This is a rewrite of the Scrubber to support markers. It also is a much nicer implementation of the scrubber that reuses more of the XAML Slider functionality, and reduces the amount of copied XAML from the default Slider style (from generic.xaml). As I was adding this change, it became apparent that the way the XAML slider draws its track would not work well with markers at 0.0 or 1.0 because the track ends up extending before and after the 0.0 and 1.0 positions which would make the markers look like they were incorrectly placed. So now we do all of the drawing (track, decrease rectangle, and thumb) using Composition. We also now mimic the mouse-over behavior of Slider so that the control feels a little more engaging. This required hooking Composition into the VisualStateManager, which is achieved by replacing the default VisualStateManager with our own implementation.

raymond-wh-leung · 2020-07-30T17:13:16Z

+                                Foreground="{ThemeResource SliderHeaderForeground}"
+                                Margin="{ThemeResource SliderTopHeaderMargin}"
+                                TextWrapping="Wrap"
+                                Visibility="Collapsed"


Visibility="Collapsed" [](start = 30, length = 24)

For the elements that are not visible (or with opacity 0), they need to remain in the template because the slider control will reference them? #Closed

Yes. I've removed some that I was confident about. I removed some others and got bitten by crashes when they were referenced by the Slider. There are probably more that can be removed but I don't feel confident about them. We're already cheaper than a normal slider as a result of removing some of the things from the template.

In reply to: 463148127 [](ancestors = 463148127)

raymond-wh-leung · 2020-07-30T17:17:20Z

+                                        Grid.Row="2"
+                                        Grid.ColumnSpan="3" />
+
+                                <!-- The Width of the Thumb has been modified to make it round. -->


modified to make it round. [](start = 69, length = 26)

Is it needed coz looks like the thumb is transparent? #Closed

The thumb is receiving the input. It just isn't visible. So yes, it's needed.
Also, some components in the template need to be there even if they're not visible because the Slider will try to reference them.

In reply to: 463150427 [](ancestors = 463150427)

raymond-wh-leung · 2020-07-30T17:18:54Z

+            // Update the size of the progress rectangle and the position of the thumb.
+            _properties.InsertScalar("Width", (float)sliderWidth);
+
+            // Update the positio of the markers.


positio [](start = 26, length = 7)

position #Closed

We need to be able to handle TrimmedAnimatables that are initialized with a default value.

raymond-wh-leung

derobinsMSFT · 2020-07-30T22:07:01Z

+                                        Opacity="0"
+                                        Style="{StaticResource SliderThumbStyle}"
+                                        DataContext="{TemplateBinding Value}"
+                                        Height="20"


Height="20" [](start = 40, length = 11)

Is there a default ThemeResource height you can use for this? Or, is this a modified height to keep the thumb round (along with the width)? #Closed

It is supposed to be the same shape as the slider we draw, which is not theme aware.

In reply to: 463298421 [](ancestors = 463298421)

derobinsMSFT · 2020-07-30T22:27:16Z

+                        markers: Colors.White,
+                        thumbFill: GetResourceBrushColor("LottieBasicBrush"),
+                        thumbStroke: GetResourceBrushColor("LottieBasicBrush"),
+                        track: GetResourceBrushColor("SliderTrackFillPointerOver"),


"SliderTrackFillPointerOver" [](start = 53, length = 28)

"SliderTrackFillPressed" for case "Pressed"? #Closed

I don't know how to press the slider, and I'm fine with it not changing colors. This makes it simpler than trying to handle the pressed state explicitly.

In reply to: 463305999 [](ancestors = 463305999)

derobinsMSFT · 2020-07-30T22:28:58Z

+            switch (e.Action)
+            {
+                case NotifyCollectionChangedAction.Add:
+                    // Add rectangles to display each marker. There are 2 rectangle - one


rectangle [](start = 74, length = 9)

"rectangles" #Closed

derobinsMSFT

karkarl · 2020-08-05T01:20:58Z

+                Maximum="1" 
+                StepFrequency="0.0001" 
+                SmallChange="0.001" 
+                AutomationProperties.Name="Progress slider">


nit: rename to ProgressSlider #ByDesign

This is a copy of the automation name that was given by another contributor. I don't know the rules for these names - is it supposed to be a single word?

In reply to: 465417002 [](ancestors = 465417002)

Discussed privately - WinUI team makes these a single Pascal-cased word, so we'll do the same.s

In reply to: 465910831 [](ancestors = 465910831,465417002)

But that would go against MSDN official advice: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.automationproperties.nameproperty?view=winrt-19041

"The Name property should be the same as the label text on screen."

We wouldn't smoosh and Pascal-case label text.

We don't have a label for this slider, but the name should correspond with the label that we would use if there were a label.

In reply to: 465975160 [](ancestors = 465975160,465910831,465417002)

karkarl · 2020-08-05T17:58:11Z

+            _decreaseRectangleBrush = CreateBoundColorBrush(_properties, "DecreaseRectangleColor");
+
+            // Create the track rectangle. This is the track that the thumb moves along.
+            _trackRectangle = c.CreateSpriteVisual();


I'm curious why the trackRectangle and decreaseRectangle have to be initialized in code behind when it's already set in the template? #Resolved

They're not the same object. The template contains the XAML Rectangles, but with opacity=0 so they don't show up. We use Composition to draw the UI, but XAML to receive the input. Composition SpriteVisuals can be animated via expressions that respond to changes on other Composition object. The problem this solves is making the slider move while the Lottie is playing. When the Lottie is playing, there's no code running in the LottieViewer process, so we can't animate the XAML Rectangles.

In reply to: 465905081 [](ancestors = 465905081)

karkarl · 2020-08-05T18:15:58Z

+
+        void SetColors(Color markers, Color thumbFill, Color thumbStroke, Color track, Color decreaseRectangle)
+        {
+            _markerBrush.Color = markers;


I'm curious as to why colours are set in code behind? The brushes are already set in the VisualStateManager in xaml, thus you can update colours through updating the VisualState. #Resolved

Different brush - this is updating the Composition objects, not the XAML objects.

In reply to: 465914466 [](ancestors = 465914466)

simeoncran requested a review from a team as a code owner July 23, 2020 15:05

Merge branch 'master' into MarkersOnScrubber

b7ac01d

raymond-wh-leung reviewed Jul 30, 2020

View reviewed changes

Simeon Cran added 2 commits July 30, 2020 10:32

Fix problem caused by merge of previous commit.

5ff35e1

We need to be able to handle TrimmedAnimatables that are initialized with a default value.

PR feedback: fix typo.

9795bf6

raymond-wh-leung previously approved these changes Jul 30, 2020

View reviewed changes

derobinsMSFT reviewed Jul 30, 2020

View reviewed changes

PR CR feedback.

62b899c

simeoncran dismissed raymond-wh-leung’s stale review via 62b899c July 30, 2020 22:53

derobinsMSFT approved these changes Jul 31, 2020

View reviewed changes

Simeon added 2 commits July 31, 2020 12:48

Merge branch 'master' into MarkersOnScrubber

162927c

Merge branch 'master' into MarkersOnScrubber

7605937

karkarl reviewed Aug 5, 2020

View reviewed changes

Simeon added 3 commits August 6, 2020 09:56

Merge branch 'master' into MarkersOnScrubber

12dc71e

Merge branch 'master' into MarkersOnScrubber

1426328

Merge branch 'master' into MarkersOnScrubber

18fe781

simeoncran merged commit 9ddf5e8 into master Aug 6, 2020

delete-merged-branch Bot deleted the MarkersOnScrubber branch August 6, 2020 20:58

Conversation

simeoncran commented Jul 23, 2020

Uh oh!

raymond-wh-leung Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymond-wh-leung Jul 30, 2020 • edited by simeoncran Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymond-wh-leung Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymond-wh-leung left a comment

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Jul 30, 2020 • edited by simeoncran Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Jul 30, 2020 • edited by simeoncran Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

karkarl Aug 5, 2020 • edited by simeoncran Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karkarl Aug 5, 2020 • edited by simeoncran Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karkarl Aug 5, 2020 • edited by simeoncran Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

raymond-wh-leung Jul 30, 2020 •

edited

Loading

raymond-wh-leung Jul 30, 2020 •

edited by simeoncran

Loading

raymond-wh-leung Jul 30, 2020 •

edited

Loading

derobinsMSFT Jul 30, 2020 •

edited by simeoncran

Loading

derobinsMSFT Jul 30, 2020 •

edited by simeoncran

Loading

derobinsMSFT Jul 30, 2020 •

edited

Loading

karkarl Aug 5, 2020 •

edited by simeoncran

Loading

karkarl Aug 5, 2020 •

edited by simeoncran

Loading

karkarl Aug 5, 2020 •

edited by simeoncran

Loading