ignore sliver underflow if the last children is no longer at the prev… #71864
Conversation
| @@ -1185,7 +1185,10 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render | |||
|
|
|||
| renderObject.debugChildIntegrityEnabled = false; // Moving children will temporary violate the integrity. | |||
| newChildren.keys.forEach(processElement); | |||
| if (_didUnderflow) { | |||
| final int newLastIndex = _childElements.lastKey() ?? -1; | |||
| // We don't worry about underflow if the last child is not longer at the | |||
goderbauer
Dec 8, 2020
Member
Why do we not worry about underflow in this condition?
Why do we not worry about underflow in this condition?
chunhtai
Dec 8, 2020
Author
Contributor
I am not entirely sure about the reason why we have the underflow logic, I can only guess by how it is used.
It seems like render sliver list set it to true when it think it has reached the end of the child list. Thus, if the children are reordered, the previous last child is moved to other place, and we no longer at the end of the child list, we should ignore this request.
Am I missing something?
I am not entirely sure about the reason why we have the underflow logic, I can only guess by how it is used.
It seems like render sliver list set it to true when it think it has reached the end of the child list. Thus, if the children are reordered, the previous last child is moved to other place, and we no longer at the end of the child list, we should ignore this request.
Am I missing something?
chunhtai
Dec 15, 2020
Author
Contributor
underflow is there to ensure children rebuild will update max scroll offset
for example if you have a list of 10 items, and you scrolled to the end of the list.
If we add an item to the end of the list, the sliver element rebuild will not change its child elements because it will only update its existing children. Since the children does not change during the rebuild, we will skip layout phase. Thus the max scroll offset will not get update, user will not be able to scroll to the 11th child.
That is why we proactively build one more child at the end to ensure the max scroll offset is updated.
This logic is not needed when "any" of the children gets updated, because it will trigger a layout. I will update the logic
underflow is there to ensure children rebuild will update max scroll offset
for example if you have a list of 10 items, and you scrolled to the end of the list.
If we add an item to the end of the list, the sliver element rebuild will not change its child elements because it will only update its existing children. Since the children does not change during the rebuild, we will skip layout phase. Thus the max scroll offset will not get update, user will not be able to scroll to the 11th child.
That is why we proactively build one more child at the end to ensure the max scroll offset is updated.
This logic is not needed when "any" of the children gets updated, because it will trigger a layout. I will update the logic
|
A friendly bump |
|
Hi @goderbauer can you take another look at this pr? |
| @@ -2,6 +2,7 @@ | |||
| // Use of this source code is governed by a BSD-style license that can be | |||
| // found in the LICENSE file. | |||
|
|
|||
| import 'package:collection/collection.dart'; | |||
goderbauer
Jan 28, 2021
Member
Are you importing this only for ListEquals? use listEquals from flutter/foundation.dart instead.
Are you importing this only for ListEquals? use listEquals from flutter/foundation.dart instead.
| @@ -1185,7 +1188,8 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render | |||
|
|
|||
| renderObject.debugChildIntegrityEnabled = false; // Moving children will temporary violate the integrity. | |||
| newChildren.keys.forEach(processElement); | |||
| if (_didUnderflow) { | |||
| // We don't worry about underflow if any child has been updated. | |||
goderbauer
Jan 28, 2021
Member
This comment should include an explanation why the underflow doesn't matter if children get updated.
This comment should include an explanation why the underflow doesn't matter if children get updated.
|
thanks @goderbauer ! i addressed all comments |
|
LGTM |
| @@ -1185,7 +1188,16 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render | |||
|
|
|||
| renderObject.debugChildIntegrityEnabled = false; // Moving children will temporary violate the integrity. | |||
| newChildren.keys.forEach(processElement); | |||
| if (_didUnderflow) { | |||
| // A element rebuild only updates existing children. The underflow check | |||
goderbauer
Feb 3, 2021
Member
A element -> An element
A element -> An element
|
This pull request is not suitable for automatic merging in its current state.
|
a65ce5b
into
flutter:master
…ious last index
Description
When sliver underflow, it will try to build one more child after the current last child. If the last child got moved to other place, the sliver will still try to build one more child after the new location. This PR fixes it.
Open Question: is the current underflow logic still valid? the sliver render object marks sliver element underflow, but it only gets picked on the next rebuild. if the sliver does not rebuild, nothing will change. and if the sliver does rebuild, the information may be outdated due to children reorder. I am not familiar with the underflow to make an improvement. Any advice?
Related Issues
fixes #71273
Tests
I added the following tests:
see files
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
Did any tests fail when you ran them? Please read Handling breaking changes.