fix a table widget render bug by xu-baolin · Pull Request #76375 · flutter/flutter

xu-baolin · 2021-02-19T10:26:40Z

Re-work of #69670

See #69670 for more process details.

@Hixie Hi,
I admit that the solution looks a bit unconventional, but I think this is due to the peculiarities of the table widget and has to do it.
In fact, this is a bug with a very clear root cause, but we cannot solve it easily, the IndexedSlot is designed to solve the scene of this issue, but we cannot use this.
1, In order to be compatible with the old API, we cannot introduce IndexedSlot,
2, should not match keys at different rows also causes us to be unable to introduce the IndexedSlot

Looking forward to discussing a perfect solution.

xu-baolin · 2021-02-19T10:36:47Z

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

I have analyzed all the scenarios of inserting RO, it is safe to do so and can fix this bug

There are three situations where this function will be called,

In the _TableElement mount phase, _lastRemovedParentDataInThisFrame is null at this time;

The _TableElement update phase. For one row update, the void update(Table newWidget) will first insert the new RO, then remove the old RO, and then update the next Row. The first RO inserted coordinations must be the last deletion of the previous row(if any). The Updata will also call _updateRenderObjectChildren at the end, so the operation is safe.

In the scene of the issue problem, _lastRemovedParentDataInThisFrame will record the last delete the coordinates of RO. If there is an insertion in this frame, we can find the position to be inserted.

xu-baolin · 2021-02-25T01:34:08Z

@Hixie Hey, please take a look at this in case you have time, I think it will take your short time this time!
: )

Hixie · 2021-02-25T06:26:21Z

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

let's put this before the first method that uses it, to aid readability

Hixie · 2021-02-25T06:26:55Z

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

this wouldn't work for cases where this is done on a PipelineOwner that isn't being run by the binding...

Could you let me know in what scenarios the situation you mentioned will be used(or some documentations)?
I am not familiar with this and want to learn it.

Hixie · 2021-02-25T06:29:50Z

I admit this is a short solution. I'm not convinced it would work reliably (solutions involving passing information out-of-band like this and scheduling post-frame cleanup are always a bit suspicious), but it might be an improvement over the current code regardless.

Did you try using the slot at all? That still seems like the most likely reliable solution.

cc @goderbauer. I'm leaning towards us just landing this since it does fix the problem, though I'd probably want to make sure we filed a bug to revisit this and consider making Table work with a real slot in due course.

xu-baolin · 2021-02-25T06:49:54Z

I admit this is a short solution. I'm not convinced it would work reliably (solutions involving passing information out-of-band like this and scheduling post-frame cleanup are always a bit suspicious), but it might be an improvement over the current code regardless.

Did you try using the slot at all? That still seems like the most likely reliable solution.

cc @goderbauer. I'm leaning towards us just landing this since it does fix the problem, though I'd probably want to make sure we filed a bug to revisit this and consider making Table work with a real slot in due course.

I totally agree, nor do I think it’s a perfect solution.

In fact, the last PR #69670 tried to use the slot solution completely, but it might inevitably break something.
maybe we should give everyone a chance to think about how best to solve (and explain) a general solution.

Hixie · 2021-02-25T07:42:12Z

In fact, the last PR #69670 tried to use the slot solution completely

I meant using the X,Y version of slot (rather than the before/after version of slot like most multi-child widgets).

Hixie · 2021-02-25T07:43:29Z

If you are running into a problem with this bug in your app, it's probably fine to land this as-is (assuming @goderbauer agrees). It's better to have a fix than no fix and this fix is not particularly more complicated than what we have now.

xu-baolin · 2021-02-25T12:58:34Z

In fact, the last PR #69670 tried to use the slot solution completely

I meant using the X,Y version of slot (rather than the before/after version of slot like most multi-child widgets).

I will try it!

xu-baolin · 2021-02-25T14:05:30Z

yes, the x,y version slot seems to work! I will update the new solution ASAP

xu-baolin · 2021-02-26T06:06:29Z

@Hixie Hey,
I believe we could completely solve this bug this time : )

xu-baolin · 2021-02-26T06:10:33Z

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

  @override
  void insertRenderObjectChild(RenderObject child, dynamic slot) {
    renderObject.setupParentData(child);
+    if (slot is _TableSlot)


1, mount phase: slot is null
2, _TableElement.update phase: slot is IndexedSlot
3, This bug scenario： slot is _TableSlot

the #1 and #2 scenario are finally update to _TableSlot type.

I think the problem here is that we are using updateChildren and that uses _IndexedSlot. We probably need to create our own version of updateChildren that doesn't mess with the slot and does things by honoring the x,y coordinate, and then we can reliably use _TableSlot everywhere, including in mount.

Truly, I considered this matter. There is an intermediate state of indexedSlot that is not so perfect, but if we overrideupdateChildren, there may be a lot of duplicate code. Do you have any suggestions for this problem?

Another question, if we do this, there will be no way to distinguish the three scenarios here, because we cannot add RO through this method in the first two scenarios.

The current implementation uses void setFlatChildren to change the dimensions of RO._children, which is different from other multiChildRO.

Now I ignore the slot during updating, any thoughts? :)

@goderbauer Hi,
There are three scenarios that will call 'insertRenderObjectChild',

mount

update

The related issue scenario
If we initialize the slots in the mount and update phases to tableslot, we cannot distinguish between these three scenarios, because the current implementation does not allow us to insert RO through this function, and RO needs to be inserted only in the scenario of the issue. Otherwise, we need to refactor the 'RenderTable' again!

I was playing around with this a little bit, what do you think about the approach in https://github.com/flutter/flutter/pull/78081/files?

So appreciate that your being able to continue this work. I have left some comments there.

xu-baolin · 2021-03-01T00:53:19Z

Ping @Hixie : )

Hixie · 2021-03-02T00:50:15Z

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

+  int get hashCode => hashValues(x, y);
+
+  @override
+  String toString() => '${super.toString()}; "x" : "$x"; "y" : "$y"; }';


see https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#override-tostring for recommended boilerplate here

Will fix this.

goderbauer · 2021-03-08T19:32:50Z

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

  /// This function is a convenience wrapper around [updateChild], which updates
  /// each individual child. When calling [updateChild], this function uses an
-  /// [IndexedSlot<Element>] as the value for the `newSlot` argument.
+  /// [IndexedSlot<Element>] as the value for the `newSlot` argument if [ignoreSlot]


nit: document what it does if ignoreSlot is true.

goderbauer · 2021-03-08T19:33:11Z

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

+  /// is `false`.
  /// [IndexedSlot.index] is set to the index that the currently processed


nit: reflow this paragraph?

goderbauer · 2021-03-08T19:40:06Z

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

  @protected
-  List<Element> updateChildren(List<Element> oldChildren, List<Widget> newWidgets, { Set<Element>? forgottenChildren }) {
+  List<Element> updateChildren(
+    List<Element> oldChildren, List<Widget> newWidgets, {Set<Element>? forgottenChildren, bool ignoreSlot = false}


The ignoreSlot argument is a little awkward. Could RenderObjectElement have a createSlotForChild method that by default returns an IndexSlot and the Table could override that to return its own slot version? Or something like that?

Make sense.
Fixed as your suggestion!

goderbauer · 2021-03-09T21:29:05Z

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

+  // This class ignores the child's slot during [_TableElement.mount] and [_TableElement.update]
+  // phase, and then updates the slot to [_TableSlot] after completing the update of the child list.
+  @override
+  Object? createSlotForChild(int index, Element? previousChild) => null;


I was actually hoping that this could create the TableSlot?

goderbauer · 2021-03-09T21:31:38Z

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

@@ -370,7 +403,10 @@ class _TableElement extends RenderObjectElement {



In the mount above, we should be able to calculate the right TableSlot and just pass it into inflateWidget, no?

So, the only remaining problem is the use of updateChildren in update then? It looks like we could figure out the appropriate slot value for each child we pass to updateChildren right there. Maybe updateChildren could take an optional list of slots (with the same length as the newWidgets list) or a function that generates a slot based on the index of the widget - and when provided the slot from that list or function is used instead of the default IndexSlot?

#76375 (comment)

Please let me know if did not explain it clearly.

I am not sure I understand what you mean...

xu-baolin · 2021-03-17T01:01:53Z

Continue working by #78081

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 19, 2021

google-cla bot added the cla: yes label Feb 19, 2021

xu-baolin mentioned this pull request Feb 19, 2021

Refactor the RenderTable class #69670

Closed

xu-baolin requested a review from Hixie February 19, 2021 10:30

xu-baolin commented Feb 19, 2021

View reviewed changes

xu-baolin requested a review from goderbauer February 19, 2021 10:37

Hixie reviewed Feb 25, 2021

View reviewed changes

packages/flutter/lib/src/widgets/table.dart Outdated

Copy link

Contributor

Hixie Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's put this before the first method that uses it, to aid readability

Hixie reviewed Feb 25, 2021

View reviewed changes

apply _TableSlot

19016d0

xu-baolin force-pushed the 0219table branch from a52480c to 19016d0 Compare February 26, 2021 06:04

xu-baolin commented Feb 26, 2021

View reviewed changes

xu-baolin added 2 commits February 26, 2021 15:10

++

4903dd9

++

f5a9484

Hixie reviewed Mar 2, 2021

View reviewed changes

ignore the slot when call updateChildren

8616ffd

goderbauer reviewed Mar 8, 2021

View reviewed changes

xu-baolin added 3 commits March 9, 2021 08:06

Merge branch 'master' of github.com:flutter/flutter into 0226

ddc32b0

review feedback

e7815ba

++

9808bb0

goderbauer reviewed Mar 9, 2021

View reviewed changes

goderbauer mentioned this pull request Mar 16, 2021

Table widget rendering bug #78081

Merged

xu-baolin closed this Mar 17, 2021

		/// is `false`.
		/// [IndexedSlot.index] is set to the index that the currently processed

		@@ -370,7 +403,10 @@ class _TableElement extends RenderObjectElement {

Conversation

xu-baolin commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-baolin Feb 23, 2021 • 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!

xu-baolin commented Feb 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-baolin Feb 25, 2021 • 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!

Hixie commented Feb 25, 2021

Uh oh!

xu-baolin commented Feb 25, 2021

Uh oh!

Hixie commented Feb 25, 2021

Uh oh!

Hixie commented Feb 25, 2021

Uh oh!

xu-baolin commented Feb 25, 2021

Uh oh!

xu-baolin commented Feb 25, 2021

Uh oh!

xu-baolin commented Feb 26, 2021

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!

xu-baolin Mar 11, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-baolin commented Mar 1, 2021

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!

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!

xu-baolin commented Feb 19, 2021 •

edited

Loading

xu-baolin Feb 23, 2021 •

edited

Loading

xu-baolin Feb 25, 2021 •

edited

Loading

xu-baolin Mar 11, 2021 •

edited

Loading