Skip to content

Conversation

@pdblasi-google
Copy link
Contributor

@pdblasi-google pdblasi-google commented May 18, 2023

  • 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

Closes #123634
Closes #115874

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.

@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels May 18, 2023
@pdblasi-google pdblasi-google requested a review from goderbauer May 18, 2023 22:33
@pdblasi-google
Copy link
Contributor Author

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 toString issue had a reason, there isn't a way to do custom formatting of the Actual part of the expect call without using toString, but I did add a parameter to the toString method to be able to actually describe just the finder itself, and the results are where that description is created.

Copy link
Member

@bartekpacia bartekpacia left a 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 :)

@github-actions github-actions bot added the f: routes Navigator, Router, and related APIs. label Jul 21, 2023
@goderbauer
Copy link
Member

nit: looks like the analyzer is unhappy about a space somewhere...

Copy link
Member

@goderbauer goderbauer left a 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!

Copy link
Member

@goderbauer goderbauer left a 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.

@goderbauer
Copy link
Member

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:

  1. When it comes to renaming things, we can do the rename deprecation first (e.g. rename Finder to WidgetFinder, add a deprecated typedef) in a separate PR upfront.
  2. We can chose to not introduce a replacement API at all (e.g. decide that Finder is a fine name and not introduce WidgetFinder).
  3. We can chose to delay introducing the replacement to a future point in time (e.g. keep Finder for now and then in a follow-up rename it to WidgetFinder, we just need to accept that that may never happen, but we will not end up in a two APIs state).
  4. ...

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:

  1. How many lines of code need to be migrated in our own repository? In google3? For out customer tests, including the packages repository?
  2. Can these deprecations be auto-migrated by dart fix?
  3. How would one migrate to the new APIs?
  4. What is the developer facing benefit for doing these migrations?

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:

  • Finder -> WidgetFinder seems to have a rather low return on investment. Sure, we should have used that name from the beginning. If almost nobody is affected by this rename we can still do it, but if it affects hundreds of callsides outside of our direct control I would lean away from it.

  • Finder.evaluate is called almost 3,000 times in google3. That's a lot of code that needs to be migrated. I am wondering if we instead should save everyone the trouble and design the new capabilities around this exiting API.

@goderbauer
Copy link
Member

Copy link
Member

@goderbauer goderbauer left a 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`
@pdblasi-google pdblasi-google added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2023
@auto-submit auto-submit bot merged commit 5df1c99 into flutter:master Aug 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 11, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 11, 2023
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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add SemanticsFinder API for searching the Semantics tree Improve Finder APIs

3 participants