Use RenderSliverPadding to inset SliverFillViewport #45432
Conversation
fluttergithubbot
added
the
framework
| } | ||
|
|
||
| /// A sliver that contains a multiple box children that each fill the viewport. | ||
| /// A sliver that contains a multiple box children that each fills the viewport. |
There was a problem hiding this comment.
| /// A sliver that contains a multiple box children that each fills the viewport. | |
| /// A sliver that contains multiple box children that each fills the viewport. |
| } | ||
|
|
||
| @override | ||
| EdgeInsetsGeometry get padding { |
There was a problem hiding this comment.
It seems odd to override the getter for padding like this. Could this just be a private method that gets called whenever we need to set the padding to obtain the value for the padding?
There was a problem hiding this comment.
Ended up exposing resolvedPadding instead of markNeedsResolution
LongCatIsLooong
force-pushed
the
sliver-fillviewport-fix
branch
from
e3f3c70 to
c9dab5c
Compare
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
LGTM after comments are resolved.
| /// its child. Any incoming [SliverConstraints.overlap] is ignored and not | ||
| /// passed on to the child. | ||
| /// | ||
| /// Applying padding to anything but the most mundane sliver is likely to have |
There was a problem hiding this comment.
Maybe use a macro here since it's the same block of text?
| } | ||
|
|
||
| @override | ||
| EdgeInsets resolvedPadding; |
There was a problem hiding this comment.
Can you turn this into a private variable with a public getter to ensure that the value cannot be changed from the outside?
| /// the viewport in the main axis. | ||
| final double viewportFraction; | ||
|
|
||
| /// The delegate that provides the children for this widget. |
There was a problem hiding this comment.
Maybe use a macro instead of repeating this text from SliverMultiBoxAdaptorWidget?
| } | ||
|
|
||
| assert(false, 'Unreachable'); | ||
| return EdgeInsets.symmetric(vertical: paddingValue); |
There was a problem hiding this comment.
The change is no longer necessary because the logic is moved to _resolve. But I wonder what the benefits of returning EdgeInsets.zero are?
| expect(tester.getTopLeft(find.text('Hawaii')), const Offset(-(4 - 1) * 800 / 2, 0)); | ||
| }); | ||
|
|
||
| // Regression test for https://github.com/flutter/flutter/issues/45096. |
There was a problem hiding this comment.
nit: I believe our style is to make this the first line inside the test body.
|
|
||
| // Regression test for https://github.com/flutter/flutter/issues/45096. | ||
| testWidgets( | ||
| 'PageView large viewportFraction can scroll and to the last page and snap', |
There was a problem hiding this comment.
nit: there seems to be an extra "and" in this sentence?
| expect(tester.getCenter(find.text('2')), const Offset(400, 300)); | ||
| }); | ||
|
|
||
| testWidgets( |
There was a problem hiding this comment.
Is there an issue that you could mention here for which this is the regression test?
| } | ||
|
|
||
| @override | ||
| EdgeInsets get resolvedPadding { |
There was a problem hiding this comment.
I wonder if its beneficial to performance to cache the value like we do in RenderSliverPadding?
LongCatIsLooong
force-pushed
the
sliver-fillviewport-fix
branch
from
18b763c to
ad7d924
Compare
4 participants
Description
This PR reimplements
SliverFillViewportin a similar vein to the reverted PR #37024.Related Issues
Fixes #23873
Fixes #27744
Fixes #45096
Tests
I added the following 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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?