Use AnimatedSwitcher's _childNumber as Key in layoutBuilder's Stack

[jimgerth]

Previously KeyedSubtree.wrap gave children of the AnimatedSwitcher their own key as a key, if available, and their _childNumber as a key, if not. This could lead to multiple children having the same key in the defaultLayoutBuilder's Stack and thus a duplicate key exception being thrown, though (see #121336 for a detailed description of how this bug occurs). This is fixed by always using the uniquely identifiable _childNumber as a key for the children of the AnimatedSwitcher.

This does not impair the functionality of having children of the same type be differentiated and transitioned between by giving them different keys, as the children's original key is still used to determine if a transition should take place or not.

This fixes #121336.

Note: This reverts the changes introduced by #107476, as they do not completely fix the issue of duplicate keys in the AnimatedSwitcher switcher as demonstrated by #121336, and introduce behavior that deviates from the AnimatedSwitcher's documentation.
Specifically, children that are transitioning out are immediately removed from the widget tree, if another child with the same key is added, while the AnimatedSwitcher is clearly supposed to support multiple children with the same key transitioning in and out at the same time according to its documentation:

The same key can be used for a new child as was used for an already-outgoing child; the two will not be considered related. (For example, if a progress indicator with key A is first shown, then an image with key B, then another progress indicator with key A again, all in rapid succession, then the old progress indicator and the image will be fading out while a new progress indicator is fading in.)

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[goderbauer]

@jimgerth Can you please sign the CLA (see previous message) and add a test for your change? Also, it looks like some other checks are failing as well. Please take a look at those as well. Thanks.

[jimgerth]

@goderbauer I have signed the CLA now and will add a test for my change once I have time.

Also it seems that the test failing currently is the one introduced by #107476, fixing a related issue of duplicate keys in the AnimatedSwitcher in what seems to me a non-optimal way.

Children, that are transitioning out are simply completely removed from the widget tree, if another child with the same key is added. As the test's name introduced by that PR implies:

AnimatedSwitcher does not duplicate animations if the same child is entered twice.

(And this does not even completely eliminate that problem of duplicate keys, as shown by #121336, which is why this PR exists...)

However this is not the expected behaviour of the AnimatedSwitcher as described in its documentation:

The same key can be used for a new child as was used for an already-outgoing child; the two will not be considered related. (For example, if a progress indicator with key A is first shown, then an image with key B, then another progress indicator with key A again, all in rapid succession, then the old progress indicator and the image will be fading out while a new progress indicator is fading in.)

It clearly states that multiple children with the same key can transition in and out at the same time.

My question now is: Am I allowed to remove this test, as it tests for "wrong" behavior? Or how should I proceed?

[jimgerth]

I've written with @Hixie on discord and they told me to change (or replace in this case) the old test.
I eventually decided to entirely revert the previous PR #107476. See my previous comment, the PR description and the commit message for my reasoning.

[goderbauer]

LGTM, thanks for the fix!

[jimgerth]

@goderbauer My pleasure :)

Could you add the autosubmit tag, as I don't have write access to do that myself?

Also, rebased onto master as suggested by the wiki entry on Understanding Google Testing, as the Google Testing job was stuck for at least 16 hours.

[auto-submit]

auto label is removed for flutter/flutter, pr: 121408, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[auto-submit]

auto label is removed for flutter/flutter, pr: 121408, due to Validations Fail.

[christopherfujino]

auto label is removed for flutter/flutter, pr: 121408, due to Validations Fail.

In cl/512895929 there's a slight goldens diff that could possibly be due to this change, WDYT @goderbauer?

[goderbauer]

In cl/512895929 there's a slight goldens diff that could possibly be due to this change

I triaged them as acceptable.