Skip to content

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Mar 25, 2021

...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

  • 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 feature I am adding, or Hixie said the 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 flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Mar 25, 2021
@google-cla google-cla bot added the cla: yes label Mar 25, 2021
@Hixie Hixie mentioned this pull request Mar 25, 2021
8 tasks
@Hixie
Copy link
Contributor Author

Hixie commented Mar 31, 2021

Ok I think the new test failures are because the code that called enterText was previously relying on the fact that we always registered the testTextInput even when we didn't want to, and for some reason we assert that we have done so. I'll remove the asserts.

@Hixie Hixie force-pushed the reland_clean_text_input branch 2 times, most recently from 8da36bc to 2ffd137 Compare March 31, 2021 22:35
@Hixie
Copy link
Contributor Author

Hixie commented Apr 2, 2021

New problem: the client ID doesn't match what the text field is expecting, so the text field ignores it...

@Hixie Hixie force-pushed the reland_clean_text_input branch 2 times, most recently from 0e5f3eb to 1a14886 Compare April 2, 2021 19:40
@Hixie
Copy link
Contributor Author

Hixie commented Apr 2, 2021

cc @LongCatIsLooong @justinmc

@Hixie Hixie marked this pull request as ready for review April 2, 2021 20:39
@Hixie
Copy link
Contributor Author

Hixie commented Apr 2, 2021

cc @dnfield @goderbauer in case you're interested in seeing the differences since the last version

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Comment on lines +547 to +548
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 👍

Copy link
Contributor

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.

Copy link
Contributor Author

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...

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor

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.

@Hixie Hixie force-pushed the reland_clean_text_input branch from 1a14886 to 87b2529 Compare April 2, 2021 22:54
@Hixie
Copy link
Contributor Author

Hixie commented Apr 2, 2021

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).

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux framework_tests_misc has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac framework_tests_misc has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows framework_tests_misc has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite framework_tests-libraries-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite framework_tests-misc-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

@Hixie Hixie force-pushed the reland_clean_text_input branch from 87b2529 to 961a963 Compare April 3, 2021 00:44
@Hixie
Copy link
Contributor Author

Hixie commented Apr 3, 2021

Updated again, I conflicted with @LongCatIsLooong's revert of his revert. :-)
Hopefully it lands this time. :-)

@fluttergithubbot fluttergithubbot merged commit b070ed3 into flutter:master Apr 3, 2021
@Hixie Hixie mentioned this pull request Apr 5, 2021
8 tasks
justinmc added a commit that referenced this pull request Apr 7, 2021
renyou pushed a commit that referenced this pull request Apr 7, 2021
Hixie added a commit to Hixie/flutter that referenced this pull request Apr 21, 2021
...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.
Hixie added a commit to Hixie/flutter that referenced this pull request May 14, 2021
* 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.
@Hixie Hixie mentioned this pull request May 14, 2021
8 tasks
Hixie added a commit to Hixie/flutter that referenced this pull request May 21, 2021
* 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.
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 c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants