-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Adds SemanticsNode Finders for searching the semantics tree #127137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc: @bartekpacia Here's the PR that cleans up the Finder API (as well as pulled out a generic base class for it. Turns out the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some small comments. Overall it looks really nice and I'm happy my suggestions from #115874 turned out to be useful :)
d78ace4 to
bd22699
Compare
|
nit: looks like the analyzer is unhappy about a space somewhere... |
730e316 to
dd4b3f1
Compare
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming together nicely now!
4846d53 to
ef60106
Compare
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a technical perspective this looks great. I do have some concerns about the deprecations and how we are going to handle them, I'll leave another top level comment about them.
|
About the deprecations: I am very much against introducing replacement APIs without deprecating the old ones - even if this state is just intended to last a short period of time. Having two sets of APIs without clear marking which one is supposed to be used is creating user confusion, we have seen this before. Furthermore, if we delay the deprecation we may realize too late that for one reason or another we cannot actually deprecate the old API and then we are stuck with both. Also, even when the intention is to follow up with the deprecations right away, intentions and plans change and then things get forgotten. I just recently fixed a "TODO: deprecate this" four years after the reason to deprecate it became true and had to do a bunch of extra clean-up because people continued to use the not-yet-deprecated property in their code since they had no better way of knowing that they weren't supposed to. I agree that it may not be practical to do all of these deprecations in this PR. For that, we have options. Some examples:
When it comes to deprecating a thing, we also need to evaluate whether the deprecation is worth it. We don't want to create too much churn for our developers without clear benefit for them. Right now, we enjoy there good will and they let use evolve the APIs to make them better. But if we - in their eyes - abuse that good will, people may become grumpy and less receptive to these kind of deprecations. To evaluate whether a depreciation is worthwhile, we should collect some data. For each deprecation it would be worthwhile to collect:
The last 3 points are what one would typically also cover in a migration guide. It is a good exercise to write this before doing the deprecations to make sure we have good reasons for it - and to illustrate that doing this is worthwhile. Lastly, some concrete thoughts on some of the deprecations proposed:
|
|
There's also some more guidance around this in our wiki: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes |
673319a to
d6a15ec
Compare
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Pulled `FinderBase` out of `Finder` * `FinderBase` can be used for any object, not just elements * Terminology was updated to be more "find" related * Re-implemented `Finder` using `FinderBase<Element>` * Backwards compatibility maintained with `_LegacyFinderMixin` * Introduced base classes for SemanticsNode finders * Introduced basic SemanticsNode finders through `find.semantics` * Updated some relevant matchers to make use of the more generic `FinderBase`
5bccb0a to
f943126
Compare
flutter/flutter@685141b...9b6945b 2023-08-11 [email protected] Roll Flutter Engine from 93e8901490e7 to 77dfeea40e10 (1 revision) (flutter/flutter#132389) 2023-08-11 [email protected] Roll Flutter Engine from 4e532b957225 to 93e8901490e7 (1 revision) (flutter/flutter#132381) 2023-08-11 [email protected] Roll Flutter Engine from 25afdb9b696d to 4e532b957225 (4 revisions) (flutter/flutter#132376) 2023-08-11 [email protected] Roll Flutter Engine from da23fb0d9a1d to 25afdb9b696d (1 revision) (flutter/flutter#132370) 2023-08-11 [email protected] Roll Flutter Engine from acd1bc5536ef to da23fb0d9a1d (2 revisions) (flutter/flutter#132367) 2023-08-11 [email protected] Roll Flutter Engine from 578a8e8aabf6 to acd1bc5536ef (1 revision) (flutter/flutter#132365) 2023-08-11 [email protected] Roll Flutter Engine from 18a71c031f5f to 578a8e8aabf6 (1 revision) (flutter/flutter#132364) 2023-08-11 [email protected] Update `dev/devicelab/**` to provide `--local-engine-host`. (flutter/flutter#132342) 2023-08-10 [email protected] Roll Flutter Engine from b019ac62f21f to 18a71c031f5f (2 revisions) (flutter/flutter#132347) 2023-08-10 [email protected] Roll Flutter Engine from 16b01b98af20 to b019ac62f21f (1 revision) (flutter/flutter#132341) 2023-08-10 [email protected] Update `flutter_tools/bin/*.(dart|sh)` to provide, if set, --local-engine-host. (flutter/flutter#132336) 2023-08-10 [email protected] Update application id and bundle id of a11y assessment app (flutter/flutter#132334) 2023-08-10 [email protected] Roll Flutter Engine from a9be77e6f475 to 16b01b98af20 (6 revisions) (flutter/flutter#132332) 2023-08-10 [email protected] Remove the fast reassemble / single widget reload feature (flutter/flutter#132255) 2023-08-10 [email protected] Analyze code snippets in integration_test docs (flutter/flutter#132314) 2023-08-10 [email protected] Adds SemanticsNode Finders for searching the semantics tree (flutter/flutter#127137) 2023-08-10 [email protected] TextField should correctly resolve provided style for material states (flutter/flutter#132330) 2023-08-10 [email protected] setState documentation (flutter/flutter#132090) 2023-08-10 [email protected] Roll Flutter Engine from ea7730c16301 to a9be77e6f475 (6 revisions) (flutter/flutter#132328) 2023-08-10 [email protected] Fix: use --web-launch-url and --web-hostname arguments in flutter drive (flutter/flutter#131763) 2023-08-10 [email protected] GridView sample code (flutter/flutter#131900) 2023-08-10 [email protected] Upgrade flutter packages. (flutter/flutter#132326) 2023-08-10 [email protected] TextPainter migration cleanup (flutter/flutter#132317) 2023-08-10 [email protected] An example of parentData usage. (flutter/flutter#131818) 2023-08-10 [email protected] Add hasInteractedByUser getter in FormField (flutter/flutter#131539) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
FinderBaseout ofFinderFinderBasecan be used for any object, not just elementsFinderusingFinderBase<Element>_LegacyFinderMixinfind.semanticsFinderBaseCloses #123634
Closes #115874
Pre-launch Checklist
///).