This is an optimization of duplicate check logic
The List operator == only equal to themselves.

Have you verified that this is actually faster? It trades a pointer comparison for an O(N) comparison during updates to avoid an O(N) cost in layout, but it's not clear to me how common it would be for the child list to be a new instance but holding the same contents.

Currently, the caller of setFlatChildrenalways generate a new List instant so this is a dead code never reachable.

Next, let's analyze different scenarios:

1. Update TableRow widgets without changing its Type, so the cells parameter will have the same renderObjects and Will achieve optimized results.
2. Change the TableRow widgets count will have no effect because listEquals will compares length first.
3. Update ``TableRow` widgets with a different renderObjectWidget and keep the same count，like you said will increase the time complexity.

In summary, I believe that the benefits of such a modification are greater than the costs, Of course, also need to listen to your suggestions ：）

If we always call with a new list, then this is converting an O(1) equality test with an O(N) equality test, which is slower, in order to avoid the code below, which is O(N) (but when avoided becomes O(1), so faster). This seems like it should be better... but my question is, is it?

The way to find out would be to benchmark it.

It appears that you don't really need an IndexedSlot here. Wouldn't it be more convenient to just define a _TableSlot with an x and y property representing the row/column of the child?

(If we do that, I am wondering whether we still need the TableCellParentData, which contains the same information and appears to only be used in removeRenderObjectChild)

In this bug scenario, we must use IndexedSlot, we need the position info to decide where to insert the new RO.

The comment on line 287 about slots not being used seems outdated?

Removed.

Now that slots have meaning, do we need an implementation for moveRenderObjectChild?

Nice catch, Under your reminder, I found some bugs, the current implementation is a bit strange, I need to continue working on this.

Why remove children here? Most RenderObjects with children have an option to specify them in the constructor.

Also: update the doc comment above.

comes back again.

Why's there no setter for columns/rows anymore? Since it is a constructor argument, you should be able to change it after the object is constructed.

Can be changed by setDimensions

This makes it sound as if there's only one dimension to the children. However, it's not a linear list.

Maybe expand the comment to describe what the model for the children is here?

Or can we change this to an API that's more explicit about the actual child model, maybe something like:

void insertChild(RenderBox child, {required int row, required int column});

Since the element has access to the index of the child, could it use that to calculate what column and row the child needs to be in?

Updated the comment below.

Now, _children is variable length, use row or column is not suitable.

nit: this name is very generic.

Maybe call this setDimensions? And move it closer to the column and row getter? Also update the docs of those getters to link to this method.

Also document that all the children need to be inserted first before calling this.

Done.

Same comment as on insertChild.

maybe turn the removed row.key == null condition into an assert here.

It's possible that add a new keyed row here,
It’s a bit difficult to handle here because we have to ensure that one-time update of children.

import 'package:flutter/material.dart';

void main() {
  runApp(
    MaterialApp(
      home: Center(
        child: SizedBox(
          width: 100,
//          height: 100,
          child: W(),
        ),
      ),
    ),
  );
}

class W extends StatefulWidget {
  const W({Key key}) : super(key: key);

  @override
  _State createState() => _State();
}

class _State extends State<W> {
  void _onTap() {
    setState(() => _toggle = !_toggle);
  }

  List<TableRow> _getRows() {
    if (_toggle) {
      return [
        TableRow(key: Key('1'), children: [Text('ke1')]),
        TableRow(children: [Text('unkey')]),
        TableRow(key: Key('2'), children: [Text('ke2')]),
      ];
    } else {
      return [
        TableRow(key: Key('2'), children: [Text('ke2')]),
//        TableRow(key: Key('1'), children: [Text('ke1')]),
        TableRow(children: [Text('unkey')]),
        TableRow(key: Key('3'), children: [Text('ke3')]),
      ];
    }
  }

  bool _toggle = true;
  @override
  Widget build(BuildContext context) {
    return Column(
      children: [
        GestureDetector(onTap: _onTap, child: Text('Tap me')),
        Table(
          children: _getRows(),
        )
      ],
    );
  }
}

For the code above, I think the old key1 should be dropChild, and then the new key3 should be adoptChild.
But our current processing is to reuse the old RO and I think there will be no problem with this processing(the real RO(Text) can be reused), what do you think?

Probably the TableRow should not have a local key I think.

We can probably do this smarter since we know what has been taken. Maybe above in line 412/413 instead of adding them to the extra "taken" list just remove them from oldKeyedRows, so in the end oldKeyedRows contains everything that's not taken?

Done

since row isn't used in this for loop maybe just fo for (int row = 0; row < rows; row++)?

Done

Why do we allow child to be null? What does it mean here to insert a null-child?

The unit test before this change inserts a lot of null-child into _children, Which is not allowed now.

This is odd, what does this mean?

(here and elsewhere)

No longer allow the existence of null RO for _children

No longer allow the existence of null RO for _children

isn't that a problem?

Refer to #69670 (comment)

this seems concerning, there shouldn't be any logic changes right?

https://github.com/flutter/flutter/pull/69670/files#r565986835

aah, i understand now. this is literally the change this PR is about. Sorry I misunderstood the test before.

@Hixie
It’s weird to deal with it this way. We should not recreate(drop and new create) the cell elements because their keys and types have not changed, right?
For example, We should not rebuild element and RO in the following situation，which you concerning
https://github.com/flutter/flutter/pull/69670/files/60b124b9a86717f4e0aeaf203e23c8d0c1e3944e#r565894797
before rebuild: [Text('A'), Text('B'), Text('C'), Text('D'), Text('E'), ] with (column:3, row:2)
after update: [Text('a'), Text('b'), Text('c'), Text('d'), Text('e'), ] with (column:2, row:3)

Ignore the child slot is the root cause of the related issue.

i agree that it would be better to not replace the elements in that case

After the slot is introduced, we can't add RO children here uniformly, we can only add children to RO through void insertRenderObjectChild.

This is exactly why we need to refactor the render. I don't want to modify it on a large scale, but we have to. RenderTable is really different from other similar multi-child RO implementations. I don't know if the author at the time had a special purpose. My modification is to make its implementation not so special, and then the issue is solved naturally.

@Hixie

If we want to use the slot index, we must update all the children (call updateChildren()) one-time, otherwise, the calculation of the slot is wrong.

This is why make the change here.