Add support for iOS UndoManager#98294
Conversation
|
I believe this is a reasonable approach – I think it may make sense to swap over to something along the lines of |
|
@justinmc I'd like to get your thoughts on the framework side of things – right now I'm adding a method to |
justinmc
left a comment
There was a problem hiding this comment.
Just to throw out a crazy idea: What if everything in the engine through to the platform message channel were separate from text editing? Since NSUndoManager itself is not specific to text editing, hypothetically this could be generic to any kind of undo/redo. In EditableText you would specifically use it for text editing. Just a thought I had, not necessarily a good idea.
Otherwise I like the idea you mentioned of an "UndoDelegate". Kind of similar to DeltaTextInputClient?
There was a problem hiding this comment.
Why is the post frame callback needed?
There was a problem hiding this comment.
Missing docs here and on canUndo above.
There was a problem hiding this comment.
Nit: Would it be better to send a boolean from the engine? It would save a little bit of space at least.
If not, maybe parse the string into an enum or compare it to constants.
There was a problem hiding this comment.
Actually, a boolean might be the best thing. I'm currently using an enum on the engine side, so I think just passing those values (0 and 1) instead of translating them to strings first might be the way to go here. Then instead of direction, this can be isRedo (or maybe split this into two methods, one to handle undo and one to handle redo)
|
Yes, I think the Having everything go through a different channel is an interesting idea, for sure. Other than needing to do the weird |
There was a problem hiding this comment.
this can be moved out of the both if(direction == 'undo') and else case so that we don't need to duplicate code
|
Thinking about this more, I really like the idea of separating this out from the text editing details. That way a drawing app could make use of it. I haven't had a chance to work on it this week (in the middle of moving), but I'm hoping to be back at it next week. Does it make sense to still use a FocusNode to know which undo client should receive messages or should that be something left up to the implementers? I think FocusNodes make sense for text editing, but I'm not sure about drawing. I think it still could, but maybe best left to others to decide. My initial thought is it would make sense to have some kind of |
|
I converted this back to draft status for now, since this is a work in progress. I added a new widget, I haven't updated lib/main.dartimport 'package:flutter/material.dart';
void main() {
runApp(const MyApp());
}
class MyApp extends StatelessWidget {
const MyApp({Key? key}) : super(key: key);
// This widget is the root of your application.
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
theme: ThemeData(
primarySwatch: Colors.blue,
),
home: const MyHomePage(title: 'Flutter Demo Home Page'),
);
}
}
class MyHomePage extends StatefulWidget {
const MyHomePage({Key? key, required this.title}) : super(key: key);
// This widget is the home page of your application. It is stateful, meaning
// that it has a State object (defined below) that contains fields that affect
// how it looks.
// This class is the configuration for the state. It holds the values (in this
// case the title) provided by the parent (in this case the App widget) and
// used by the build method of the State. Fields in a Widget subclass are
// always marked "final".
final String title;
@override
State<MyHomePage> createState() => _MyHomePageState();
}
class _MyHomePageState extends State<MyHomePage> {
final TextEditingController _controller = TextEditingController();
final FocusNode _focusNode = FocusNode();
final GlobalKey<UndoHistoryState> _undoKey = GlobalKey<UndoHistoryState>();
TextStyle? get enabledStyle => Theme.of(context).textTheme.bodyMedium;
TextStyle? get disabledStyle => Theme.of(context).textTheme.bodyMedium?.copyWith(color: Colors.grey);
bool _canUndo = false, _canRedo = false;
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
// Here we take the value from the MyHomePage object that was created by
// the App.build method, and use it to set our appbar title.
title: Text(widget.title),
),
body: Center(
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: <Widget>[
UndoHistory<TextEditingValue>(
key: _undoKey,
value: _controller,
onTriggered: (value) {
_controller.value = _controller.value.copyWith(text: value.text, selection: value.selection);
},
focusNode: _focusNode,
shouldChangeUndoStack: (TextEditingValue? oldValue, TextEditingValue newValue) {
if (oldValue == null) {
return true;
}
if (newValue == TextEditingValue.empty) {
return false;
}
return oldValue.text != newValue.text;
},
onUndoStackChanged: () => setState(() {
_canUndo = _undoKey.currentState?.canUndo ?? false;
_canRedo = _undoKey.currentState?.canRedo ?? false;
}),
child: TextField(
maxLines: 4,
controller: _controller,
focusNode: _focusNode,
),
),
Row(
children: [
TextButton(
child: Text('Undo', style: _canUndo ? enabledStyle : disabledStyle),
onPressed: () {
_undoKey.currentState?.undo();
}),
TextButton(
child: Text('Redo', style: _canRedo ? enabledStyle : disabledStyle),
onPressed: () {
_undoKey.currentState?.redo();
}),
],
),
],
),
),
);
}
}@justinmc Let me know your thoughts on that approach. I'm also thinking it might be worth adding another level of indirection around which client is currently connected to the |
|
I like that approach, looks really reusable. Would that replace _TextEditingHistory or is it separate? |
|
Cool, yes, I actually was able to get to the point of replacing There are a couple of things I don't love about the implementation right now, so let me know if you have thoughts on them:
|
|
All right, I swapped this over to use a controller and I think it's a lot cleaner: https://github.com/flutter/flutter/pull/98294/files#diff-0dd319c779a725fd887d8a664e656ce5cf92b62934a821cf2f3815750c630245R56 Let me know how that looks! |
|
CC @LongCatIsLooong who is probably also interested in this. We talked about the 3 finger swipe gesture, etc. before. |
justinmc
left a comment
There was a problem hiding this comment.
A few quick comments here, I owe you a full review by Monday. Really excited for this PR.
There was a problem hiding this comment.
Nit: Remove this boilerplate comment.
There was a problem hiding this comment.
Thanks for adding a full example! Even just for me as a reviewer it helps me understand this PR.
There was a problem hiding this comment.
Nit: Maybe use a template and macro for this repeated comment.
There was a problem hiding this comment.
Nit: Maybe add a "See also" with a reference to UndoManager.
There was a problem hiding this comment.
This actually needs to be removed since the TextInput channel is no longer used by this feature
There was a problem hiding this comment.
Nit: Add a "See also" with a reference to the Apple docs.
There was a problem hiding this comment.
Is this separate from undo/redo gestures (3 finger swipe)?
There was a problem hiding this comment.
No, that's a good point. I updated the comment to include the gestures.
There was a problem hiding this comment.
Very low chance of a breaking change here.
There was a problem hiding this comment.
Is that okay? Should we leave the typedef for now, just in case?
There was a problem hiding this comment.
We should remove it if it's not used by the framework.
If it passes the customer tests and the Google tests then I think it's ok to try merging it. If it does break anything, then the migration process should be pretty simple (users should include their own typedef in their app I guess?).
|
Gold has detected about 1 new digest(s) on patchset 24. |
There was a problem hiding this comment.
Mostly just small things, but one point I want to make sure everyone agrees on is the addition of the new controller (UndoHistoryController). I left a comment about that below.
Otherwise I really like this approach and the fact that undo/redo isn't tied to text editing.
Also I think the Skia Gold failure is not a problem, maybe try pushing a merge commit and see if it goes away?
There was a problem hiding this comment.
Nit: Maybe handlePlatformUndo? Since it's not only a keyboard thing.
There was a problem hiding this comment.
I like this way of organizing UndoHistory 👍 . So this TextEditingValue-specific logic goes here, and UndoHistory can take a generic type parameter.
There was a problem hiding this comment.
Nit: Periods at the end of these comments here and below.
There was a problem hiding this comment.
Nit: Maybe make this private since it's not used anywhere outside of this file.
There was a problem hiding this comment.
Nit: Maybe add a comment describing this function (even though I know there wasn't one before).
There was a problem hiding this comment.
Another nit here: I think that should actually be a doc comment with 3 slashes ///.
There was a problem hiding this comment.
By the way, I'm glad sendKeys will be reusable now. I've wanted it in other test files before.
There was a problem hiding this comment.
I wonder what others think of this controller. It doesn't exactly follow the pattern that something like TextEditingController does because its value is UndoHistoryValue, not the actual value that is undone/redone. It kind of wraps the two ChangeNotifiers, but doesn't deal in a value itself.
Maybe that is a good thing...
Ultimately the purpose of this controller is to allow programmatic undo/redo, right? Other options:
- Add this functionality to TextEditingController (though that's probably something we don't want to bloat).
- List for an Intent? Undo/RedoIntent. Maybe too indirect and confusing.
This might already be the best solution as-is, but I just wanted to make sure we discuss it before committing to anything.
There was a problem hiding this comment.
You are right that the UndoHistoryValue is more like a state than a value, so it is different from the TextEditingController in that way. That value is important for propagating the current undo/redo state back to the UI so that custom undo/redo interfaces can be built. The other two ChangeNotifiers are for sending events to allow programmatic undo/redo.
I don't love the idea of adding it to TextEditingController, or at least not only there, so that this can remain more generic: for example, someone could build a drawing interface with it and undo/redo lines on the drawing.
I could get on board with Intents, since we already use those for the keyboard shortcuts, though I agree that might be more confusing.
There was a problem hiding this comment.
Reading through this again I think I'm onboard with UndoHistoryController as-is. It makes a lot of sense in the example you included in the examples directory.
|
Gold has detected about 1 new digest(s) on patchset 25. |
There was a problem hiding this comment.
I'm currently investigating whether or not we should use a mixin in cases like this instead of an abstract class (to avoid breaking users that extend this if/when a method is added). I'll let you know what I figure out and then I owe you a full review.
There was a problem hiding this comment.
Here is the design doc. There's still some debate going on.
There was a problem hiding this comment.
Update: Making this a mixin and using it via with instead of implements is typically preferred. Anytime a new method is added here in the future, giving it an empty implementation will avoid breaking changes. If breaking changes are desired, a method can be added with no implementation.
c094938 to
2c73403
Compare
| child: Text('Undo', style: value.canUndo ? enabledStyle : disabledStyle), | ||
| onPressed: () { | ||
| _undoController.undo(); | ||
| }), |
There was a problem hiding this comment.
Here and below, split the closing }) onto two separate lines.
|
|
||
| /// A low-level interface to the system's undo manager. | ||
| /// | ||
| /// To receive events from the system UndoManager, create an |
There was a problem hiding this comment.
Nit: "UndoManager" => "undo manager" to match the previous paragraph?
| /// Set the [MethodChannel] used to communicate with the system's text input | ||
| /// control. | ||
| /// | ||
| /// This is only meant for testing within the Flutter SDK. Changing this | ||
| /// will break the ability to input text. This has no effect if asserts are | ||
| /// disabled. |
There was a problem hiding this comment.
This sounds like TextInputClient, can you tweak them so they're about undo/redo?
There was a problem hiding this comment.
Update: Making this a mixin and using it via with instead of implements is typically preferred. Anytime a new method is added here in the future, giving it an empty implementation will avoid breaking changes. If breaking changes are desired, a method can be added with no implementation.
| /// Will be true if there are past values on the stack. | ||
| bool get canUndo; | ||
|
|
||
| /// Will be false if there are future values on the stack. |
There was a problem hiding this comment.
Should this "false" be "true" or am I misreading it?
| UndoHistoryController({UndoHistoryValue? value}) : super(value ?? UndoHistoryValue.empty); | ||
|
|
||
| /// Notifies listeners that [undo] has been called. | ||
| final ChangeNotifier onUndo = ChangeNotifier(); |
There was a problem hiding this comment.
Should these ChangeNotifiers be disposed?
There was a problem hiding this comment.
Can it be private? It seems to be a way an API just to wire up the undo/redo to the UndoHistory widget.
There was a problem hiding this comment.
Possibly, but I think making it public makes this more useful to consumers, for example, if they are implementing their own version of UndoHistory, they would need to listen to the undo/redo notifications.
There was a problem hiding this comment.
Reading through this again I think I'm onboard with UndoHistoryController as-is. It makes a lot of sense in the example you included in the examples directory.
| } | ||
|
|
||
| @override | ||
| void handlePlatformUndo(String direction) { |
There was a problem hiding this comment.
In general I've been thinking about moving away from the pattern of passing raw, privately defined strings around from these platform methods.
Is there a way you could convert this direction to an enum or something that is publicly defined? Then in your tests you could also reference that instead of hardcoded "undo" and "redo" strings that I saw in a few places.
There was a problem hiding this comment.
Another nit here: I think that should actually be a doc comment with 3 slashes ///.
There was a problem hiding this comment.
By the way, I'm glad sendKeys will be reusable now. I've wanted it in other test files before.
chunhtai
left a comment
There was a problem hiding this comment.
left a reply with a suggestion, otherwise this LGTM
| /// If the state would still be the same before and after the undo/redo, this | ||
| /// will not be called. For example, receiving a redo when there is nothing | ||
| /// to redo will not call this method. | ||
| final void Function(T value) onTriggered; |
There was a problem hiding this comment.
I think this may be a corner case that is not so common in real life. I vote for just assert the value should stay the same after this method is called. If this does crash some use case, we can figure out how to handle it later. Also at that point we may have a better picture on how we should support this use case
…e, instead encoding that behavior in _push
|
@fbcouch Can you update this branch by merging in master? GitHub is saying there is a conflict, though it doesn't say what the conflict is... Otherwise I think we need to merge this! |
|
Thanks for sticking with this for so long, @fbcouch! |
|
I spoke a little too soon, we should make sure that this comment thread is resolved. I just left a comment explaining the composing text behavior. |
… after onTriggered call
justinmc
left a comment
There was a problem hiding this comment.
Renewing my LGTM with the latest changes 👍
Thanks for doing all the work to bring this back up to date. I'll plan to merge it tomorrow morning if no one else objects.
| /// If the state would still be the same before and after the undo/redo, this | ||
| /// will not be called. For example, receiving a redo when there is nothing | ||
| /// to redo will not call this method. | ||
| final void Function(T value) onTriggered; |
There was a problem hiding this comment.
I tried this out again and it seems to work for me on both iOS and Android with Gboard, where it correctly includes composing state in the undo history. I also tried the example above and got the assertion. Thanks!
Add support for iOS UndoManager
This PR is modifies the framework to allow the Undo/Redo support added by
_TextEditingHistoryto interact with the iOSUndoManager. That should enable 3-finger/shake gestures to undo/redo text editing and enable undo/redo from the iPad keyboard.I don't love this approach, since it feels a little bit hacky and requires a breaking change to the TextInputClient interface. It's possible #98280 could enable a better way of doing this.
I haven't had a chance to test this out – still waiting for the engine to compile, but wanted to plant the flag on this and possibly feedback on the approach.Related to #34749 and #77614
Engine PR: flutter/engine#31415
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.