[Windows] Begin decoupling text input plugin from the view by loic-sharma · Pull Request #47833 · flutter/engine

loic-sharma · 2023-11-08T22:49:08Z

Currently the text input plugin is strongly tied to a single view. This change makes the text input plugin tied to the engine in preparation for multi-view world.

Part of flutter/flutter#115611

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

loic-sharma · 2023-11-09T20:09:16Z

shell/platform/windows/text_input_plugin.h

  // For more information on the delta model, see:
  // https://master-api.flutter.dev/flutter/services/TextInputConfiguration/enableDeltaModel.html
-  bool enable_delta_model;
+  bool enable_delta_model = false;


The lack of a deterministic default value caused optimized unit tests to send delta model messages unexpectedly.

Strange - this should be default zero-initialized. Do we know in which specific situations there were problems?

fwiw even if only for readability as a declaration of intent, we should make this change.

shell/platform/windows/text_input_plugin.h

yaakovschectman

Alrighty then

chinmaygarde · 2023-11-16T21:10:08Z

Can we land this? Looks good to go.

cbracken · 2023-11-16T22:27:21Z

shell/platform/windows/text_input_plugin.h

+  //
+  // Triggered when the user commits the current composing text while using a
+  // multi-step input method such as in CJK text input. Composing continues with
+  // the next keypress.


uber-nit: it's not necessarily the user who explicitly performs a commit -- in Korean, for example, just typing the next character is enough to trigger a commit of the composing region. Maybe "Called when the user triggers a commit of the [...]" to be very slightly more accurate.

Thanks for the additional info, I'm not very familiar with this area. Updated! :)

cbracken · 2023-11-16T22:28:41Z

shell/platform/windows/text_input_plugin.h

+  // Called on an IME compose end event.
+  //
+  // Triggered when the user commits the composing text while using a multi-step
+  // input method such as in CJK text input.


In this case called when composing ends, which is often preceded by a commit, but if they press ESC, for example, won't be.

Updated this!

cbracken · 2023-11-17T02:06:37Z

shell/platform/windows/text_input_plugin_unittest.cc

+  MockFlutterWindowsView* view() { return view_.get(); }
+  MockWindowBindingHandler* window() { return window_; }
+
+  void use_headless_engine() {


style (here and the next one): Use UpperCamelCase for methods. For simple accessors and mutators, you can use the same naming as variables (e.g., engine and set_engine).

Ref: https://google.github.io/styleguide/cppguide.html#Function_Names

The ones above all look good!

Fixed this and the other areas where I stole this name from :)

cbracken

Looks good! Just a few small nits.

cbracken

lgtm! Looks great!

jonahwilliams · 2023-11-30T21:05:07Z

Is this safe to land?

loic-sharma · 2023-11-30T21:49:44Z

Yes, landed! :)

…lutter/engine#47833)

flutter/engine@74d2df5...c26e6ce 2023-12-01 matanlurey@users.noreply.github.com Roll a new version of googletest (2021->2023). (flutter/engine#48285) 2023-12-01 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Add ImageFilter.compose (flutter/engine#48546) 2023-12-01 dnfield@google.com Avoid reloading the kernel snapshot when spawning an isolate in the same group (flutter/engine#48478) 2023-12-01 43054281+camsim99@users.noreply.github.com [Android] Check for text to paste before trying to retrieve data from URI (flutter/engine#48166) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 88a1bcb9e43e to 0a90c366ff87 (3 revisions) (flutter/engine#48549) 2023-11-30 skia-flutter-autoroll@skia.org Manual roll Dart SDK to 3.3.0-174.2.beta (4 revisions) (flutter/engine#48538) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from db3399a541f3 to 88a1bcb9e43e (1 revision) (flutter/engine#48545) 2023-11-30 737941+loic-sharma@users.noreply.github.com [Windows] Begin decoupling text input plugin from the view (flutter/engine#47833) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 2d236de89898 to db3399a541f3 (3 revisions) (flutter/engine#48543) 2023-11-30 flar@google.com [Impeller] Add direct tesselation of circles for DrawCircle and Round end caps (flutter/engine#48103) 2023-11-30 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Revert to `drawImage` rendering on Chrome 110 or earlier (flutter/engine#48515) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 6b4bdebaab88 to 2d236de89898 (1 revision) (flutter/engine#48537) 2023-11-30 30870216+gaaclarke@users.noreply.github.com [Impeller] Started expanding the blur clip region (flutter/engine#48535) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 0e479728cc1f to 6b4bdebaab88 (1 revision) (flutter/engine#48536) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 0968fe18ff75 to 0e479728cc1f (1 revision) (flutter/engine#48534) 2023-11-30 103135467+sealesj@users.noreply.github.com Use Chromium mirror for archive dependency (flutter/engine#48509) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 2f01d500a352 to 0968fe18ff75 (2 revisions) (flutter/engine#48531) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 11a15444a3aa to 2f01d500a352 (1 revision) (flutter/engine#48524) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 84fdd36b1eea to 11a15444a3aa (1 revision) (flutter/engine#48523) 2023-11-30 leroux_bruno@yahoo.fr [Android] Add support for the PlatformChannel "Share.invoke" command (flutter/engine#48265) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 5d64b1322879 to 84fdd36b1eea (1 revision) (flutter/engine#48521) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 23721750e433 to 5d64b1322879 (1 revision) (flutter/engine#48520) 2023-11-30 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 8wu5EgBh1yJPNOd5W... to Bb2k375udWIltCEAx... (flutter/engine#48519) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 5a635f2211ce to 23721750e433 (1 revision) (flutter/engine#48514) 2023-11-29 skia-flutter-autoroll@skia.org Roll Skia from 928e8950e8e3 to 5a635f2211ce (1 revision) (flutter/engine#48511) 2023-11-29 mdebbar@google.com [web] No implicit view in multi-view mode (flutter/engine#48505) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 8wu5EgBh1yJP to Bb2k375udWIl 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 matanl@google.com,rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…39342) flutter/engine@74d2df5...c26e6ce 2023-12-01 matanlurey@users.noreply.github.com Roll a new version of googletest (2021->2023). (flutter/engine#48285) 2023-12-01 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Add ImageFilter.compose (flutter/engine#48546) 2023-12-01 dnfield@google.com Avoid reloading the kernel snapshot when spawning an isolate in the same group (flutter/engine#48478) 2023-12-01 43054281+camsim99@users.noreply.github.com [Android] Check for text to paste before trying to retrieve data from URI (flutter/engine#48166) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 88a1bcb9e43e to 0a90c366ff87 (3 revisions) (flutter/engine#48549) 2023-11-30 skia-flutter-autoroll@skia.org Manual roll Dart SDK to 3.3.0-174.2.beta (4 revisions) (flutter/engine#48538) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from db3399a541f3 to 88a1bcb9e43e (1 revision) (flutter/engine#48545) 2023-11-30 737941+loic-sharma@users.noreply.github.com [Windows] Begin decoupling text input plugin from the view (flutter/engine#47833) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 2d236de89898 to db3399a541f3 (3 revisions) (flutter/engine#48543) 2023-11-30 flar@google.com [Impeller] Add direct tesselation of circles for DrawCircle and Round end caps (flutter/engine#48103) 2023-11-30 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Revert to `drawImage` rendering on Chrome 110 or earlier (flutter/engine#48515) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 6b4bdebaab88 to 2d236de89898 (1 revision) (flutter/engine#48537) 2023-11-30 30870216+gaaclarke@users.noreply.github.com [Impeller] Started expanding the blur clip region (flutter/engine#48535) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 0e479728cc1f to 6b4bdebaab88 (1 revision) (flutter/engine#48536) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 0968fe18ff75 to 0e479728cc1f (1 revision) (flutter/engine#48534) 2023-11-30 103135467+sealesj@users.noreply.github.com Use Chromium mirror for archive dependency (flutter/engine#48509) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 2f01d500a352 to 0968fe18ff75 (2 revisions) (flutter/engine#48531) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 11a15444a3aa to 2f01d500a352 (1 revision) (flutter/engine#48524) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 84fdd36b1eea to 11a15444a3aa (1 revision) (flutter/engine#48523) 2023-11-30 leroux_bruno@yahoo.fr [Android] Add support for the PlatformChannel "Share.invoke" command (flutter/engine#48265) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 5d64b1322879 to 84fdd36b1eea (1 revision) (flutter/engine#48521) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 23721750e433 to 5d64b1322879 (1 revision) (flutter/engine#48520) 2023-11-30 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 8wu5EgBh1yJPNOd5W... to Bb2k375udWIltCEAx... (flutter/engine#48519) 2023-11-30 skia-flutter-autoroll@skia.org Roll Skia from 5a635f2211ce to 23721750e433 (1 revision) (flutter/engine#48514) 2023-11-29 skia-flutter-autoroll@skia.org Roll Skia from 928e8950e8e3 to 5a635f2211ce (1 revision) (flutter/engine#48511) 2023-11-29 mdebbar@google.com [web] No implicit view in multi-view mode (flutter/engine#48505) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 8wu5EgBh1yJP to Bb2k375udWIl 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 matanl@google.com,rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added affects: desktop platform-windows labels Nov 8, 2023

loic-sharma force-pushed the windows_multi_view_text_input branch from c4b9fcd to 993ce4d Compare November 9, 2023 19:27

loic-sharma requested review from cbracken and yaakovschectman November 9, 2023 20:07

loic-sharma marked this pull request as ready for review November 9, 2023 20:07

loic-sharma commented Nov 9, 2023

View reviewed changes

yaakovschectman reviewed Nov 9, 2023

View reviewed changes

shell/platform/windows/text_input_plugin.h Show resolved Hide resolved

yaakovschectman approved these changes Nov 9, 2023

View reviewed changes

cbracken reviewed Nov 16, 2023

View reviewed changes

cbracken reviewed Nov 17, 2023

View reviewed changes

loic-sharma added 2 commits November 29, 2023 14:53

[Windows] Begin decoupling text input plugin from the view

aa66302

Address feedback

8a4aa88

loic-sharma force-pushed the windows_multi_view_text_input branch from 993ce4d to 8a4aa88 Compare November 29, 2023 23:31

Tweak

3db73aa

cbracken approved these changes Nov 30, 2023

View reviewed changes

loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 30, 2023

auto-submit bot merged commit 56fa63c into flutter:main Nov 30, 2023

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

Roll Flutter Engine from 74d2df52514a to 58502dc2e02f (23 revisions) flutter/flutter#139336

Closed

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

56fa63c [Windows] Begin decoupling text input plugin from the view (f…

e748b1d

…lutter/engine#47833)

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

Roll Flutter Engine from 74d2df52514a to 63484b316d78 (25 revisions) flutter/flutter#139340

Closed

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

56fa63c [Windows] Begin decoupling text input plugin from the view (f…

53e2dba

…lutter/engine#47833)

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

Roll Flutter Engine from 74d2df52514a to c26e6ced11df (26 revisions) flutter/flutter#139342

Merged

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

56fa63c [Windows] Begin decoupling text input plugin from the view (f…

968228f

…lutter/engine#47833)

loic-sharma deleted the windows_multi_view_text_input branch December 1, 2023 16:59

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

loic-sharma Nov 9, 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!

cbracken Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yaakovschectman left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Nov 16, 2023

Uh oh!

cbracken Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Nov 17, 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 Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 30, 2023

Uh oh!

loic-sharma commented Nov 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

loic-sharma commented Nov 8, 2023 •

edited

Loading

loic-sharma Nov 9, 2023 •

edited

Loading

cbracken Nov 17, 2023 •

edited

Loading