Skip to content

add text_input_action deserialization factory #131776

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.

Sign up for GitHub

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

Jump to bottom
Closed
sw-tester wants to merge 5 commits into from
Closed

add text_input_action deserialization factory #131776

sw-tester wants to merge 5 commits into from

Conversation

@sw-tester
Copy link

@sw-tester sw-tester commented Aug 2, 2023 •
edited
Loading

adding deserialisation factory for SendTextInputAction.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Aug 2, 2023

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Aug 2, 2023
@sw-tester sw-tester mentioned this pull request Aug 2, 2023
Open
@pdblasi-google
Copy link
Contributor

pdblasi-google commented Aug 9, 2023

Hi @5v1988,

Thanks for the contribution! It looks like this PR is missing a couple of things though:

  • It's best to file an issue in case there are any problems downstream and this PR needs to be reverted and relanded. That way references to this change can be made without being dependent on this specific PR introducing it. It also helps others find issues when they have the same problem you have.
  • We'll also need a test to make sure this isn't accidentally undone in the future. This area of the codebase is a bit strange to test, but you should be able to test this (indirectly at least) through extension_test.dart. There are other tests there that validate specific commands are being deserialized and acted upon that you can use as a reference.

@Piinks
Copy link
Contributor

Piinks commented Aug 30, 2023

Hi @5v1988, since there has not been an update here for some time, I am going to close this PR for now to remove it from our review queue. If you would like to return to it and address the feedback above, please feel free to reopen it. Thanks for contributing!

@Piinks Piinks closed this Aug 30, 2023
@nonvachara-paloit nonvachara-paloit mentioned this pull request Nov 21, 2023
Closed
@nonvachara-paloit
Copy link
Contributor

nonvachara-paloit commented Nov 22, 2023

I ran into an issue relates to this PR today and I'm will to fix. Should I create a new PR or reopen (don't know how) this one?

@Piinks
Copy link
Contributor

Piinks commented Nov 27, 2023

I ran into an issue relates to this PR today and I'm will to fix. Should I create a new PR or reopen (don't know how) this one?

@nonvachara-paloit thanks for contributing! I would recommend opening a new PR since this was authored by someone else.

@nonvachara-paloit nonvachara-paloit mentioned this pull request Nov 29, 2023
Merged
auto-submit bot pushed a commit that referenced this pull request Dec 21, 2023
@nonvachara-paloit
Add send_text_input_action case to deserialization_factory to allow s…
90badf7 
…endTextInputAction usages through flutter_driver. (#139197)

**As a follow up to #131776

**Summary:**
Previously in #106561, SendTextInputAction was added to Flutter Driver.
But it still cannot be used from flutter_driver tests. This PR intends to resolve that issue.

**Issue:**
An `DriverError: Unsupported command kind send_text_input_action` would be thrown from `flutter_driver/lib/src/common/deserialization_factory.dart` when a call to `driver.sendTextInputAction(TextInputAction.done);` was made despite the method `sendTextInputAction` is available for use since #106561.

Previous works has been done in #131776, I merely added tests.

Best regards.
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
@nonvachara-paloit @CoderDake
Add send_text_input_action case to deserialization_factory to allow s…
5c25bd0 
…endTextInputAction usages through flutter_driver. (flutter#139197)

**As a follow up to flutter#131776

**Summary:**
Previously in flutter#106561, SendTextInputAction was added to Flutter Driver.
But it still cannot be used from flutter_driver tests. This PR intends to resolve that issue.

**Issue:**
An `DriverError: Unsupported command kind send_text_input_action` would be thrown from `flutter_driver/lib/src/common/deserialization_factory.dart` when a call to `driver.sendTextInputAction(TextInputAction.done);` was made despite the method `sendTextInputAction` is available for use since flutter#106561.

Previous works has been done in flutter#131776, I merely added tests.

Best regards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LongCatIsLooong LongCatIsLooong Awaiting requested review from LongCatIsLooong

@pdblasi-google pdblasi-google Awaiting requested review from pdblasi-google

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sw-tester @pdblasi-google @Piinks @nonvachara-paloit