Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport #37024

LongCatIsLooong · 2019-07-26T19:06:25Z

Description

Deprecate RenderSliverFillViewport
Change PageView's build method to use SliverLayoutBuilder instead.

Breaking Changes

RenderSliverFillViewport is now marked as deprecated.

Related Issues

Closes #23873
Closes #27744

Tests

I added the following tests:

All visible pages are able to receive touch events.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.

goderbauer · 2019-07-29T09:01:10Z

packages/flutter/lib/src/widgets/sliver.dart

@@ -1030,6 +1028,7 @@ class SliverGrid extends SliverMultiBoxAdaptorWidget {
 ///    the main axis extent of each item.
 ///  * [SliverList], which does not require its children to have the same
 ///    extent in the main axis.
+@Deprecated('Use SliverLayoutBuilder instead.')
 class SliverFillViewport extends SliverMultiBoxAdaptorWidget {


Can we keep this widget and just implement it in terms of SliverLayoutBuilder and SliverFixedExtendList? That would limit the scope of the API breakage and we'd only remove the RenderSliverFillViewport, I think?

chunhtai

overall looks good, but one concern about the behavior when viewportFraction > 1.0

chunhtai · 2019-07-29T23:31:14Z

packages/flutter/test/widgets/page_view_test.dart

@@ -556,13 +556,59 @@ void main() {

    await tester.pumpWidget(build(controller));

-    expect(tester.getTopLeft(find.text('Alabama')), const Offset(-100.0, 0.0));
-    expect(tester.getBottomRight(find.text('Alabama')), const Offset(900.0, 600.0));
+    expect(tester.getTopLeft(find.text('Alabama')), Offset.zero);


why this change?

it looks like we changed the behavior when viewportFraction > 1
The page start at the very left edge of the page instead of after extra space.

Yeah it does change the behavior when viewportFraction > 1 and breaks the API (although I doubt it would really affect anyone, and starting from 0 makes more sense to me) . Context: #8539

What's the motivation for changing this?

Please add a description of the changed behavior to the PR description.

I think the original behavior make more sense to me. I would expect the viewport is center align with the page, that is why you have the padding when viewportfraction<1. If the same thing apply when viewportfraction>1, the original behavior is expected. and i am not quite sure how #8539 related to this behavior. Can you elaborate?

Ah I thought a weird viewportFraction like 1.3 would mess up the snapping behavior but that's not true. Thanks for pointing out. But SliverPaddings can't go negative. I'll find an alternative for it.

goderbauer

Can you update the PR's description? Please also include all changes in behavior there.

You'll also have to follow https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes for both of the breaking changes.

goderbauer · 2019-07-30T08:10:07Z

packages/flutter/lib/src/widgets/sliver.dart

+        final double fixedExtent = constraints.viewportMainAxisExtent * viewportFraction;
+        final double padding = math.max(0, constraints.viewportMainAxisExtent - fixedExtent) / 2;
+
+        final bool isHorizontal = constraints.axisDirection == AxisDirection.left


Can you just use constraints.axis?

Also, with enums instead of checking individual values use a switch/case. That way the analyzer tells us that we need to update this code if we ever add new enum values.

goderbauer · 2019-07-30T08:16:39Z

packages/flutter/test/widgets/page_view_test.dart

@@ -556,13 +556,59 @@ void main() {

    await tester.pumpWidget(build(controller));

-    expect(tester.getTopLeft(find.text('Alabama')), const Offset(-100.0, 0.0));
-    expect(tester.getBottomRight(find.text('Alabama')), const Offset(900.0, 600.0));
+    expect(tester.getTopLeft(find.text('Alabama')), Offset.zero);


What's the motivation for changing this?

Please add a description of the changed behavior to the PR description.

goderbauer · 2019-07-30T08:17:20Z

packages/flutter/test/widgets/page_view_test.dart

+  });
+
+  testWidgets('All visible pages are able to receive touch events',
+    (WidgetTester tester) async {


nit: either place this at the end of the previous line (preferred) or indent this more.

goderbauer · 2019-07-30T08:17:58Z

packages/flutter/test/widgets/page_view_test.dart

+          itemBuilder: (BuildContext context, int index) {
+            return GestureDetector(
+              onTap: () => tappedIndex = index,
+              child: SizedBox.expand(child: Text(index.toString())),


Suggested change

child: SizedBox.expand(child: Text(index.toString())),

child: SizedBox.expand(child: Text('$index')),

Also below - I think that improves readability.

goderbauer · 2019-07-30T08:19:03Z

packages/flutter/test/widgets/page_view_test.dart

+    (WidgetTester tester) async {
+    final PageController controller = PageController(viewportFraction: 1/4, initialPage: 0);
+    int tappedIndex;
+    Iterable<int> visiblePages;


Declare this were the first value is assigned to it?

goderbauer · 2019-07-30T08:23:23Z

packages/flutter/test/widgets/page_view_test.dart

+      expect(find.text(index.toString()), findsOneWidget);
+      // The center of page 2's x-coordinate is 800, so we have to manually
+      // offset it a bit to make sure the tap lands within the screen.
+      final Offset center = tester.getCenter(find.text(index.toString())).translate(-3, 0);


You could also just subtract a small offset, which may make the intend here clearer:

Suggested change

final Offset center = tester.getCenter(find.text(index.toString())).translate(-3, 0);

final Offset center = tester.getCenter(find.text('$index')) - const Offset(3, 0);

chunhtai · 2019-07-31T23:02:23Z

packages/flutter/lib/src/widgets/page_view.dart

    if (oldPage != null)
-      forcePixels(getPixelsFromPage(oldPage));
+      // This part should only be reacheable when it will definitely trigger a rebuild.
+      correctPixels(getPixelsFromPage(oldPage));


this feel dangerous to me. That is true when you change this property in didupdatewidget. If you are certain that is the case for the future as well, you should add an assert to verify that.

I am not sure if this is better but how about

if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.persistentCallbacks) { correctPixels(getPixelsFromPage(oldPage)); } else { forcePixels(getPixelsFromPage(oldPage)); }

This class itself is private, and the setter is only used in PageController.attach. I'm not sure if we should call notifyListeners in ScrollController.attach, I think it should be up to the caller?

I am more worry about we change the scroll offset but not markneedbuild, and yes this should be fine because this is private class and as of now it only called during the build stage. However, it is possible this might change. If you think that is unlikely, we should still add an assert here to guard it.

Yeah let me get rid of the setter altogether. The implementation should probably live in PageController.attach which has more context.

chunhtai · 2019-07-31T23:02:36Z

packages/flutter/lib/src/widgets/page_view.dart

@@ -364,22 +364,24 @@ class _PagePosition extends ScrollPositionWithSingleContext implements PageMetri
      return;
    final double oldPage = page;
    _viewportFraction = value;
+


nit, extra line

chunhtai · 2019-07-31T23:03:59Z

packages/flutter/test/widgets/page_view_test.dart

@@ -556,13 +556,95 @@ void main() {

    await tester.pumpWidget(build(controller));

-    expect(tester.getTopLeft(find.text('Alabama')), const Offset(-100.0, 0.0));
+    expect(tester.getTopLeft(find.text('Alabama')), const Offset(-100, 0));


let's revert this change

LongCatIsLooong · 2019-08-09T17:48:45Z

@goderbauer It no longer changes the behavior when viewFraction > 1 I believe. Should I post the breakage to flutter-announcement now or there are further changes that need to be made?

goderbauer · 2019-08-09T19:54:30Z

packages/flutter/lib/src/widgets/page_view.dart

@@ -252,7 +252,14 @@ class PageController extends ScrollController {
  void attach(ScrollPosition position) {
    super.attach(position);
    final _PagePosition pagePosition = position;
-    pagePosition.viewportFraction = viewportFraction;
+
+    if (pagePosition.viewportFraction == viewportFraction)


Why move this out of the setter for viewportFraction?

forcePixel (in this new version) and correctPixel also have slightly different semantics regarding notifications...

from ScrollPosition.forcePixel documentation:

/// This should not be called during layout (e.g. when setting the initial
/// scroll offset). Consider [correctPixels] if you find you need to adjust
/// the position during layout.

Also it doesn't sound right to notify the listeners in ScrollController.attach.

Why move it out of the setter though?

IMO since using either forcePixel or correctPixel needs to be decided on a case-by-case basis, it feels a bit error-prone to have a viewportFraction setter which sounds like a silver bullet for changing viewportFraction.
Also attach is the only caller of the setter right now, so I got rid of the setter and moved the implementation to attach.

this is build, not layout. should be fine to call during build.

goderbauer · 2019-08-09T19:55:38Z

packages/flutter/lib/src/widgets/page_view.dart

-      forcePixels(getPixelsFromPage(oldPage));
-  }
+
+  double get _initialPageOffset => math.max(0, viewportDimension * (viewportFraction - 1) / 2);


It may be worthwhile to document what the "initial page offset" is.

Piinks · 2019-08-15T17:29:08Z

packages/flutter/test/widgets/page_view_test.dart

+
+  testWidgets(
+    'All visible pages are able to receive touch events',
+    (WidgetTester tester) async {


Same nit as above here regarding formatting.

I double checked the indentation, line 607 does have 2 more spaces than 606, it's just ( takes much less space than a regular character.

chunhtai · 2019-08-15T17:52:17Z

packages/flutter/lib/src/widgets/page_view.dart

+    final double oldPage = pagePosition.page;
+    pagePosition._viewportFraction = viewportFraction;
+    if (oldPage != null) {
+      pagePosition.correctPixels(pagePosition.getPixelsFromPage(oldPage));


according to documentation of correctPixels, this function should only be called during layout phase
can you add an assert something like
assert(SchedulerBinding.instance.schedulerPhase == SchedulerPhase.persistentCallbacks, 'attach can only be called during SchedulerPhase.persistentCallbacks')

If this assumption no longer held, would be better to update the documentation of correctPixels to include this case

should really only be called when debugDoingLayout is true for the viewport in question

goderbauer

LGTM

goderbauer · 2019-08-16T18:27:47Z

packages/flutter/lib/src/widgets/gesture_detector.dart

@@ -930,8 +930,7 @@ class RawGestureDetectorState extends State<RawGestureDetector> {
    }
  }

-  /// This method can be called outside of the build phase to filter the list of
-  /// available semantic actions.
+  /// This method can be called to filter the list of available semantic actions.


This should document when it is valid to call this method (e.g. what the assert is asserting).

chunhtai

LGTM, with some nit

chunhtai · 2019-08-16T19:38:30Z

packages/flutter/lib/src/widgets/page_view.dart

-  double getPixelsFromPage(double page) {
-    return page * viewportDimension * viewportFraction;
-  }
+  double getPixelsFromPage(double page) => page * viewportDimension * viewportFraction + _initialPageOffset;


I think it has to be a good old function if the line is too long

tvolkert · 2019-08-22T15:57:17Z

It looks like this regressed the "stock_build_iteration" microbenchmark by about 2%.

@LongCatIsLooong can you investigate? #39060

Hixie · 2019-11-08T22:11:50Z

packages/flutter/lib/src/rendering/sliver_fill.dart

@@ -27,6 +27,7 @@ import 'sliver_multi_box_adaptor.dart';
 ///  * [RenderSliverFixedExtentList], which has a configurable [itemExtent].
 ///  * [RenderSliverList], which does not require its children to have the same
 ///    extent in the main axis.
+@Deprecated('Use SliverLayoutBuilder instead.')


Looking into this further, I'm not sure this deprecation makes sense. If someone is using RenderSliverFillViewport directly, then they're not using widgets, which means that using SliverLayoutBuilder isn't an option.

We should probably remove this render object if we're not supporting it any more (which I presume we're not, since we didn't try to fix the hit test bug in this render object).

cc @goderbauer @Piinks

we move the padding from rendering layer to widget layer. I can't think of a way to achieve the same behavior by using our current rendering library. I agree we should remove this given we have deprecated warning for a while.

Yes, this is no longer used and should just be removed: #44612

…rSliverFillViewport (flutter#37024)" This reverts commit 9aea03f.

…rSliverFillViewport (#37024)" (#44778) * Revert "Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport (#37024)" This reverts commit 9aea03f.

…te RenderSliverFillViewport (flutter#37024)" (flutter#44778)" This reverts commit 851d699.

LongCatIsLooong added framework f: scrolling labels Jul 26, 2019

LongCatIsLooong requested review from goderbauer, HansMuller and chunhtai July 26, 2019 19:06

googlebot added the cla: yes label Jul 26, 2019

deprecate

11f23ef

LongCatIsLooong force-pushed the deprecate-sliverfillviewport branch from 655bf62 to 11f23ef Compare July 26, 2019 19:07

LongCatIsLooong added the c: API break label Jul 26, 2019

goderbauer reviewed Jul 29, 2019

View reviewed changes

LongCatIsLooong changed the title ~~Implement PageView using SliverLayoutBuilder, Deprecate SliverFillViewport~~ Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport Jul 29, 2019

Keep SliverFillViewport

1d0d78d

LongCatIsLooong force-pushed the deprecate-sliverfillviewport branch from 48aa1af to 1d0d78d Compare July 29, 2019 17:45

chunhtai reviewed Jul 29, 2019

View reviewed changes

goderbauer reviewed Jul 30, 2019

View reviewed changes

LongCatIsLooong changed the title ~~Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport~~ WIP Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport Jul 30, 2019

Piinks added the work in progress; do not review label Jul 30, 2019

review

ea59527

LongCatIsLooong force-pushed the deprecate-sliverfillviewport branch 2 times, most recently from 56c16e2 to 9662143 Compare July 31, 2019 22:18

Piinks added the f: gestures label Jul 31, 2019

LongCatIsLooong force-pushed the deprecate-sliverfillviewport branch 2 times, most recently from bffde64 to 8cc50bf Compare July 31, 2019 22:20

add a new test for changing viewportFraction

439a869

LongCatIsLooong force-pushed the deprecate-sliverfillviewport branch from 8cc50bf to 439a869 Compare July 31, 2019 22:22

LongCatIsLooong changed the title ~~WIP Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport~~ Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport Jul 31, 2019

LongCatIsLooong requested review from goderbauer and chunhtai July 31, 2019 22:26

LongCatIsLooong removed the work in progress; do not review label Jul 31, 2019

chunhtai reviewed Jul 31, 2019

View reviewed changes

LongCatIsLooong added 4 commits July 31, 2019 18:56

review

2c5c6ec

review

08f49be

Update page_view.dart

75ca211

get rid of getter

e4d1d92

goderbauer reviewed Aug 9, 2019

View reviewed changes

add comments explaining what _initalPageOffset is for

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

61fa9d7

Piinks reviewed Aug 15, 2019

View reviewed changes

chunhtai reviewed Aug 15, 2019

View reviewed changes

goderbauer approved these changes Aug 16, 2019

View reviewed changes

chunhtai approved these changes Aug 16, 2019

View reviewed changes

update

b545282

LongCatIsLooong force-pushed the deprecate-sliverfillviewport branch from 7770d61 to b545282 Compare August 16, 2019 19:59

Update page_view.dart

a485821

goderbauer added the waiting for tree to go green label Aug 21, 2019

LongCatIsLooong merged commit 9aea03f into flutter:master Aug 21, 2019

LongCatIsLooong deleted the deprecate-sliverfillviewport branch August 21, 2019 22:02

tvolkert mentioned this pull request Aug 22, 2019

stock_build_iteration microbenchmark regression #39060

Closed

hbali mentioned this pull request Oct 27, 2019

ontap not working when a page has index 1 best-flutter/flutter_swiper#28

Open

Hixie reviewed Nov 8, 2019

View reviewed changes

Hixie mentioned this pull request Nov 11, 2019

Remove RenderSliverFillViewport since it has no widget #44612

Closed

LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Nov 13, 2019

Revert "Implement PageView using SliverLayoutBuilder, Deprecate Rende…

a993d08

…rSliverFillViewport (flutter#37024)" This reverts commit 9aea03f.

LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Nov 15, 2019

Revert "Revert "Implement PageView using SliverLayoutBuilder, Depreca…

693dcfe

…te RenderSliverFillViewport (flutter#37024)" (flutter#44778)" This reverts commit 851d699.

pennzht mentioned this pull request Nov 21, 2019

PageView Cannot Scroll to Last Page #45339

Closed

LongCatIsLooong mentioned this pull request Nov 22, 2019

Use RenderSliverPadding to inset SliverFillViewport #45432

Merged

9 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

	child: SizedBox.expand(child: Text(index.toString())),
	child: SizedBox.expand(child: Text('$index')),

	final Offset center = tester.getCenter(find.text(index.toString())).translate(-3, 0);
	final Offset center = tester.getCenter(find.text('$index')) - const Offset(3, 0);

Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport #37024

Implement PageView using SliverLayoutBuilder, Deprecate RenderSliverFillViewport #37024

Conversation

LongCatIsLooong commented Jul 26, 2019 • edited Loading

Description

Breaking Changes

Related Issues

Tests

Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvolkert commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong commented Jul 26, 2019 •

edited

Loading

chunhtai Jul 30, 2019 •

edited

Loading