[Slider] Custom track, ticker, and overlay shape painters by clocksmith · Pull Request #25008 · flutter/flutter

clocksmith · 2018-12-05T22:59:14Z

Create a slider shape for custom track, tick mark, and overlay painting. Originally, this work was part of the larger range slider effort, but can and should work independently of having more than 1 thumb.

Without this change

Part             | Custom Painter? | Custom Options?
__________________________________
Track            | ** NO **        | color
Ticks            | ** NO **        | periodicity, color
Thumbs           | YES             | color
Overlay          | ** NO **        | color
Value Indicators | YES             | color, text style

With this change

Part             | Custom Painter? | Custom Options?
__________________________________
Track            | ** YES **       | color, ** height **
Ticks            | ** YES **       | periodicity, color
Thumbs           | YES             | color
Overlay          | ** YES **       | color
Value Indicators | YES             | color, text style

CHANGES:

SliderTrackShape is added as slider theme param, with a default of RectangularSliderTrackShape, and slider RenderBox paint() is updated to use it.
- The default track shape uses the slider theme colors to maintain compatibility with the activeTrackColor and inactiveTrackColor params in Slider.
- The default track shape uses a newly created slider theme param: trackHeight, but is overridden if custom track shape is used.
- Dependency between overlay, track length, and value mapping is removed from the slider class, and made an artifact of the default track shape.
SliderComponentShape for ticks is added as a slider theme param, with a default of RoundSliderTickShape, and slider RenderBox paint() is updated to use it.
All tick marks are now painted, instead of excluding the one with the same center as the thumb. This does not affect the default state, but is important if custom tick marks extend the bounds of a thumb
- Default tick mark radius is a function of track height.
SliderComponentShape for overlay is added as a slider theme param, with a default of RoundSliderOverlayShape, and slider RenderBox paint() is updated to use it.
The new shape API and slider paint() works for a single thumb, but also allows for easy adaptation to multiple thumbs, in preparation for the RangeSlider, without having to change the new public API.

NEXT UP:

Update rendering to material spec.
- Tick marks should be aligned with the center of the value, not adjusted to be contained within the track.
- Thumb shape default radius should be larger.
More decoupling and customization.
- Default overlay shape painting should be a function of thumbShape.
- Default overlay shape should paint after thumb and be painted as an annulus.
- Default overlay color should use new color theme.
- Paint color tweens can be initialized once, rather than on every paint.
- RangeSlider

closes #22325

johnsonmh

Looks good, I just had a bunch of nits but it's really just the same 2/3 things.

clocksmith · 2018-12-18T21:39:33Z

Updated, PTAL

gspencergoog · 2018-12-18T22:55:29Z

You will need to add tests before you commit, but it looks OK to me. Also, it would be nice to add to the "custom slider" demo in the Flutter gallery app to show a simple customization of the track, etc.

clocksmith · 2018-12-19T17:27:45Z

I added tests, and am going to update the gallery with examples next (sep PR)

HansMuller

Still working on this. Progress report :-)

HansMuller · 2018-12-19T21:16:56Z

I think it's sufficient to say that it's the default [Slider] track shape.

Is it the default value of SliderThemeData.trackShape?

Technically it is the default shape of the SliderThemeData.fromPrimaryColors ctor which is the default for the Theme. I copied the description directly from the 2 existing default shapes (RoundThumb and PaddleValueIndicator). Can cover this in #25672 as well

HansMuller

More feedback that's mostly about the docs. Adding this much new API requires adding a commensurate amount of helpful documentation.

HansMuller · 2018-12-19T22:37:40Z

This isn't really helpful (if it is helpful then the reader is out of their depth).

It might be worth pointing out how he trackShape's preferred size contribute's to the slider's overall preferred size.

It would be worth pointing out that the default value of trackShape is RectangularSliderTrackShape.

I was following the pattern of the other existing components here. If I remove "This can be customized by implementing a subclass of Foo" from the 3 new components, should I also take it away from the existing 2?

It might be worth pointing out how he trackShape's preferred size contribute's to the slider's overall preferred size.

It doesn't actually do that in this change.

The default track size paints relative to the size of the parent box, but it subtracts out the size largest overlapping piece: the overlay. The intrinsic sizes are dependent on the overlay, thumb, and value indicators. The track shape's preferredRect is used inside the Slider class for:

Tap gesture points to thumb position range, where 0.0 is track.left and 1.0 is track.right.

The thumb center and discrete tick centers for their shape painters.

I added these as doc comments, but will go in more detail once I get to the actual SliderTrackShape doc comments.

It would be worth pointing out that the default value of trackShape is RectangularSliderTrackShape.

Done

HansMuller · 2018-12-19T22:38:19Z

Doesn't define behavior.

The size and shape of the each [Slider] tick.

Will resolve from above question:

I was following the pattern of the other existing components here. If I remove "This can be customized by implementing a subclass of Foo" from the 3 new components, should I also take it away from the existing 2?

No need to fix them up in this PR. Filing an issue, so that the work happens eventually, would be helpful.

Done, I ended up keeping some comments consistent by updating the, and some constsent by leaving tham as is, but I have filed #25672 to further improve the docs, and create a diagram

clocksmith · 2018-12-21T19:07:10Z

Added 2 issues for follow up PRs

#25672 - Documentation diagram and consistency
#25671 - DiagnostiProperties tests

clocksmith · 2019-01-04T18:48:58Z

Ready for next review, PTAL

HansMuller · 2019-01-08T00:02:50Z

This is still hard to understand. This "overlay" is the temporary highlight that appears around the slider's thumb when the thumb is pressed and dragged?

Yeah overlay was an existing term used within this class. The spec does not call it anything specifically, but it is visible when the thumb is pressed, focused, or hovered.

I added a little more description.

HansMuller · 2019-01-08T01:14:42Z

Why does this method have an isEnabled parameter?

In case a developer wants the track size for enabled/disabled to be different

HansMuller · 2019-01-08T01:17:07Z

The conventional boilerplate for see also references like this is:

[RectangularSliderTrackShape], which default track shape.

Fixed through out

HansMuller · 2019-01-08T01:18:27Z

If this is just the value of Slider.isInteractive, why isn't the parameter called isInteractive?

The correct terminology is "enabled" and "disabled", but isInteractive already exists within slider.dart. It is already correctly referred to as isEnabled in other places slider_theme.dart, so I am continuing with that. Ideally, it would be refactored to isEnabled, enabled, disabled, etc in slider.dart as well.

https://material.io/design/components/sliders.html#continuous-slider
see "States" ^

HansMuller · 2019-01-08T01:32:02Z

One other thing: the SliderTheme "see also" doc should mention the new shape values.

HansMuller

Looks like there are still comments from the last review that need to be addressed. Before submitting for another review, please read everything carefully:

Watch out for obvious typos and indentos.
Don't document methods or properties by reiterating the name.
Make sure all feedback has been addressed.

HansMuller · 2019-01-14T18:14:42Z

This appears to assume that the centers of the 3 shapes will be aligned. That doesn't seem correct?

HansMuller · 2019-01-14T18:22:42Z

This makes more sense then the original; not clear why _overlayRadius was used. On the other hand the code was known to work ... any idea why we used to subtract _overlayRadius?

Because the track was always padded from the slider edges by the overlay radius, but now that is encapsulated in the _trackRect

HansMuller · 2019-01-14T18:26:53Z

Is the original comment obsolete now?

It looks like the underlying assumption here is that the thumb/overlay/tick could extend horizontally past both ends of the track by 50%. If that's correct, it would be helpful to say as much.

HansMuller · 2019-01-14T18:36:25Z

computeMaxIntrinsicWidth isn't supposed to be called directly (see https://docs.flutter.io/flutter/rendering/RenderBox/computeMaxIntrinsicWidth.html), call getMaxIntrinsicWith().

HansMuller · 2019-01-14T19:29:57Z

extra indent space

fixed in macro

HansMuller · 2019-01-14T19:31:13Z

[isEnabled] has the same value as [Slider.isInteractive]. If true the slider will respond to input.

fixed in macro

HansMuller · 2019-01-14T19:33:10Z

Please don't just reiterate the name of a property as its definition.

This param no longer exists, removed

Moved a lot of these things to macros too

HansMuller · 2019-01-14T19:34:28Z

The [center] offset is relative to the origin of the canvas.

I think this comment may have to be adjusted with the macros that are now being used

clocksmith · 2019-01-16T01:47:17Z

One other thing: the SliderTheme "see also" doc should mention the new shape values.

It mentions the abstract shapes. Should it also mention the defaults?

HansMuller

This is really close. Just some simple cleanups.

HansMuller · 2019-01-29T20:45:27Z

+  // need to padding to the track relative to the slider box itself. It is
+  // assumed that these shapes are vertically centered on the track.
+  double get _maxHorizontalTrackPadding => _sizesThatCanPadTrack.map((Size size) => size.width).reduce(math.max);
+  double get _maxVerticalTrackPadding => _sizesThatCanPadTrack.map((Size size) => size.width).reduce(math.max);


These are width and height values not really padding values. It would be clearer if they were called: maxSliderPartHeight and maxSliderPartWidth; the list below would be sliderPartSizes.

HansMuller · 2019-01-29T20:46:12Z


  @override
-  double computeMinIntrinsicHeight(double width) => _verticalPaddingFromShapes;
+  double computeMinIntrinsicHeight(double width) => _maxVerticalTrackPadding;


Shouldn't this be math.max(minPreferredTrackHeight, maxSliderPartHeight)

Same change in performResize

Yes that is right. I changed it. Now the slider will size correctly if the track height is larger than the max height of the other parts.

HansMuller · 2019-01-29T23:47:20Z

  });

+  testWidgets('SliderThemeData assigns the correct default shapes', (WidgetTester tester) async {
+    final SliderThemeData sliderTheme = _createDefaultSliderTheme();


This value should be ThemeData().sliderTheme.

HansMuller · 2019-01-29T23:50:25Z

+    );
+    final SliderThemeData sliderTheme = theme.sliderTheme.copyWith(thumbColor: Colors.red.shade500);
+    double value = 0.25;
+    Widget buildApp({


One line:

Widget buildApp({ bool enabled = true }) {

HansMuller · 2019-01-29T23:51:30Z

+      bool enabled = true,
+    }) {
+      final ValueChanged<double> onChanged = enabled ? (double d) => value = d : null;
+      return Directionality(


It's OK to must build a MaterialApp/Scaffold for tests like this

I made to be consistent with other tests (using the "Directionality -> MediaQuery -> Material -> Center -> SLiderTheme -> Slider" tree structure.) Is this one different?

No, just a suggestion since pasting things together by hand is tedious and doesn't actually test the common case (MaterialApp/Scaffold is the common case).

HansMuller

LGTM

Hooray!

johnsonmh

LGTM

…ider (flutter#25008) Create a slider shape for custom track, tick mark, and overlay painting, for the material slider.

clocksmith requested review from gspencergoog and willlarche December 5, 2018 22:59

googlebot added the cla: yes label Dec 5, 2018

clocksmith changed the title ~~Slider track painter~~ [WIP] [Slider] Custom track shape painter Dec 5, 2018

zoechi added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Dec 6, 2018

clocksmith changed the title ~~[WIP] [Slider] Custom track shape painter~~ [WIP] [Slider] Custom track, ticker, and overlay shape painters Dec 11, 2018

clocksmith force-pushed the slider-track-painter branch from fee0205 to 8aaf7b9 Compare December 12, 2018 17:23

clocksmith changed the title ~~[WIP] [Slider] Custom track, ticker, and overlay shape painters~~ [Slider] Custom track, ticker, and overlay shape painters Dec 12, 2018

clocksmith requested a review from johnsonmh December 12, 2018 20:54

johnsonmh requested changes Dec 12, 2018

View reviewed changes

gspencergoog reviewed Dec 18, 2018

View reviewed changes

clocksmith requested a review from HansMuller December 19, 2018 17:27

HansMuller suggested changes Dec 19, 2018

View reviewed changes

clocksmith mentioned this pull request Jan 3, 2019

Provide ability to make origin of active track color in Slider start at arbitrary value #22325

Closed

HansMuller reviewed Jan 8, 2019

View reviewed changes

clocksmith force-pushed the slider-track-painter branch 3 times, most recently from 8f575f9 to 5d08a96 Compare January 9, 2019 19:47

HansMuller suggested changes Jan 14, 2019

View reviewed changes

rebase

57c3b79

clocksmith force-pushed the slider-track-painter branch from 3a8caa7 to 57c3b79 Compare January 14, 2019 23:22

clocksmith added 10 commits January 15, 2019 22:46

refactored docs to use macros

21be680

refactored docs to use macros

b9494a4

indentation

d3bca04

comments

fed7a90

comments

d57eb73

more docs

fcba4d4

Formatting

a5b525a

Formatting

0f49801

Formatting

4e75610

updated instrinsic width calcs

d5744e5

HansMuller reviewed Jan 29, 2019

View reviewed changes

clocksmith added 2 commits January 30, 2019 11:44

resolve slider and slider_theme comments

ed477da

resolve test comments

caeddfc

HansMuller approved these changes Jan 31, 2019

View reviewed changes

clocksmith added 2 commits January 31, 2019 18:20

merge

2c66014

merge

2f40dd2

johnsonmh approved these changes Jan 31, 2019

View reviewed changes

clocksmith merged commit cfe992d into flutter:master Feb 1, 2019

clocksmith mentioned this pull request Feb 25, 2019

Material Slider track should be customizable. #19216

Closed

clocksmith deleted the slider-track-painter branch September 12, 2019 16:56

Conversation

clocksmith commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnsonmh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clocksmith commented Dec 18, 2018

Uh oh!

gspencergoog commented Dec 18, 2018

Uh oh!

clocksmith commented Dec 19, 2018

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clocksmith commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clocksmith commented Jan 4, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clocksmith commented Dec 5, 2018 •

edited

Loading

clocksmith commented Dec 21, 2018 •

edited

Loading

clocksmith Jan 8, 2019 •

edited

Loading

HansMuller left a comment •

edited

Loading