-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[flutter_driver] support send text input action #106561
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
[flutter_driver] support send text input action #106561
Conversation
|
@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. |
|
Hi @dkwingsmt, please help me review this PR. thanks. |
|
@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! |
dkwingsmt
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.
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.
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.
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.
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.
yes, my editor auto reformat, i reverted.
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.
Is it needed to rename it or can we keep TextInputAction?
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.
yes, we needed to rename, because it conflict with TextInputAction class.
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.
let me try to rename.
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.
Same as the ReceiveAction class: can we rename it to sendTextInputAction?
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.
done, thanks.
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.
Indent.
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.
done, thanks.
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.
If the class is renamed, keep this string in sync.
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.
done, thanks.
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.
Can we rename the file to text_input_action.dart?
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.
done, thanks.
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Hi @dkwingsmt, i fixed your comments, pls help me re-review. thanks. |
dkwingsmt
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.
Minor change requests about the comments. Otherwise good to go.
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.
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.
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.
Fixed, thank you so much.
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.
| /// | |
| /// 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. |
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.
Fixed, thanks.
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.
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}
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.
yes, i updated, thanks.
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.
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
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.
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.
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 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
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 got it, let me write a test for it. thanks.
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.
@jonahwilliams , i added unit-test for do it, pls help me review again.
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.
Neat! 👍
jonahwilliams
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.
LGTM once tests are fixed. Well done and thank you!
|
I'm not sure why these customer tests are failing. Can you try to rebase your PR onto the latest master? |
Co-authored-by: Tong Mu <[email protected]>
Co-authored-by: Tong Mu <[email protected]>
Co-authored-by: Tong Mu <[email protected]>
* 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]>
…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.
…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.
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.