Add `undoStackModifier` to `UndoHistory` #138674

Renzo-Olivares · 2023-11-18T22:33:47Z

This change adds a feature to UndoHistory that allows the user to modify the value being pushed onto the undo stack.

This is used by the framework to ignore the composing region when pushing history entries to the Undo stack on Android. This is so an undo does not trigger an input connection restart by the Android TextInputPlugin, which occurs when the framework changes the composing region. This is also the native platform behavior observed in Google Keep app on Android, where doing an undo during composing reverts to the previous state but with composing inactive and a subsequent redo does not bring back the composing region.

Fixes #130881
Partial fix for #134398

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], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

Renzo-Olivares · 2023-11-20T17:15:13Z

I think another way to solve the issue of discarding the composing region so the Android text input plugin does not trigger a restart is to have EditableText use a private implementation of UndoHistory. This would also involve probably making _push on UndoHistoryState public. Curious to hear any thoughts on any of these approaches.

LongCatIsLooong · 2023-11-21T22:07:25Z

packages/flutter/lib/src/widgets/undo_history.dart

Does this dedup like the ValueNotifier class does? If you map all inputs to the same value, does it push one entry to the stack, or multiple entries that are all identical?

In that case it would just push one entry.

flutter/packages/flutter/lib/src/widgets/undo_history.dart

Lines 169 to 171 in db7ef5b

if (widget.value.value == _lastValue) {

return;

}

Oh I was mistaken, this happens before the modifier gets a chance to do anything. I can make sure the modified value is taken into account for this logic.

There is also this logic in _UndoStack.push that prevents this.

flutter/packages/flutter/lib/src/widgets/undo_history.dart

Lines 390 to 392 in db7ef5b

if (value == currentValue) {

return;

}

hellohuanlin · 2023-11-28T22:48:30Z

packages/flutter/lib/src/widgets/editable_text.dart

this is likely something that wants detailed documentation (for future readers and maybe future you)

LongCatIsLooong · 2023-11-29T06:07:52Z

packages/flutter/lib/src/widgets/undo_history.dart

why invoke this twice?

Good catch, i'll use the variable already defined.

LongCatIsLooong · 2023-11-29T06:16:51Z

packages/flutter/lib/src/widgets/editable_text.dart

The PR description seems to indicate clearing the composing region was to prevent the android input connection from restarting but the comment is a bit different? Is it the TextInputPlugin that's resetting the composing region or it's the undo mechanism?
(also IIRC android doesn't have a built-in undo/redo functionality, the native behavior refers to what Gboard does right?)

It is the TextInputPlugin restarting the input here due to the composing region being changed when we do an undo/redo on the framework side.

The Google Keep app has in-app Undo/Redo controls and this is the behavior I observed.

The Google Keep app has in-app Undo/Redo

I'm being pedantic, but that's not the "native" behavior right? It's just one app that chose to implement it that way.

It is the TextInputPlugin restarting the input here due to the composing region being changed when we do an undo/redo on the framework side.

but the comment is saying this is to prevent the text input plugin from resetting the composing region?

Yeah that is true, I have updated the comments to better reflect this and match the pr description.

…fier

LongCatIsLooong

LGTM modulo comments.

)

flutter/flutter@918e336...d861ce4 2023-12-02 [email protected] Roll Flutter Engine from f0122c32c5cc to cfabe42bc0c6 (1 revision) (flutter/flutter#139423) 2023-12-02 [email protected] Roll Flutter Engine from f23c33f3831c to f0122c32c5cc (1 revision) (flutter/flutter#139422) 2023-12-02 [email protected] Roll Flutter Engine from d441f087052c to f23c33f3831c (2 revisions) (flutter/flutter#139421) 2023-12-02 [email protected] Roll Flutter Engine from 27d37db84b8e to d441f087052c (1 revision) (flutter/flutter#139419) 2023-12-02 [email protected] Roll Flutter Engine from 5a9f33e3a41e to 27d37db84b8e (1 revision) (flutter/flutter#139418) 2023-12-02 [email protected] Roll Flutter Engine from 9f8502c4e255 to 5a9f33e3a41e (1 revision) (flutter/flutter#139416) 2023-12-02 [email protected] Roll Flutter Engine from 43a1598713bb to 9f8502c4e255 (1 revision) (flutter/flutter#139414) 2023-12-02 [email protected] Roll Flutter Engine from 039439c1ffe8 to 43a1598713bb (1 revision) (flutter/flutter#139412) 2023-12-02 [email protected] Roll Flutter Engine from 4d19fedb7617 to 039439c1ffe8 (1 revision) (flutter/flutter#139410) 2023-12-02 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Retry on transient Skia failure." (flutter/flutter#139407) 2023-12-01 [email protected] Roll Flutter Engine from 162ad29a576f to 4d19fedb7617 (1 revision) (flutter/flutter#139397) 2023-12-01 [email protected] [l10n] Update Material shareButtonLabel (flutter/flutter#138899) 2023-12-01 [email protected] Retry on transient Skia failure. (flutter/flutter#139182) 2023-12-01 [email protected] Roll Flutter Engine from 820cb686d17d to 162ad29a576f (1 revision) (flutter/flutter#139394) 2023-12-01 [email protected] Roll Flutter Engine from 00316e4b7680 to 820cb686d17d (2 revisions) (flutter/flutter#139390) 2023-12-01 [email protected] Roll Flutter Engine from 95995c48d591 to 00316e4b7680 (1 revision) (flutter/flutter#139389) 2023-12-01 [email protected] Roll Flutter Engine from 69f0e5550702 to 95995c48d591 (6 revisions) (flutter/flutter#139388) 2023-12-01 [email protected] Added vscode-insiders path installed via snap (flutter/flutter#137117) 2023-12-01 [email protected] Typo fix in dartdoc in tool test (flutter/flutter#139386) 2023-12-01 [email protected] Roll Flutter Engine from 51ef7642750f to 69f0e5550702 (1 revision) (flutter/flutter#139348) 2023-12-01 [email protected] Roll Flutter Engine from 894360cca1ec to 51ef7642750f (1 revision) (flutter/flutter#139346) 2023-12-01 [email protected] Roll Flutter Engine from c26e6ced11df to 894360cca1ec (1 revision) (flutter/flutter#139345) 2023-12-01 [email protected] Roll Flutter Engine from 74d2df52514a to c26e6ced11df (26 revisions) (flutter/flutter#139342) 2023-12-01 [email protected] Roll Flutter Engine from 35939ca8534f to 74d2df52514a (1 revision) (flutter/flutter#139264) 2023-11-30 [email protected] Add `undoStackModifier` to `UndoHistory` (flutter/flutter#138674) 2023-11-30 [email protected] Migrate docs_test to shard. (flutter/flutter#139282) 2023-11-30 [email protected] Write Tests for API Examples of `cupertino_text_field.0`, `data_table.0`, `icon_button.2` & `ink_well.0` (flutter/flutter#139258) 2023-11-30 [email protected] Refactor prepare_package.dart (flutter/flutter#139277) 2023-11-30 [email protected] Roll Packages from e4aaba8 to bc72d15 (4 revisions) (flutter/flutter#139307) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages 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 Packages: 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

This change adds a feature to `UndoHistory` that allows the user to modify the value being pushed onto the undo stack. This is used by the framework to ignore the composing region when pushing history entries to the Undo stack on Android. This is so an undo does not trigger an input connection restart by the Android TextInputPlugin, which occurs when the framework changes the composing region. This is also the native platform behavior observed in Google Keep app on Android, where doing an undo during composing reverts to the previous state but with composing inactive and a subsequent redo does not bring back the composing region. Fixes flutter#130881 Partial fix for flutter#134398

)

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Nov 18, 2023

Renzo-Olivares changed the title ~~Add historyModifier to UndoHistory~~ Add undoStackModifier to UndoHistory Nov 19, 2023

Renzo-Olivares marked this pull request as ready for review November 20, 2023 04:27

Renzo-Olivares requested review from LongCatIsLooong and hellohuanlin and removed request for hellohuanlin November 20, 2023 17:08

LongCatIsLooong reviewed Nov 21, 2023

View reviewed changes

Renzo-Olivares requested a review from LongCatIsLooong November 22, 2023 01:46

hellohuanlin reviewed Nov 28, 2023

View reviewed changes

Renzo-Olivares force-pushed the undo-history-modifier branch from bc71182 to b729891 Compare November 29, 2023 00:14

LongCatIsLooong reviewed Nov 29, 2023

View reviewed changes

Renzo-Olivares added 8 commits November 28, 2023 23:08

Add history modifier

7e7eb28

fix analyzer

f571d74

fix tests

06e13af

fix analyzer

7fe49d3

rename

51ed9df

protect from duplicate _push

bac629c

Add some documentation for android specific behavior of undoStackModi…

022022f

…fier

address comments

175605e

Renzo-Olivares force-pushed the undo-history-modifier branch from cc3adbb to 175605e Compare November 29, 2023 07:09

Renzo-Olivares requested review from LongCatIsLooong and hellohuanlin November 29, 2023 19:18

LongCatIsLooong approved these changes Nov 29, 2023

View reviewed changes

update comments

48245cc

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

auto-submit bot merged commit 75011b2 into flutter:master Nov 30, 2023

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

Roll Flutter from 918e336014e1 to e5762baf2497 (10 revisions) flutter/packages#5531

Closed

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

75011b2 Add undoStackModifier to UndoHistory (flutter/flutter#138674

c605b3a

)

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

Roll Flutter from 918e336014e1 to e5762baf2497 (10 revisions) flutter/packages#5534

Closed

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

75011b2 Add undoStackModifier to UndoHistory (flutter/flutter#138674

8181f37

)

HansMuller mentioned this pull request Dec 1, 2023

Flutter Framework Team: Notable Commits #121415

Closed

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

Manual roll Flutter from 918e336014e1 to 1e19113852c6 (22 revisions) flutter/packages#5539

Closed

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

75011b2 Add undoStackModifier to UndoHistory (flutter/flutter#138674

063ecea

)

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

Roll Flutter from 918e336014e1 to d861ce496c9e (29 revisions) flutter/packages#5541

Closed

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

75011b2 Add undoStackModifier to UndoHistory (flutter/flutter#138674

39c8668

)

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

Roll Flutter from 918e336014e1 to d861ce496c9e (29 revisions) flutter/packages#5542

Merged

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

75011b2 Add undoStackModifier to UndoHistory (flutter/flutter#138674

529d226

)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

75011b2 Add undoStackModifier to UndoHistory (flutter/flutter#138674

ce4a47d

)

Add undoStackModifier to UndoHistory #138674

Add undoStackModifier to UndoHistory #138674

Uh oh!

Conversation

Renzo-Olivares commented Nov 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Renzo-Olivares commented Nov 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Nov 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Nov 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `undoStackModifier` to `UndoHistory` #138674

Add `undoStackModifier` to `UndoHistory` #138674

Renzo-Olivares commented Nov 18, 2023 •

edited

Loading

Renzo-Olivares Nov 21, 2023 •

edited

Loading

Renzo-Olivares Nov 29, 2023 •

edited

Loading