Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add undoStackModifier to UndoHistory #138674

Merged
merged 9 commits into from
Nov 30, 2023

Conversation

Renzo-Olivares
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares commented Nov 18, 2023

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.

@github-actions 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 Renzo-Olivares changed the title Add historyModifier to UndoHistory Add undoStackModifier to UndoHistory Nov 19, 2023
@Renzo-Olivares Renzo-Olivares marked this pull request as ready for review November 20, 2023 04:27
@Renzo-Olivares Renzo-Olivares requested review from hellohuanlin and LongCatIsLooong and removed request for hellohuanlin November 20, 2023 17:08
@Renzo-Olivares
Copy link
Contributor Author

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.


_throttleTimer = _throttledPush(widget.value.value);
_throttleTimer = _throttledPush(_lastValue as T);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case it would just push one entry.

if (widget.value.value == _lastValue) {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (value == currentValue) {
return;
}

@@ -4823,6 +4823,9 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien

return oldValue.text != newValue.text || oldValue.composing != newValue.composing;
},
undoStackModifier: (TextEditingValue value) {
return defaultTargetPlatform == TargetPlatform.android ? value.copyWith(composing: TextRange.empty) : value;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return;
}

_lastValue = widget.undoStackModifier?.call(widget.value.value) ?? widget.value.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

why invoke this twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// On Android we should discard the composing region when pushing
// a new entry to the undo stack. This is matches the native
// behavior and prevents the TextInputPlugin from resetting the
// composing region on every undo/redo.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

@Renzo-Olivares Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 30, 2023
@auto-submit auto-submit bot merged commit 75011b2 into flutter:master Nov 30, 2023
70 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 2, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Dec 2, 2023
flutter/flutter@918e336...d861ce4

2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from f0122c32c5cc to cfabe42bc0c6 (1 revision) (flutter/flutter#139423)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from f23c33f3831c to f0122c32c5cc (1 revision) (flutter/flutter#139422)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from d441f087052c to f23c33f3831c (2 revisions) (flutter/flutter#139421)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 27d37db84b8e to d441f087052c (1 revision) (flutter/flutter#139419)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5a9f33e3a41e to 27d37db84b8e (1 revision) (flutter/flutter#139418)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9f8502c4e255 to 5a9f33e3a41e (1 revision) (flutter/flutter#139416)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 43a1598713bb to 9f8502c4e255 (1 revision) (flutter/flutter#139414)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 039439c1ffe8 to 43a1598713bb (1 revision) (flutter/flutter#139412)
2023-12-02 engine-flutter-autoroll@skia.org 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 engine-flutter-autoroll@skia.org Roll Flutter Engine from 162ad29a576f to 4d19fedb7617 (1 revision) (flutter/flutter#139397)
2023-12-01 leroux_bruno@yahoo.fr [l10n] Update Material shareButtonLabel (flutter/flutter#138899)
2023-12-01 ian@hixie.ch Retry on transient Skia failure. (flutter/flutter#139182)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 820cb686d17d to 162ad29a576f (1 revision) (flutter/flutter#139394)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 00316e4b7680 to 820cb686d17d (2 revisions) (flutter/flutter#139390)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 95995c48d591 to 00316e4b7680 (1 revision) (flutter/flutter#139389)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 69f0e5550702 to 95995c48d591 (6 revisions) (flutter/flutter#139388)
2023-12-01 kristijan.zic@gmail.com Added vscode-insiders path installed via snap (flutter/flutter#137117)
2023-12-01 mdebbar@google.com Typo fix in dartdoc in tool test (flutter/flutter#139386)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 51ef7642750f to 69f0e5550702 (1 revision) (flutter/flutter#139348)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 894360cca1ec to 51ef7642750f (1 revision) (flutter/flutter#139346)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from c26e6ced11df to 894360cca1ec (1 revision) (flutter/flutter#139345)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 74d2df52514a to c26e6ced11df (26 revisions) (flutter/flutter#139342)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 35939ca8534f to 74d2df52514a (1 revision) (flutter/flutter#139264)
2023-11-30 rmolivares@renzo-olivares.dev Add `undoStackModifier` to `UndoHistory` (flutter/flutter#138674)
2023-11-30 godofredoc@google.com Migrate docs_test to shard. (flutter/flutter#139282)
2023-11-30 pateltirth454@gmail.com 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 christopherfujino@gmail.com Refactor prepare_package.dart (flutter/flutter#139277)
2023-11-30 engine-flutter-autoroll@skia.org 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com 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
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
…r#5542)

flutter/flutter@918e336...d861ce4

2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from f0122c32c5cc to cfabe42bc0c6 (1 revision) (flutter/flutter#139423)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from f23c33f3831c to f0122c32c5cc (1 revision) (flutter/flutter#139422)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from d441f087052c to f23c33f3831c (2 revisions) (flutter/flutter#139421)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 27d37db84b8e to d441f087052c (1 revision) (flutter/flutter#139419)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5a9f33e3a41e to 27d37db84b8e (1 revision) (flutter/flutter#139418)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9f8502c4e255 to 5a9f33e3a41e (1 revision) (flutter/flutter#139416)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 43a1598713bb to 9f8502c4e255 (1 revision) (flutter/flutter#139414)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 039439c1ffe8 to 43a1598713bb (1 revision) (flutter/flutter#139412)
2023-12-02 engine-flutter-autoroll@skia.org 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 engine-flutter-autoroll@skia.org Roll Flutter Engine from 162ad29a576f to 4d19fedb7617 (1 revision) (flutter/flutter#139397)
2023-12-01 leroux_bruno@yahoo.fr [l10n] Update Material shareButtonLabel (flutter/flutter#138899)
2023-12-01 ian@hixie.ch Retry on transient Skia failure. (flutter/flutter#139182)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 820cb686d17d to 162ad29a576f (1 revision) (flutter/flutter#139394)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 00316e4b7680 to 820cb686d17d (2 revisions) (flutter/flutter#139390)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 95995c48d591 to 00316e4b7680 (1 revision) (flutter/flutter#139389)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 69f0e5550702 to 95995c48d591 (6 revisions) (flutter/flutter#139388)
2023-12-01 kristijan.zic@gmail.com Added vscode-insiders path installed via snap (flutter/flutter#137117)
2023-12-01 mdebbar@google.com Typo fix in dartdoc in tool test (flutter/flutter#139386)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 51ef7642750f to 69f0e5550702 (1 revision) (flutter/flutter#139348)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 894360cca1ec to 51ef7642750f (1 revision) (flutter/flutter#139346)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from c26e6ced11df to 894360cca1ec (1 revision) (flutter/flutter#139345)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 74d2df52514a to c26e6ced11df (26 revisions) (flutter/flutter#139342)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 35939ca8534f to 74d2df52514a (1 revision) (flutter/flutter#139264)
2023-11-30 rmolivares@renzo-olivares.dev Add `undoStackModifier` to `UndoHistory` (flutter/flutter#138674)
2023-11-30 godofredoc@google.com Migrate docs_test to shard. (flutter/flutter#139282)
2023-11-30 pateltirth454@gmail.com 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 christopherfujino@gmail.com Refactor prepare_package.dart (flutter/flutter#139277)
2023-11-30 engine-flutter-autoroll@skia.org 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com 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
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…r#5542)

flutter/flutter@918e336...d861ce4

2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from f0122c32c5cc to cfabe42bc0c6 (1 revision) (flutter/flutter#139423)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from f23c33f3831c to f0122c32c5cc (1 revision) (flutter/flutter#139422)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from d441f087052c to f23c33f3831c (2 revisions) (flutter/flutter#139421)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 27d37db84b8e to d441f087052c (1 revision) (flutter/flutter#139419)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5a9f33e3a41e to 27d37db84b8e (1 revision) (flutter/flutter#139418)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9f8502c4e255 to 5a9f33e3a41e (1 revision) (flutter/flutter#139416)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 43a1598713bb to 9f8502c4e255 (1 revision) (flutter/flutter#139414)
2023-12-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 039439c1ffe8 to 43a1598713bb (1 revision) (flutter/flutter#139412)
2023-12-02 engine-flutter-autoroll@skia.org 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 engine-flutter-autoroll@skia.org Roll Flutter Engine from 162ad29a576f to 4d19fedb7617 (1 revision) (flutter/flutter#139397)
2023-12-01 leroux_bruno@yahoo.fr [l10n] Update Material shareButtonLabel (flutter/flutter#138899)
2023-12-01 ian@hixie.ch Retry on transient Skia failure. (flutter/flutter#139182)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 820cb686d17d to 162ad29a576f (1 revision) (flutter/flutter#139394)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 00316e4b7680 to 820cb686d17d (2 revisions) (flutter/flutter#139390)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 95995c48d591 to 00316e4b7680 (1 revision) (flutter/flutter#139389)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 69f0e5550702 to 95995c48d591 (6 revisions) (flutter/flutter#139388)
2023-12-01 kristijan.zic@gmail.com Added vscode-insiders path installed via snap (flutter/flutter#137117)
2023-12-01 mdebbar@google.com Typo fix in dartdoc in tool test (flutter/flutter#139386)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 51ef7642750f to 69f0e5550702 (1 revision) (flutter/flutter#139348)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 894360cca1ec to 51ef7642750f (1 revision) (flutter/flutter#139346)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from c26e6ced11df to 894360cca1ec (1 revision) (flutter/flutter#139345)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 74d2df52514a to c26e6ced11df (26 revisions) (flutter/flutter#139342)
2023-12-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 35939ca8534f to 74d2df52514a (1 revision) (flutter/flutter#139264)
2023-11-30 rmolivares@renzo-olivares.dev Add `undoStackModifier` to `UndoHistory` (flutter/flutter#138674)
2023-11-30 godofredoc@google.com Migrate docs_test to shard. (flutter/flutter#139282)
2023-11-30 pateltirth454@gmail.com 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 christopherfujino@gmail.com Refactor prepare_package.dart (flutter/flutter#139277)
2023-11-30 engine-flutter-autoroll@skia.org 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undo/Redo history disappears on Japanese keyboard
3 participants