Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Linux] composing delta fixes #42648

Merged
auto-submit merged 17 commits into from
Jun 8, 2023
Merged

[Linux] composing delta fixes #42648

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
cfe03b1
send correct delta range on preedit changed and send previous text on…
Jun 7, 2023
81e7972
fix analyzer
Jun 7, 2023
a4311b6
fix trailing spaces
Jun 7, 2023
5fe5f77
formatting
Jun 7, 2023
290a5c4
more formatting
Jun 7, 2023
4f4f4ce
more formatting
Jun 7, 2023
bed42da
more formatting
Jun 8, 2023
2acbad0
remove logs
Renzo-Olivares Jun 8, 2023
82d49a6
Fix exisiting test
Renzo-Olivares Jun 8, 2023
dfd89c4
Fix existing test and add a test
Renzo-Olivares Jun 8, 2023
2bade18
updates
Renzo-Olivares Jun 8, 2023
bedd07c
Address reviewer comments
Renzo-Olivares Jun 8, 2023
3d2f51c
patch
Renzo-Olivares Jun 8, 2023
1317a3a
Update shell/platform/linux/fl_text_input_plugin.cc
cbracken Jun 8, 2023
73fd1ed
pass raw pointer and not unique ptr
Renzo-Olivares Jun 8, 2023
bd1e88e
revert preedit-end using composing_before_change
Renzo-Olivares Jun 8, 2023
5c3f14a
updates
Renzo-Olivares Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions shell/platform/linux/fl_text_input_plugin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ static void im_preedit_changed_cb(FlTextInputPlugin* self) {
FlTextInputPluginPrivate* priv = static_cast<FlTextInputPluginPrivate*>(
fl_text_input_plugin_get_instance_private(self));
std::string text_before_change = priv->text_model->GetText();
flutter::TextRange composing_before_change =
priv->text_model->composing_range();
g_autofree gchar* buf = nullptr;
gint cursor_offset = 0;
gtk_im_context_get_preedit_string(priv->im_context, &buf, nullptr,
Expand All @@ -278,7 +280,7 @@ static void im_preedit_changed_cb(FlTextInputPlugin* self) {
if (priv->enable_delta_model) {
std::string text(buf);
flutter::TextEditingDelta delta = flutter::TextEditingDelta(
text_before_change, priv->text_model->composing_range(), text);
text_before_change, composing_before_change, text);
update_editing_state_with_delta(self, &delta);
} else {
update_editing_state(self);
Expand All @@ -290,17 +292,27 @@ static void im_commit_cb(FlTextInputPlugin* self, const gchar* text) {
FlTextInputPluginPrivate* priv = static_cast<FlTextInputPluginPrivate*>(
fl_text_input_plugin_get_instance_private(self));
std::string text_before_change = priv->text_model->GetText();
flutter::TextRange composing_before_change =
priv->text_model->composing_range();
flutter::TextRange selection_before_change = priv->text_model->selection();
gboolean wasComposing = priv->text_model->composing();

@cbracken cbracken Jun 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
gboolean wasComposing = priv->text_model->composing();
gboolean was_composing = priv->text_model->composing();


priv->text_model->AddText(text);
if (priv->text_model->composing()) {
priv->text_model->CommitComposing();
}

if (priv->enable_delta_model) {
flutter::TextEditingDelta delta = flutter::TextEditingDelta(
text_before_change, selection_before_change, text);
update_editing_state_with_delta(self, &delta);
flutter::TextEditingDelta* delta;
if (wasComposing) {
delta = new flutter::TextEditingDelta(text_before_change,
composing_before_change, text);

@cbracken cbracken Jun 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Avoid new/delete in C++ code. Use std::unique_ptr<Foo>, which you can create with std::make_unique<Foo>(constructor, args, go, here);

@justinmc justinmc Jun 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cbracken What's the reasoning behind this? (for my own education)

@cbracken cbracken Jun 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally always use RAII-based smart pointers (primarily unique_ptr) unless it's impossible, they're guaranteed to clean themselves up at end of scope whether it's a return or throw, and they're effectively zero overhead.

In this case there's only one way out of the scope, but there's a function call in there that could do something crazy like throw (but almost certainly doesn't 😉). The more ways out of a scope the more deletes you'll need to make sure not to forget. Smart pointers guarantee cleanup so why even risk it?

In modern c++ new/delete are generally considered bad practice unless absolutely necessary. In our codebase there are maybe two spots I've found where we couldn't use either unique_ptr or shared_ptr, and we still have a new/delete, but we should avoid adding any more.

More reading in the C++ FAQ and C++ Core Guidelines.

} else {
delta = new flutter::TextEditingDelta(text_before_change,
selection_before_change, text);
}

@cbracken cbracken Jun 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Possibly more succinct:

flutter::TextRange replace_range = was_composing ? composing_before_change : selection_before_change;
std::unique_ptr<flutter::TextEditingDelta> delta =
    std::make_unique<flutter::TextEditingDelta>(text_before_change, replacement_range, text);

update_editing_state_with_delta(self, delta);
delete delta;
} else {
update_editing_state(self);
}
Expand All @@ -310,10 +322,12 @@ static void im_commit_cb(FlTextInputPlugin* self, const gchar* text) {
static void im_preedit_end_cb(FlTextInputPlugin* self) {
FlTextInputPluginPrivate* priv = static_cast<FlTextInputPluginPrivate*>(
fl_text_input_plugin_get_instance_private(self));
std::string text_before_change = priv->text_model->GetText();
priv->text_model->EndComposing();
if (priv->enable_delta_model) {
flutter::TextEditingDelta delta = flutter::TextEditingDelta(
"", flutter::TextRange(-1, -1), priv->text_model->GetText());
text_before_change, flutter::TextRange(-1, -1),

@cbracken cbracken Jun 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm surprised this works. The type of the base/extent positions are size_t, which is unsigned. I'm going to take a quick look at what options we have for modelling this properly.

@cbracken cbracken Jun 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here's an incomplete prototype of something we could use to do this safely:
https://github.com/flutter/engine/compare/main...cbracken:flutter_engine:add-invalid-range?expand=1

If this seems useful, I could commit it, but when I added TextRange originally my thought was to see how far along we could make it without modelling things using invalid positions like -1 since some platforms (macOS, iOS) don't model any positions with negative numbers, so we're going to need to do translation between models somewhere regardless and TextRange was cleaner without the concept of negative values.

If we think this is something that we absolutely need to model in the embedders, I could clean this up and update the existing TextRange-using embedders to use it.

@Renzo-Olivares Renzo-Olivares Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think in this case it may be correct to use composing_before_change. So this may not be needed. Thanks for checking that out!

@Renzo-Olivares Renzo-Olivares Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

correction, a delta range of (-1,-1), is needed but we should just use TextEditingDelta(const std::u16string & text) for this case.

priv->text_model->GetText());
update_editing_state_with_delta(self, &delta);
} else {
update_editing_state(self);
Expand Down
253 changes: 247 additions & 6 deletions shell/platform/linux/fl_text_input_plugin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,8 @@ TEST(FlTextInputPluginTest, ComposingDelta) {

messenger.ReceiveMessage("flutter/textinput", set_client);

g_signal_emit_by_name(context, "preedit-start", nullptr);

// update
EXPECT_CALL(context,
gtk_im_context_get_preedit_string(
Expand All @@ -976,7 +978,7 @@ TEST(FlTextInputPluginTest, ComposingDelta) {
.old_text = "",
.delta_text = "Flutter ",
.delta_start = 0,
.delta_end = 8,
.delta_end = 0,
.selection_base = 8,
.selection_extent = 8,
.composing_base = 0,
Expand Down Expand Up @@ -1004,13 +1006,13 @@ TEST(FlTextInputPluginTest, ComposingDelta) {
build_list({
build_editing_delta({
.old_text = "Flutter ",
.delta_text = "engine",
.delta_start = 8,
.delta_text = "Flutter engine",
.delta_start = 0,
.delta_end = 8,
.selection_base = 14,
.selection_extent = 14,
.composing_base = 0,
.composing_extent = 8,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
Expand All @@ -1024,7 +1026,7 @@ TEST(FlTextInputPluginTest, ComposingDelta) {
FlValueEq(commit)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "engine", nullptr);
g_signal_emit_by_name(context, "commit", "Flutter engine", nullptr);

// end
g_autoptr(FlValue) end = build_list({
Expand All @@ -1033,6 +1035,7 @@ TEST(FlTextInputPluginTest, ComposingDelta) {
"deltas",
build_list({
build_editing_delta({
.old_text = "Flutter engine",
.delta_text = "Flutter engine",
.selection_base = 14,
.selection_extent = 14,
Expand All @@ -1051,3 +1054,241 @@ TEST(FlTextInputPluginTest, ComposingDelta) {

g_signal_emit_by_name(context, "preedit-end", nullptr);
}

TEST(FlTextInputPluginTest, NonComposingDelta) {
::testing::NiceMock<flutter::testing::MockBinaryMessenger> messenger;
::testing::NiceMock<flutter::testing::MockIMContext> context;
::testing::NiceMock<flutter::testing::MockTextInputViewDelegate> delegate;

g_autoptr(FlTextInputPlugin) plugin =
fl_text_input_plugin_new(messenger, context, delegate);
EXPECT_NE(plugin, nullptr);

// set config
g_autoptr(FlValue) args = build_input_config({
.client_id = 1,
.enable_delta_model = true,
});
g_autoptr(FlJsonMethodCodec) codec = fl_json_method_codec_new();
g_autoptr(GBytes) set_client = fl_method_codec_encode_method_call(
FL_METHOD_CODEC(codec), "TextInput.setClient", args, nullptr);

g_autoptr(FlValue) null = fl_value_new_null();
EXPECT_CALL(messenger, fl_binary_messenger_send_response(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::A<FlBinaryMessengerResponseHandle*>(),
SuccessResponse(null), ::testing::A<GError**>()))
.WillOnce(::testing::Return(true));

messenger.ReceiveMessage("flutter/textinput", set_client);

// commit F
g_autoptr(FlValue) commit = build_list({
fl_value_new_int(1), // client_id
build_map({{
"deltas",
build_list({
build_editing_delta({
.old_text = "",
.delta_text = "F",
.delta_start = 0,
.delta_end = 0,
.selection_base = 1,
.selection_extent = 1,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
});

EXPECT_CALL(messenger,
fl_binary_messenger_send_on_channel(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::StrEq("flutter/textinput"),
MethodCall("TextInputClient.updateEditingStateWithDeltas",
FlValueEq(commit)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "F", nullptr);

// commit l
g_autoptr(FlValue) commitL = build_list({
fl_value_new_int(1), // client_id
build_map({{
"deltas",
build_list({
build_editing_delta({
.old_text = "F",
.delta_text = "l",
.delta_start = 1,
.delta_end = 1,
.selection_base = 2,
.selection_extent = 2,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
});

EXPECT_CALL(messenger,
fl_binary_messenger_send_on_channel(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::StrEq("flutter/textinput"),
MethodCall("TextInputClient.updateEditingStateWithDeltas",
FlValueEq(commitL)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "l", nullptr);

// commit u
g_autoptr(FlValue) commitU = build_list({
fl_value_new_int(1), // client_id
build_map({{
"deltas",
build_list({
build_editing_delta({
.old_text = "Fl",
.delta_text = "u",
.delta_start = 2,
.delta_end = 2,
.selection_base = 3,
.selection_extent = 3,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
});

EXPECT_CALL(messenger,
fl_binary_messenger_send_on_channel(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::StrEq("flutter/textinput"),
MethodCall("TextInputClient.updateEditingStateWithDeltas",
FlValueEq(commitU)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "u", nullptr);

// commit t
g_autoptr(FlValue) commitTa = build_list({
fl_value_new_int(1), // client_id
build_map({{
"deltas",
build_list({
build_editing_delta({
.old_text = "Flu",
.delta_text = "t",
.delta_start = 3,
.delta_end = 3,
.selection_base = 4,
.selection_extent = 4,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
});

EXPECT_CALL(messenger,
fl_binary_messenger_send_on_channel(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::StrEq("flutter/textinput"),
MethodCall("TextInputClient.updateEditingStateWithDeltas",
FlValueEq(commitTa)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "t", nullptr);

// commit t again
g_autoptr(FlValue) commitTb = build_list({
fl_value_new_int(1), // client_id
build_map({{
"deltas",
build_list({
build_editing_delta({
.old_text = "Flut",
.delta_text = "t",
.delta_start = 4,
.delta_end = 4,
.selection_base = 5,
.selection_extent = 5,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
});

EXPECT_CALL(messenger,
fl_binary_messenger_send_on_channel(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::StrEq("flutter/textinput"),
MethodCall("TextInputClient.updateEditingStateWithDeltas",
FlValueEq(commitTb)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "t", nullptr);

// commit e
g_autoptr(FlValue) commitE = build_list({
fl_value_new_int(1), // client_id
build_map({{
"deltas",
build_list({
build_editing_delta({
.old_text = "Flutt",
.delta_text = "e",
.delta_start = 5,
.delta_end = 5,
.selection_base = 6,
.selection_extent = 6,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
});

EXPECT_CALL(messenger,
fl_binary_messenger_send_on_channel(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::StrEq("flutter/textinput"),
MethodCall("TextInputClient.updateEditingStateWithDeltas",
FlValueEq(commitE)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "e", nullptr);

// commit r
g_autoptr(FlValue) commitR = build_list({
fl_value_new_int(1), // client_id
build_map({{
"deltas",
build_list({
build_editing_delta({
.old_text = "Flutte",
.delta_text = "r",
.delta_start = 6,
.delta_end = 6,
.selection_base = 7,
.selection_extent = 7,
.composing_base = -1,
.composing_extent = -1,
}),
}),
}}),
});

EXPECT_CALL(messenger,
fl_binary_messenger_send_on_channel(
::testing::Eq<FlBinaryMessenger*>(messenger),
::testing::StrEq("flutter/textinput"),
MethodCall("TextInputClient.updateEditingStateWithDeltas",
FlValueEq(commitR)),
::testing::_, ::testing::_, ::testing::_));

g_signal_emit_by_name(context, "commit", "r", nullptr);
}