-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Refactor text editing test APIs #79061
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
|
Ok I think the new test failures are because the code that called |
8da36bc to
2ffd137
Compare
|
New problem: the client ID doesn't match what the text field is expecting, so the text field ignores it... |
0e5f3eb to
1a14886
Compare
|
cc @dnfield @goderbauer in case you're interested in seeing the differences since the last version |
justinmc
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 👍
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.
Sounds reasonable.
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.
Should the selection be set to the end of the text?
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.
@LongCatIsLooong is doing that in #79506 but it got reverted due to google3 breakage.
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.
Ah I thought that sounded familiar 👍
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.
Nit: Alternatively you could use a group and do the incrementing in a setUp. Fine either way though.
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 still live in hope that we can destroy group and setUp and so on...
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 think it's way more readable if you just have the code running bit by bit rather than having to know what order setUp/group/etc run in, and what the implications of all those methods are, Zones, how it impacts futures, etc)
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.
Ah I hadn't realized anyone was anti-group/setUp, I've been using them just because they were there. I'll think about this next time I encounter it.
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 still can't think of a better way than this magic -1 thing. It doesn't seem too bad.
1a14886 to
87b2529
Compare
|
Thanks for the review! I've marked this so it'll land on green (had to fix a merge conflict due to the revert of @LongCatIsLooong's PR). |
|
This pull request is not suitable for automatic merging in its current state.
|
87b2529 to
961a963
Compare
|
Updated again, I conflicted with @LongCatIsLooong's revert of his revert. :-) |
...and remove a legacy TODO whose referenced issue (flutter#51885) is now closed. To actually fix this required some refactoring of the text editing test APIs. See flutter#77454 and flutter#79061 for earlier discussions. This was last reverted in flutter#79559.
* Moves the handlePlatformMessage logic to using ChannelBuffers. * Moves the testing APIs from services library to flutter_test. * Refactors text editing test APIs. * This is a reland of flutter#76288 and flutter#78637. See flutter#77454, flutter#79061, flutter#80003, flutter#81235, and flutter#82057 for earlier discussions. This was last reverted in flutter#82525.
* Moves the handlePlatformMessage logic to using ChannelBuffers. * Moves the testing APIs from services library to flutter_test. * Refactors text editing test APIs. * This is a reland of flutter#76288 and flutter#78637. See flutter#77454, flutter#79061, flutter#80003, flutter#81235, and flutter#82057 for earlier discussions. This was last reverted in flutter#82525.
...and remove a legacy TODO whose referenced issue (#51885) is now closed.
To actually fix this required some refactoring of the text editing test APIs.
See #77454 for original discussion.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.