-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Added a composable waitForCondition Driver/extension API #37736
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
| final Map<String, String> jsonMap = <String, String>{ | ||
| 'conditionName': 'CombinedCondition' | ||
| }; | ||
| final List<Map<String, String>> jsonConditions = <Map<String, String>>[]; |
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.
Could the following few lines be simplified by using List.map? e.g. conditions.map((WaitCondition c) => ...);
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.
The deserialization logic may also simplify with something like that.
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.
Simplified serialize(). Kept the deserialize() the same, as I didn't find a good solution to handle the type casting from the json.decode() resultdynamic => Map<String, dynamic>...
| bool get condition => SchedulerBinding.instance.transientCallbackCount == 0; | ||
|
|
||
| @override | ||
| Future<void> wait() async { |
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'd still argue that the default is to wait until the next frame if the condition is currently unmet. FirstFrameRasterizedCondition can still override that to provide a custom impl
|
Ping for comments? |
|
Is there a link to the breaking change announcment for this? |
dnfield
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.
Sorry for the late review - most of these should be small.
Thanks for checking! Will work on the announcement. |
dnfield
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, but we should wait until some time after an announcement has been posted to make sure this doesn't break anyone too badly.
|
For sure. I'm going to send an announcement tomorrow, but this change probably won't break any Flutter users, except Espressso :), as Espresso calls the extension APIs directly. |
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
Please link to the breaking change announcement.
|
This broke a bunch of tests on Flutter's devicelab. Please revert. |
|
Oh, sorry for the breakage! Working on a revert. |
…tter#37736)" This reverts commit 8edc3f7.
This reverts commit 0e8ead0.
Description
This PR added a waitForCondition API that can be used to compose conditions that the client would like to wait for. Currently supported conditions are:
In favor of this change, we deprecate the
_waitUntilNoTransientCallbacks,_waitUntilNoPendingFrame,_waitUntilFirstFrameRasterizedextension APIs.Related Issues
#37409
Tests
I added the following tests:
flutter_driver_test.dart
extension_test.dart
wait_test.dart
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?
It deprecates a few Flutter Driver extension wait APIs in favor of the new
waitForConditionAPI:_waitUntilNoTransientCallbacks&WaitUntilNoTransientCallbackscommand;_waitUntilNoPendingFrame&WaitUntilNoPendingFramecommand;_waitUntilFirstFrameRasterized&WaitUntilFirstFrameRasterizedcommand.Note, the user facing Driver API
waitUntilNoTransientCallbacks,waitUntilFirstFrameRasterizedare NOT deprecated, but under the hood, it useswaitForConditionto talk to the Dart service extension API. Thus, we expect minimum user impact unless they use the extension APIs directly (e.g. a testing framework like Espresso/EarlGrey).