[Windows - TextInput] Insert new line only when TextInputAction.newline #42244

bleroux · 2023-05-23T13:28:14Z

Description

This PR updates the Windows text input plugin to avoid adding a new line on a multiline text field when action is not set to TextInputAction.newline.

Related Issue

Fixes flutter/flutter#125879 as Linux and macOS implementations are merged.
Linux PR: #41895
macOS PR: #41977

Tests

Adds 2 tests.

justinmc

LGTM 👍 Maybe wait for @cbracken in case he wants to take a look, I'm no expert on the Windows tests here. Thanks for following up on another platform for this!

yaakovschectman · 2023-05-30T19:01:28Z

shell/platform/windows/text_input_plugin_unittest.cc

+    std::string input_action) {
+  auto arguments = std::make_unique<rapidjson::Document>(rapidjson::kArrayType);
+  auto& allocator = arguments->GetAllocator();
+  arguments->PushBack(42, allocator);


What is the significance of this value 42 here?

This is the client ID. I duplicated this from existing code (see lines 173, 254 and 301).
I can add an inline comment such as arguments->PushBack(42, allocator); // Client id.or a constant such as kDefaultClientId, which one do you prefer?

I am inclined to prefer using a named constant, but either approach sounds good to me.

+1 for named constant here

loic-sharma · 2023-05-30T20:51:01Z

shell/platform/windows/text_input_plugin_unittest.cc

+        int length = static_cast<int>(message_size);
+        std::vector<uint8_t> last_message(length);
+        memcpy(&last_message[0], &message[0], length * sizeof(uint8_t));
+        messages.push_back(last_message);


What do you think of storing std::strings instead?

Suggested change

int length = static_cast<int>(message_size);

std::vector<uint8_t> last_message(length);

memcpy(&last_message[0], &message[0], length * sizeof(uint8_t));

messages.push_back(last_message);

std::string last_message(reinterpret_cast<const char*>(message), message_size);

messages.push_back(last_message);

Would this still allow use of std::equal on https://github.com/flutter/engine/pull/42244/files/16ea94095b930f2c0a85e244b43abf31dbe690f0#diff-f913efa93fd3ac5c33b9fc924f41ff6e1a7f8300a5e548aa52fb0d30a3904b4cR247, as update_state_message will still be the same type?

What do you think of storing std::strings instead?

I choosed to rely on std::vector<uint8_t> because this is the type that is returned by codec.EncodeMethodCall, I think it is more understandable to stick to the type expectations of the existing APIs use by the test.

Storing std::strings would make debugging a bit easier if things break.

If you still prefer storing uint8_ts, could you replace the memcpy's &last_message[0] argument with last_message.data()?

Would this still allow use of std::equal on https://github.com/flutter/engine/pull/42244/files/16ea94095b930f2c0a85e244b43abf31dbe690f0#diff-f913efa93fd3ac5c33b9fc924f41ff6e1a7f8300a5e548aa52fb0d30a3904b4cR247, as update_state_message will still be the same type?

We can replace EXPECT_TRUE(std::equal(a, b)) with EXPECT_EQ(a, b) if both values are std::strings. Please let me know if I misunderstood your question!

I have updated the PR to store messages as std::string as suggested (less code and not relying on memcpy, thanks for the suggestion).
I have not updated the check on line 248 because I did not manage to find a less verbose way to do it.

Personally for line 248 I would just use a string with the expected JSON. Here's an example where we do that:

engine/shell/platform/fuchsia/flutter/text_delegate_unittests.cc

Lines 228 to 231 in 7c0a639

EXPECT_EQ(

R"({"method":"TextInputClient.performAction","args":[7,"TextInputAction.done"]})",

MessageToString(*last_message_));

}

I don't feel strongly about this though, feel free to keep the existing logic

yaakovschectman

LGTM as long as @loic-sharma is good with the use of vectors as you've been discussing

loic-sharma

Consider using a std::string for the expected JSON. See this comment: #42244 (comment)

Otherwise, LGTM! Thank you for the patch! :)

…tion.newline (flutter/engine#42244)

…128089) flutter/engine@3a453f2...02d6fbb 2023-06-01 [email protected] [Impeller] Emplace directly into host buffer (avoid VBB) for text data (flutter/engine#42484) 2023-06-01 [email protected] Ensure PlatformView engine life cycle callbacks are invoked (flutter/engine#42491) 2023-06-01 [email protected] Roll Skia from c408e8e9cc96 to 082a7d1f72f7 (8 revisions) (flutter/engine#42496) 2023-06-01 [email protected] [Windows - TextInput] Insert new line only when TextInputAction.newline (flutter/engine#42244) 2023-06-01 [email protected] Revert "Move clang tidy v2 build to prod." (flutter/engine#42495) 2023-06-01 [email protected] Add myself to AUTHORS (flutter/engine#42406) 2023-06-01 [email protected] [Impeller] Add Impeller Metal support in the embedder API (flutter/engine#42411) 2023-06-01 [email protected] Support DisposalMethod::kRestorePrevious in MultiFrameCodec and fix the apng problem. (flutter/engine#42153) 2023-06-01 [email protected] Fix crash getting spell-check suggestions (flutter/engine#42466) 2023-06-01 [email protected] Fix lint in rectangle packer (flutter/engine#42489) 2023-06-01 [email protected] Wait for GL command completion in the ExternalTextureGLRefreshedTooOften test (flutter/engine#42438) 2023-06-01 [email protected] Reland "[web] Remove the JS API for url strategy (#42134)" (flutter/engine#42486) 2023-06-01 [email protected] Roll Skia from f5bc3d12f0eb to c408e8e9cc96 (9 revisions) (flutter/engine#42487) 2023-06-01 [email protected] Clean up Skia includes around SkSurfaceCharacterization (flutter/engine#42485) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter-dashboard bot added the platform-windows label May 23, 2023

bleroux force-pushed the windows_text_input_insert_new_line_only_for_input_action_newline branch 2 times, most recently from 0a29291 to 16ea940 Compare May 23, 2023 14:16

bleroux requested review from cbracken and justinmc May 23, 2023 15:11

justinmc approved these changes May 23, 2023

View reviewed changes

loic-sharma requested review from loic-sharma and yaakovschectman May 30, 2023 18:35

yaakovschectman reviewed May 30, 2023

View reviewed changes

loic-sharma reviewed May 30, 2023

View reviewed changes

bleroux force-pushed the windows_text_input_insert_new_line_only_for_input_action_newline branch from 16ea940 to cc96f0c Compare May 31, 2023 06:56

bleroux requested review from loic-sharma and yaakovschectman May 31, 2023 07:08

yaakovschectman approved these changes May 31, 2023

View reviewed changes

bleroux added 2 commits June 1, 2023 07:56

[Windows - TextInput] Insert new line only when TextInputAction.newline

253352a

Define a constant for text input client id

4f6dead

bleroux force-pushed the windows_text_input_insert_new_line_only_for_input_action_newline branch from cc96f0c to 75da226 Compare June 1, 2023 07:19

Replace std::vector<uint8_t>with std::string

b4eea8b

bleroux force-pushed the windows_text_input_insert_new_line_only_for_input_action_newline branch from 75da226 to b4eea8b Compare June 1, 2023 14:15

loic-sharma approved these changes Jun 1, 2023

View reviewed changes

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 1, 2023

auto-submit bot merged commit f382b85 into flutter:main Jun 1, 2023

engine-flutter-autoroll mentioned this pull request Jun 1, 2023

Roll Flutter Engine from 3a453f2ccb77 to 59dfac7a1eda (12 revisions) flutter/flutter#128085

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 1, 2023

f382b8571 [Windows - TextInput] Insert new line only when TextInputAc…

238b3eb

…tion.newline (flutter/engine#42244)

engine-flutter-autoroll mentioned this pull request Jun 1, 2023

Roll Flutter Engine from 3a453f2ccb77 to 59afddd4cf5b (13 revisions) flutter/flutter#128087

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 1, 2023

f382b8571 [Windows - TextInput] Insert new line only when TextInputAc…

b98ff48

…tion.newline (flutter/engine#42244)

engine-flutter-autoroll mentioned this pull request Jun 1, 2023

Roll Flutter Engine from 3a453f2ccb77 to 02d6fbb68b69 (14 revisions) flutter/flutter#128089

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 1, 2023

f382b8571 [Windows - TextInput] Insert new line only when TextInputAc…

4b1f60d

…tion.newline (flutter/engine#42244)

bleroux deleted the windows_text_input_insert_new_line_only_for_input_action_newline branch June 2, 2023 05:03

	EXPECT_EQ(
	R"({"method":"TextInputClient.performAction","args":[7,"TextInputAction.done"]})",
	MessageToString(*last_message_));
	}

[Windows - TextInput] Insert new line only when TextInputAction.newline #42244

[Windows - TextInput] Insert new line only when TextInputAction.newline #42244

Uh oh!

Conversation

bleroux commented May 23, 2023

Description

Related Issue

Tests

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

yaakovschectman May 30, 2023

Choose a reason for hiding this comment

Uh oh!

bleroux May 30, 2023

Choose a reason for hiding this comment

Uh oh!

yaakovschectman May 30, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma May 30, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma May 30, 2023

Choose a reason for hiding this comment

Uh oh!

yaakovschectman May 30, 2023

Choose a reason for hiding this comment

Uh oh!

bleroux May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleroux Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakovschectman left a comment

Choose a reason for hiding this comment

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bleroux May 31, 2023 •

edited

Loading

loic-sharma May 31, 2023 •

edited

Loading

bleroux Jun 1, 2023 •

edited

Loading

loic-sharma Jun 1, 2023 •

edited

Loading