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

Revert text cursor position calculation windows #51164

Closed
deniz-lee wants to merge 7 commits into from
Closed

Revert text cursor position calculation windows #51164

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
23d5261
Reverts the logic for calculating the TextField input text cursor pos…
deniz-lee Mar 4, 2024
5926fa9
modify header includes
deniz-lee Mar 4, 2024
bc80cd9
Remove platform branching codes in the common area, add to check kore…
deniz-lee Mar 11, 2024
10aefb3
reformatting codes
deniz-lee Mar 11, 2024
9a032ed
reformatting codes
deniz-lee Mar 11, 2024
5387704
Modify checking to korean regular expression to wregex function
deniz-lee Mar 11, 2024
d3640b1
Remove unused variable.
deniz-lee Mar 11, 2024
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
5 changes: 5 additions & 0 deletions shell/platform/common/text_input_model.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <algorithm>
#include <string>

#include "flutter/fml/build_config.h"
#include "flutter/fml/string_conversion.h"

namespace flutter {
Expand Down Expand Up @@ -79,8 +80,12 @@ void TextInputModel::UpdateComposingText(const std::u16string& text,
composing_range_.collapsed() ? selection_ : composing_range_;
text_.replace(rangeToDelete.start(), rangeToDelete.length(), text);
composing_range_.set_end(composing_range_.start() + text.length());
#if FML_OS_WIN
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Mar 5, 2024

Choose a reason for hiding this comment

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

My understanding is that this class is meant to be an OS-agnostic abstraction, so having OS dependant logic here seems weird.

Copy link
Author

@deniz-lee deniz-lee Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This issue (#140739) only occurs on windows.
It seems to have occurred after the modifications below.
#45667

Copy link
Member

@cbracken cbracken Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Correct - this class is common code used by multiple OSes.

We should avoid #ifdef as much as possible since Flutter is used on many platforms, including some which are not part of our repo -- developers for those embedders can't patch in platform-specific #ifdefs so neither should we. (We'd also need platform specific additional testing etc.)

Instead, we should make the patches in the platform-specific embedder that requires updated behaviour. For the case where all platforms need an update we can put it in the common code.

Copy link
Author

@deniz-lee deniz-lee Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that it is not a good idea to include platform branching statements in code in the common area.

selection_ = TextRange(composing_range_.end());
#else // FML_OS_WIN
selection_ = TextRange(selection.start() + composing_range_.start(),
selection.extent() + composing_range_.start());
#endif // FML_OS_WIN
}

void TextInputModel::UpdateComposingText(const std::u16string& text) {
Expand Down
1 change: 1 addition & 0 deletions shell/platform/windows/text_input_plugin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ void TextInputPlugin::ComposeChangeHook(const std::u16string& text,
std::string text_before_change = active_model_->GetText();
TextRange composing_before_change = active_model_->composing_range();
active_model_->AddText(text);
cursor_pos += active_model_->composing_range().extent();
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Mar 4, 2024

Choose a reason for hiding this comment

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

IIRC cursor_pos is relative to the start of the new composing region. So this does not seem to be right.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Mar 4, 2024

Choose a reason for hiding this comment

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

However I suspect the AddText is no longer needed (and neither is text_after_change but that's unrelated to this PR.

Copy link
Author

@deniz-lee deniz-lee Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is related below:
Use start instead of extent for Windows IME cursor position #45667
Fix macOS text composing #49314

It was applied to solve problems when inputting Japanese and Chinese, but new issues are occurring in Korean.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Mar 5, 2024

Choose a reason for hiding this comment

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

Do you by chance have a repro with stack frames (especially the input arguments of this method when the cursor is placed at the wrong location)?

Copy link
Author

@deniz-lee deniz-lee Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the case of Korean, cursor_pos is based on the end of the new composing area.
When entering Korean, the text cursor is behind single space. #140739
https://bugs.chromium.org/p/chromium/issues/detail?id=653160
스크린샷 2024-03-05 16 37 36

This PR did not pass the unittests below.

  • UpdateSelectionWhileComposing (text_input_model_unittests.cc),
  • CompositionCursorPos (text_input_plugin_unittest.cc)

So, this PR commits are incomplete and needs improvement.
However, I don't fully understand process of that unittest cases, so I'm checking it further.

Copy link
Author

@deniz-lee deniz-lee Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@loic-sharma , @LongCatIsLooong When I looked it up, there was no branching with regular expressions in the engine code before.
So I'm not sure if adding regular expressions to check Korean is the right way, but I tried modifying the code to fix the issue.

When I ran the unit tests on the local engine, they all passed, but I am not sure if the difference in clang library version is the cause, but the unit tests failed on Mac and Linux.

Even if the above pull request is not approved, please reexamine to confirm the fix for the Korean issue.

active_model_->UpdateComposingText(text, TextRange(cursor_pos, cursor_pos));
std::string text_after_change = active_model_->GetText();
if (enable_delta_model) {
Expand Down