Skip to content

Conversation

@nploi
Copy link
Contributor

@nploi nploi commented Jun 24, 2022

Currently, we don't support keyboard events in flutter_driver, this PR will be support that feature.

List which issues are fixed by this PR. You must list at least one issue.

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 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 Jun 24, 2022
@goderbauer
Copy link
Member

@nploi Looks like this PR is failing some checks. Can you take a look and fix those up? Thanks!

@nploi
Copy link
Contributor Author

nploi commented Jun 30, 2022

@nploi Looks like this PR is failing some checks. Can you take a look and fix those up? Thanks!

yes, sure!, i writing more unit-test for this.

@nploi nploi marked this pull request as ready for review June 30, 2022 17:04
@HansMuller HansMuller requested a review from dkwingsmt July 1, 2022 21:45
@nploi
Copy link
Contributor Author

nploi commented Jul 4, 2022

Hi @dkwingsmt, please help me review this PR. thanks.

@goderbauer
Copy link
Member

@nploi It looks like some of the checks are still failing on this PR. Can you please take a look and fix those issues? Thanks!

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

Also, add the following comment to services/TextInputAction:

// This class has been cloned to `flutter_driver/lib/src/common/action.dart` as `TextInputAction`,
// and must be kept in sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section simply reformatting? Unless there is solid reasons, I don't think you should reformat existing code.

The same for other places in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, my editor auto reformat, i reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it needed to rename it or can we keep TextInputAction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we needed to rename, because it conflict with TextInputAction class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me try to rename.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the ReceiveAction class: can we rename it to sendTextInputAction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the class is renamed, keep this string in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename the file to text_input_action.dart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks.

@nploi nploi marked this pull request as draft July 7, 2022 15:57
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

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

@nploi nploi changed the title [flutter_driver] support receive input action [flutter_driver] support send text input action Jul 7, 2022
@flutter-dashboard flutter-dashboard bot added the a: text input Entering text in a text field or keyboard related problems label Jul 7, 2022
@nploi nploi marked this pull request as ready for review July 7, 2022 17:37
@nploi nploi requested a review from dkwingsmt July 7, 2022 17:37
@nploi
Copy link
Contributor Author

nploi commented Jul 7, 2022

Hi @dkwingsmt, i fixed your comments, pls help me re-review. thanks.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

Minor change requests about the comments. Otherwise good to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

This new paragraph should intentionally be a normal comment (starting with //, after the dart doc) instead of a dart doc (starting with ///). A dart doc is visible to the end developers, but the developers should not care about what's in flutter_driver. This paragraph is only meant for Flutter framework developers who might edit this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thank you so much.

Comment on lines 36 to 39
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
///
/// This class is identical to [TextInputAction](https://api.flutter.dev/flutter/services/TextInputAction.html).
/// This class is cloned from `TextInputAction` and must be kept in sync. The cloning is needed
/// because importing is not allowed directly.
// This class is identical to [TextInputAction](https://api.flutter.dev/flutter/services/TextInputAction.html).
// This class is cloned from `TextInputAction` and must be kept in sync. The cloning is needed
// because importing is not allowed directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably update these code blocks so that they are analyzed. You can do it for just this one by adding:

/// {@tool snippet}
///
... The existing sample code
/// {@end-tool}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i updated, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we do this for other enums in this package? if so, I'd be worried about keeping them in sync. It would be great to have a test that checked that they had the same entries at least

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @jonahwilliams
i tried to use the same entries, but build error because that package include UI, so it can't compile in server side.

Do you have any suggestion for me ? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean using the same enum, I mean writing a test that verifies that these enums have the same values. i.e. in the test open the files and check the values

Copy link
Contributor Author

@nploi nploi Jul 13, 2022

Choose a reason for hiding this comment

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

ah, i got it, let me write a test for it. thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonahwilliams , i added unit-test for do it, pls help me review again.

@nploi nploi marked this pull request as draft July 12, 2022 02:23
@nploi nploi marked this pull request as ready for review July 12, 2022 02:40
@nploi nploi requested review from dkwingsmt and jonahwilliams July 12, 2022 02:40
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat! 👍

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM once tests are fixed. Well done and thank you!

@dkwingsmt
Copy link
Contributor

I'm not sure why these customer tests are failing. Can you try to rebase your PR onto the latest master?

@dkwingsmt dkwingsmt merged commit 0cf9d41 into flutter:master Jul 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jul 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 15, 2022
@nploi nploi deleted the flutter-driver-support-receive-input-action branch July 16, 2022 05:50
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
* Support receive input action

* Fix error syntax

* Fix compile

* Add documents

* Add unit-test

* Update import

* Fixed unit-test and lint

* Add authors for me

* Fixed lint

* Fixed lint

* Add example

* Fixed lint

* Fix gen docs

* Revert code

* Remove flutter_dev

* Update packages/flutter_driver/lib/src/driver/driver.dart

Co-authored-by: Tong Mu <[email protected]>

* Update packages/flutter_driver/lib/src/common/action.dart

Co-authored-by: Tong Mu <[email protected]>

* Update packages/flutter_driver/lib/src/common/action.dart

Co-authored-by: Tong Mu <[email protected]>

* Rename ReceiveAction to SendTextInputAction

* Rename DriverTextInputAction to TextInputAction and fix unit-test

* Reorder import

* Remove space

* Reorder import

* Update text_input.dart

* Update flutter_driver_test.dart

* Update comment to normal comment after dart doc

* Update example

* Update AUTHORS

Co-authored-by: Tong Mu <[email protected]>

* Fix analyze

* Add type dart for example

* Add unit-test to check the same entries

Co-authored-by: Tong Mu <[email protected]>
auto-submit bot pushed a commit that referenced this pull request Dec 21, 2023
…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
…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

Labels

a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants