Added new RefreshIndicator widget to be used with SliverAppBar. by elbeicktalat · Pull Request #105769 · flutter/flutter

elbeicktalat · 2022-06-10T09:17:11Z

This PR. introduce a new widget called SliverRefreshIndicator which is the material equivalent to CupertinoSliverRefreshControl, both widget are subclasses of BaseSliverRefreshControl which holds the shared logic.

It is also provide a new named constructor partiallyRevealed to the existing RefreshProgressIndicator, which allows the getting RefreshProgressIndicator based on passed progress value.

Fix #54272

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

Piinks · 2022-06-10T19:35:34Z

Self-pinging to put this in my inbox for review. Thank you for putting this together. It looks like the failing tests are either broken doc references, analyzer issues or an issue with the sample code you are adding.

It sounds like this PR is really two features as described above. Can this be split into two separate PRs then? It makes it easier to review, but more importantly, if 1 of 2 features causes an issue, it will be reverted, meaning both would have to be reverted rather than just the one.

elbeicktalat · 2022-06-10T22:47:22Z

I think it doesn't make sense splitting this in two PR. since they are related to each other.

In the end, if you see that essential splitting this in two PR. then I'll do it.

Thank you!

Piinks

Can you see about resolving the failures below first? Then we can give this a proper review.
Also, the sample code you have written requires tests to ensure it works as expected. Those tests can be found under examples/api/test.
Thanks!

Piinks · 2022-06-13T22:02:30Z

+                  childCount: 20,
+                  (BuildContext context, int index) {


This is causing one of the analysis failures the named parameter should come after the builder.

Piinks · 2022-06-13T22:02:57Z

+        childCount: testListLength,
+        (BuildContext context, int index) {


Same analysis error here

Piinks · 2022-06-13T22:06:40Z

+/// When the user scrolls past [refreshTriggerPullDistance],
+/// this sample shows material pull to refresh indicator for 1 second and
+/// adds a new item to the top of the list view.
+///
+/// ** See code in examples/api/lib/material/refresh_indicator/sliver_refresh_control.0.dart **
+/// {@end-tool}


This is causing some of the failures as well. Can you check that the sample code follows the same pattern as other samples? It is throwing an error.

Piinks · 2022-06-13T22:09:57Z

+///  * [CustomScrollView], a typical sliver holding scroll view this control
+///    should go into.
+///  * [RefreshIndicator], a Material Design version of the pull-to-refresh
+///    paradigm. This widget works differently than [SliverRefreshControl]


There does not appear to be a class with this name, this is one of the docs failures.

Piinks · 2022-06-13T22:10:26Z

+///
+/// See also:
+///
+/// * [SliverRefreshControl], A sliver refresh control, that reflects the standard Material pull-to-refresh behavior.


Same here, this does not exist.

Piinks · 2022-06-13T22:12:02Z

+// Flutter code sample for SliverRefreshIndicator
+
+import 'package:flutter/material.dart';
+


The failing framework test shard is due to this sample code see logs below:

The following TestFailure was thrown running a test: Expected: exactly one matching node in the widget tree Actual: _WidgetTypeFinder:<zero widgets with type "WidgetsApp" (ignoring offstage widgets)> Which: means none were found but one was expected When the exception was thrown, this was the stack: #4 main.<anonymous closure> (file:///tmp/flutter%20sdk/examples/api/integration_test/smoke_integration_test.dart:2457:9) <asynchronous suspension> <asynchronous suspension> (elided one frame from package:stack_trace) This was caught by the test expectation on the following line: file:///tmp/flutter%20sdk/examples/api/integration_test/smoke_integration_test.dart line 2457 The test description was: Smoke test material/refresh_indicator/sliver_refresh_indicator.0.dart

I would refer to other sample code to see if there is something missing from your case here.

Piinks · 2022-06-13T22:34:20Z

I think it doesn't make sense splitting this in two PR. since they are related to each other.

One way to tell is, how does adding partiallyRevealed resolve #54272? If it does not resolve the issue, it should probably be a separate PR.

Piinks · 2022-06-13T22:47:39Z

+  Widget build(BuildContext context) {
+    return Scaffold(
+      body: CustomScrollView(
+        physics: const BouncingScrollPhysics(


Is this required for this to work? This would not match native scroll physics for folks that want a native look and feel on Android.

Yes, this is required to create a overscrolling space which is not available the the older versions of android, I don't think that flutter dose include the new behavior of native android.

Is this can be considered as an issue?

Piinks · 2022-06-13T22:51:09Z

Also of note, the material version here does not currently match the native behavior.

This is the current behavior of RefreshIndicator:

Screen.Recording.2022-06-13.at.5.46.35.PM.mov

And this is the proposed sliver refresh here:

Screen.Recording.2022-06-13.at.5.43.41.PM.mov

The behavior does not look quite right. Can you take a look?

This page should detail of the expected behaviors of this widget:
https://material.io/design/platform-guidance/android-swipe-to-refresh.html#usage

LongCatIsLooong · 2022-06-14T06:58:37Z

+  const SliverRefreshIndicator({
+    super.key,
+    super.refreshTriggerPullDistance,
+    super.refreshIndicatorExtent,


The material refresh indicator does not occupy any layoutExtent or scrollExtent, see: https://material.io/design/platform-guidance/android-swipe-to-refresh.html#usage

LongCatIsLooong · 2022-06-14T07:04:23Z

+  /// A builder that's called as this sliver's size changes, and as the state
+  /// changes.
+  ///
+  /// Can be set to null, in which case nothing will be drawn in the overscrolled


This doesn't seem to be true. It falls back to buildRefreshIndicator in _BaseSliverRefreshControlState.build. Is there a reason you're making this change?

LongCatIsLooong · 2022-06-14T07:13:49Z

+  /// When provided, the value of [progress] must be between 0.0
+  /// and 1.0 (full circle) inclusive. Defaults to 1.0.
+  /// {@macro flutter.material.ProgressIndicator.ProgressIndicator}
+  const RefreshProgressIndicator.partiallyRevealed({


The cupertino refresh control reveals the "spinner" slice by slice as you drag thus the name (see the gif here: https://stackoverflow.com/questions/29791995/swift-pull-to-refresh). The material circular progress indicator doesn't do the same so partiallyRevealed doesn't seem to accurately reflect the actual behavior.

Without partiallyRevealed the result will be like:

Screen_Recording_20220614-101128.mp4

As you scroll the RefreshIndicator is being animated.

With partiallyRevealed the result like:

Screen_Recording_20220614-101207.mp4

LongCatIsLooong · 2022-06-14T07:17:57Z

 }

-class _CircularProgressIndicatorState extends State<CircularProgressIndicator> with SingleTickerProviderStateMixin {
+class _CircularProgressIndicatorState<T extends CircularProgressIndicator> extends State<T> with SingleTickerProviderStateMixin {


Is there a reason this needs to be made a type parameter? RefreshProgressIndicator is already a subtype of CircularProgressIndicator.

Well I have added a new property on the subclass without this I can't access to it. I could cast the super class as the subclass, but I think this is can be useful for the future.

LongCatIsLooong · 2022-06-14T07:31:57Z

+    return SizedBox(
+      width: radius * 2,
+      height: radius * 2,
+      child: FittedBox(


The material refresh indicator does not resize when you overscroll I think? If you use a FittedBox here it will expand when the user overscrolls and shrink when the scroll view bounces back.

This fit the circular indicator in the refresh indicator, other way the look will be like:

elbeicktalat · 2022-06-14T13:47:44Z

Hi @Piinks,
I have fixed all errors, now the big deal is the behavior of the SliverRefreshIndicator, since this widget becomes part of the slivers, then I hit the following:

The indicator can't begin behind of SliverAppBar.
The CustomScrollView must have a physics of type BouncingScrollPhysics and it should be AlwaysScrollableScrollPhysics.

With the actual structure can arrive to something like:

Screen_Recording_20220614-145925.mp4

If this doesn't seems to be right, then probably I'll reset The CupertinoSliverRefreshControl and provide a dedicated implementation for SliverRefreshIndicatro.

Thank you!

Piinks · 2022-06-21T19:55:47Z

The indicator can't begin behind of SliverAppBar.

The CustomScrollView must have a physics of type BouncingScrollPhysics and it should be AlwaysScrollableScrollPhysics.

This breaks the design of the Material indicator, so I do not think we will want to make this change. If there is a way to change this proposal and match the design spec, we can definitely take another look!

elbeicktalat · 2022-06-22T17:30:51Z

For sure I can get out of this issue, but I have to revert a few changes like: having a Base implementation for both iOS and Material.

Should I allow RefreshIndicator to warp the CustomListView, or make a new refresh indicator widget which becomes part of CustomListView?

Thanks!

Piinks · 2022-06-22T18:30:59Z

Should I allow RefreshIndicator to warp the CustomListView, or make a new refresh indicator widget which becomes part of CustomListView?

Do you mean CustomScrollView? I am not sure! I would return to the issue you are trying to solve, and see what you think would be the best approach. I do have a proposal tat is almost finished that may resolve this, I would love your feedback on it when it is ready. :)

Hixie · 2022-09-20T23:27:34Z

@elbeicktalat Will you be able to continue working on this? It's totally fine if not; we can take it over (though it might take longer). I'm just asking because I'm trying to clean out our review queue.

elbeicktalat · 2022-09-24T17:21:16Z

Hi @Hixie I'm really sorry for the delay in responding,
I was working on this until @Piinks side to me that she has a solution for this, any way unfortunately I'm no more able to work on this, but I still able to give info if there is need.

Thanks, and again I'm sorry!

Piinks · 2022-12-16T00:00:53Z

Hey @elbeicktalat, I am going to close this PR. I do still have plans to support this without breaking the current spec (#107182 was an earlier proposal), I just haven't circled back around to it yet.
I really appreciate your contribution and feedback on what we need to be supporting better here. Thank you!

elbeicktalat added 8 commits June 9, 2022 18:21

add BaseSliverRefreshControl

8dd7693

make CupertinoSliverRefreshControl inherit BaseSliverRefreshControl

a874bde

draw RefreshProgressIndicator based on passed percentage

3fe28bd

add material sliver implementation

bde8fff

pass all cupertino refresh tests

3a0feda

test material SliverRefreshControl

1a10fed

remove unnecessary protected

ae5d9e5

rename material refresh sliver

fd19c09

Piinks self-requested a review June 10, 2022 19:24

Piinks mentioned this pull request Jun 10, 2022

RefreshIndicator should appear from the top of the dragged widget where the [GlowingOverscrollIndicator] appears position. #71819

Closed

fix code format

6eb32f7

Piinks reviewed Jun 13, 2022

View reviewed changes

LongCatIsLooong reviewed Jun 14, 2022

View reviewed changes

elbeicktalat force-pushed the fix_54272 branch from 910683a to 6eb32f7 Compare June 14, 2022 09:54

fix fatal errors

3cfa763

Piinks closed this Dec 16, 2022

		childCount: testListLength,
		(BuildContext context, int index) {

		// Flutter code sample for SliverRefreshIndicator

		import 'package:flutter/material.dart';

Conversation

elbeicktalat commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Piinks commented Jun 10, 2022

Uh oh!

elbeicktalat commented Jun 10, 2022

Uh oh!

Piinks left a comment

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!

Piinks commented Jun 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks commented Jun 13, 2022

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!

elbeicktalat commented Jun 14, 2022

Uh oh!

Piinks commented Jun 21, 2022

Uh oh!

elbeicktalat commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Jun 22, 2022

Uh oh!

Hixie commented Sep 20, 2022

Uh oh!

elbeicktalat commented Sep 24, 2022

Uh oh!

Piinks commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elbeicktalat commented Jun 10, 2022 •

edited

Loading

elbeicktalat commented Jun 22, 2022 •

edited

Loading