Add Cupertino scrollbar by xster · Pull Request #13290 · flutter/flutter

xster · 2017-12-01T04:33:36Z

Fixes #8774

Extract the scrollbar painter to widget
Add a bunch of new configurables like margins, sizing, when to fade etc
Create CupertinoScrollbar
Let material Scrollbar change look dynamically

xster · 2017-12-07T00:03:41Z

Rebased from upstream

Hixie · 2017-12-07T00:19:30Z

Please run flutter analyze --dartdocs to catch new members that are missing documentation.

Ah crap, thanks for catching. Added a bunch missing. Surprised it wasn't in travis.

Hixie · 2017-12-07T00:20:14Z

Would it make sense for this just to be a private member on the state?

Material scrollbar actually draws with cupertino painter on iOS. Seems consistent with googlekit on iOS.

ah, got it.

xster · 2017-12-07T23:10:01Z

Oops, didn't actually force push after rebasing. Pushed for real now.

Hixie · 2017-12-08T23:42:30Z

you can remove this TODO (and the matching one on the other side), we're not likely to do this any time soon.

This is easier to do now. Is it worth doing and reshaping the tree each time opacity comes in and out of = 0.0?

I think we should just wait until we have a reason to do it.

Hixie · 2017-12-08T23:46:01Z

IMHO we really shouldn't make the original _ScrollbarPainter class public. It's a crazy class with crazy semantics (e.g. it's a custom painter but you have to dispose it).

If I left 2 implementations in 2 separate files, it seems to alleviate your concern here and on the next comment. Do you prefer that approach? A bit copy paste-y I initially thought.

Hixie · 2017-12-08T23:46:29Z

It doesn't make any sense for a CustomPainter to have externally mutable state.

Maybe that's too strong a statement. Let me say instead, it's very much contrary to how CustomPainter was intended to work.

Seems like the existing custom painter in material already mutates the painter (when the inherited theme changes). I didn't see any class docs on CustomPainter on its desired immutability.

Hixie · 2017-12-08T23:48:13Z

false is wrong here. It should be comparing every constructor argument, and they should all be immutable fields, so that when you create a new instance of this custom painter, the painting logic knows whether it's different or not.

xster · 2017-12-16T00:43:11Z

I did the refactor we talked about.

Basically the ScrollbarPainter is mostly immutable minus opacity and scroll metrics.
Material scrollbar just nests cupertino scrollbar on iOS.

Moved animation, controller, timer out to the widget states.

Hixie · 2017-12-21T00:44:57Z

This is on my list to review, sorry I haven't gotten to it yet.

Hixie · 2017-12-21T20:48:19Z

+    final ThemeData theme = Theme.of(context);
+    _currentPlatform = theme.platform;
+
+    if (_currentPlatform == TargetPlatform.iOS) {


here and in various other places: where you currently us an if to check the platform, use a switch so that the analyzer tells us if we missed handling a new platform when we add one

Hixie · 2017-12-21T20:48:35Z

+
+/// A [CustomPainter] for painting scrollbars.
+///
+/// Unlike [CustomPainter]s that sub-classes [CustomPainter] and only repaints


s/sub-classes/subclass/

s/repaints/repaint/

Hixie · 2017-12-21T20:49:11Z

+  /// An opacity [Animation] that dictates the opacity of the thumb.
+  /// Changes in value of this [Listenable] will automatically trigger repaints.
+  /// Mustn't be null.
+  Animation<double> fadeoutOpacityAnimation;


should be final

Ah right, thanks

Hixie · 2017-12-21T20:50:41Z

+///
+///  * the scroll position changes; and
+///  * when the scrollbar fades away.
+///


These docs should talk about update() and how to use it

Hixie · 2017-12-21T20:51:14Z

+       assert(mainAxisMargin != null),
+       assert(crossAxisMargin != null),
+       assert(minLength != null) {
+    fadeoutOpacityAnimation.addListener(notifyListeners);


when do you removeListener?

I think you need to override dispose() and call removeListener there.

Also, the docs need to talk about how it's important to call dispose() on this object when you're done with it.

Added doc about needing to dispose.
ChangeNotifier.dispose already more or less does removeListener.

How does ChangeNotifier know about fadeoutOpacityAnimation?

ah you're right. Sorry, I mixed the 2 layers of listeners up.

Hixie · 2017-12-21T20:52:03Z

modulo some minor issues.

xster · 2017-12-21T21:25:02Z

Thanks for reviewing

* Create CupertinoScrollbar * handle main axis margin * Adaptive material scrollbar and tests * Small tweaks * reapply changes on head * Docs * start * Refactored ScrollbarPainter to be more immutable * fix tests * fix bug: one animationcontroller pointed to multiple painters * some docs tweak * remove unused import * review * review * add dispose

googlebot added the cla: yes label Dec 1, 2017

xster force-pushed the cupertino-scrollbar branch from 1ec6aa2 to 2fc0f22 Compare December 7, 2017 00:03

Hixie reviewed Dec 7, 2017

View reviewed changes

xster force-pushed the cupertino-scrollbar branch from 2fc0f22 to f5693e9 Compare December 7, 2017 23:09

Hixie reviewed Dec 8, 2017

View reviewed changes

xster added 6 commits December 14, 2017 16:24

Create CupertinoScrollbar

9ce7957

handle main axis margin

c4ed09e

Adaptive material scrollbar and tests

f22c0a3

Small tweaks

b6a34e6

reapply changes on head

c6bc04d

Docs

563a0a3

xster force-pushed the cupertino-scrollbar branch from f5693e9 to 563a0a3 Compare December 15, 2017 00:24

xster added 5 commits December 14, 2017 16:37

start

2743dcf

Refactored ScrollbarPainter to be more immutable

2702234

fix tests

266c870

fix bug: one animationcontroller pointed to multiple painters

7684301

some docs tweak

36e4444

xster added 2 commits December 18, 2017 13:18

remove unused import

9745357

review

38f9e6e

Hixie reviewed Dec 21, 2017

View reviewed changes

review

a3c57dd

add dispose

bc043a9

xster merged commit 4fcdb50 into flutter:master Dec 22, 2017

xster deleted the cupertino-scrollbar branch December 22, 2017 00:44

justinmc mentioned this pull request Jul 25, 2019

Theme's highlightColor fails to change Scrollbar color #36603

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 12, 2021

Conversation

xster commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xster commented Dec 7, 2017

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!

xster commented Dec 7, 2017

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!

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!

xster commented Dec 16, 2017

Uh oh!

Hixie commented Dec 21, 2017

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!

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!

Hixie commented Dec 21, 2017

Uh oh!

xster commented Dec 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

xster commented Dec 1, 2017 •

edited

Loading