SliverLayoutBuilder by LongCatIsLooong · Pull Request #35941 · flutter/flutter

LongCatIsLooong · 2019-07-10T23:24:44Z

Description

Introduce SliverLayoutBuilder.

Related Issues

Closes #35927

Tests

I added the following tests:

SliverLayoutBuilder versions of most LayoutBuilder tests

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?

No, this is not a breaking change.

chunhtai · 2019-07-15T21:13:48Z

A thought I have after look at the pr, instead of doing the inherit structure, you can try to wrap Layoutbuilder by using SliverConstraints.asBoxConstraints to convert the constraints that passed down to builder and RenderSliverToBoxAdapter to convert the renderbox back to rendersliver. This should make it a SliverLayoutbuilder.

chunhtai

Overall looks good, I leave some minor comment.
I do not have enough background on this. Can you explain more how this can help we solve #23873? What are the obstacle we facing if we don't have SliverLayoutBuilder?

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

chunhtai · 2019-07-18T22:40:52Z

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

+  void applyPaintTransform(RenderObject child, Matrix4 transform) {
+    assert(child != null);
+    assert(child == this.child);
+    // No-op because child's offset is always (0, 0),


We don't use the word 'no-op' in our documentation. maybe 'It does not require transformation because..."

packages/flutter/test/widgets/layout_builder_and_global_keys_test.dart

packages/flutter/test/widgets/layout_builder_mutations_test.dart

chunhtai · 2019-07-18T22:47:54Z

packages/flutter/test/widgets/layout_builder_test.dart

+    final RenderBox renderChild1 = tester.renderObject(find.byKey(childKey1));
+    final RenderBox renderChild2 = tester.renderObject(find.byKey(childKey2));
+
+    // scrollController.scrollOffset = 0:


end with dot

packages/flutter/test/widgets/layout_builder_test.dart

…tter into sliver-layoutbuilder

chunhtai · 2019-07-19T18:09:16Z

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

+typedef SliverLayoutWidgetBuilder = Widget Function(BuildContext context, SliverConstraints constraints);
+
+// A widget that defers its building until layout.
+abstract class _GenericLayoutBuilder<ConstraintType extends Constraints> extends RenderObjectWidget {


after talked with @dnfield offline, it feels strange that we have public class inherit from private class, let's make this class public. In that class, the build property can documented here instead of duplicated in two subclass.

Perhaps this could be called ConstrainedLayoutBuilder

Also exposed RenderConstrainedLayoutBuilder for subclassing. _LayoutBuilderElement should be generic enough to stay private.

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

dnfield

The parts I looked at LGTM, I'll defer to @chunhtai to finish the rest of his review

goderbauer · 2019-07-24T12:17:31Z

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

    if (child != null)
      context.paintChild(child, offset);
  }
+


Maybe outsource the sliver stuff in a separate sliver_layout_builder.dart file?

goderbauer · 2019-07-24T12:21:56Z

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

+mixin RenderConstrainedLayoutBuilder<ConstraintType extends Constraints, ChildType extends RenderObject> on RenderObjectWithChildMixin<ChildType> {
+  LayoutCallback<ConstraintType> _callback;
+  /// Change the layout callback.
+  set callback(LayoutCallback<ConstraintType> value) {


Having a setter without a getter is strange...

goderbauer · 2019-07-24T12:35:47Z

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

+  }) : super(key: key, builder: builder);
+
+  @override
+  LayoutWidgetBuilder get builder;


Maybe I am missing something, but how does this override work? It doesn't have a body and the class is not marked abstract?

Woah I didn't realize that and it passed the tests. Turns out this is expected, according to the language spec:

section: 16.19:

Let m be an identifier, o an object, L a library, and C a class which is the
class of o or a superclass thereof.
...
The result of a getter lookup for m in o with respect to L starting in class
C is the result of a getter lookup for m in C with respect to L. The result of a
getter lookup for m in C with respect to L is: If C declares a concrete instance
getter named m that is accessible to L, then that getter declaration is the result
of the getter lookup, and we say that the getter was looked up in C. Otherwise,
if C has a superclass S, the result of the getter lookup is the result of a getter
lookup for m in S with respect to L. Otherwise, we say that the getter lookup
has failed.

section 10.4:

The purpose of an abstract method is to provide a declaration for purposes
such as type checking and reflection. In mixins, it is often useful to introduce
such declarations for methods that the mixin expects will be provided by the
superclass the mixin is applied to.
We wish to detect if one declares a concrete class with abstract members.
However, code like the following should work:

class Base { int get one => 1; } abstract class Mix { int get one; int get two => one + one; } class C extends Base with Mix { }

At run time, the concrete method one declared in Base will be executed, and
no problem should arise. Therefore no error should be raised if a corresponding
concrete member exists in the hierarchy.

added implementation.

goderbauer

LGTM

goderbauer · 2019-07-25T07:45:49Z

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

 import 'debug.dart';
 import 'framework.dart';

+export 'sliver_layout_builder.dart';


This export should probably be in flutter/lib/widgets.dart instead.

goderbauer · 2019-07-25T07:46:38Z

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


-  @override
-  _RenderLayoutBuilder createRenderObject(BuildContext context) => _RenderLayoutBuilder();
+  /// Called at layout time to construct the widget tree. The builder must not


nit: the first paragraph of a doc comment should be a one-sentence summary.

chunhtai

LGTM

chunhtai · 2019-07-25T22:04:51Z

packages/flutter/test/widgets/layout_builder_test.dart

+    expect(parentSliver.geometry.scrollExtent, childHeight);
+    expect(parentSliver.geometry.paintExtent, childHeight);
+
+    // When child is over-sized.


half sentence

LongCatIsLooong added 6 commits July 8, 2019 19:28

wip

9d2b125

adding tests

8c42ab9

more tests

92d2587

more tests

7d61b60

fix hit testing

21ac84e

docs

4d12260

googlebot added the cla: yes label Jul 10, 2019

LongCatIsLooong requested a review from goderbauer July 10, 2019 23:24

LongCatIsLooong added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jul 10, 2019

indentation

b381e07

LongCatIsLooong requested review from HansMuller and chunhtai July 15, 2019 17:51

chunhtai reviewed Jul 18, 2019

View reviewed changes

LongCatIsLooong added 2 commits July 19, 2019 10:06

explicitly verify no exceptions were thrown

6c5a538

Merge branch 'sliver-layoutbuilder' of github.com:LongCatIsLooong/flu…

1481e27

…tter into sliver-layoutbuilder

chunhtai reviewed Jul 19, 2019

View reviewed changes

dnfield reviewed Jul 19, 2019

View reviewed changes

packages/flutter/lib/src/widgets/layout_builder.dart Outdated Show resolved Hide resolved

dnfield reviewed Jul 19, 2019

View reviewed changes

packages/flutter/lib/src/widgets/layout_builder.dart Outdated Show resolved Hide resolved

LongCatIsLooong added 4 commits July 19, 2019 11:41

Exposing the generic superclass

493302c

exposing superclass

12ced67

exposing more stuff

9f3ced4

docs change

3832d2b

LongCatIsLooong requested review from chunhtai and dnfield July 19, 2019 20:50

dnfield reviewed Jul 19, 2019

View reviewed changes

goderbauer reviewed Jul 24, 2019

View reviewed changes

review

9a00c51

goderbauer approved these changes Jul 25, 2019

View reviewed changes

review

6f6f4bb

chunhtai approved these changes Jul 25, 2019

View reviewed changes

Update layout_builder_test.dart

d4ef875

LongCatIsLooong merged commit 9c8badd into flutter:master Jul 26, 2019

LongCatIsLooong deleted the sliver-layoutbuilder branch July 26, 2019 17:39

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Add SliverLayoutBuilder (flutter#35941)

70b1a4b

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

Conversation

LongCatIsLooong commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

chunhtai commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

dnfield 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!

LongCatIsLooong Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

section: 16.19:

section 10.4:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer 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!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LongCatIsLooong commented Jul 10, 2019 •

edited

Loading

chunhtai commented Jul 15, 2019 •

edited

Loading

chunhtai left a comment •

edited

Loading

LongCatIsLooong Jul 24, 2019 •

edited

Loading