Any better name than onChanged or is that ok? It's a little confusing because it's only called on undo/redo, not when widget.controller changes for other reasons.

Maybe onTriggered?

It doesn't matter for UndoRedo, but I couldn't get this to work with more than one argument because it doesn't seem possible to refer to all arguments in Dart. Maybe Dart isn't meant for this kind of functional programming.

Maybe define a constant instead?

also curious how we are going to support different platform behavior? macOS seems to have a vary unique way of determine the redo undo

Will do the constant 👍 .

My plan is to not support different platform behavior for now. The different platforms seem so subtly different in weird ways and I'm hoping users don't care. I will monitor this and be ready to implement platform-specific behavior if anyone wants it, though.

The worst difference I can see now is that if you type very slowly, then the undo/redo chunks are small. On native Mac, the chunks are of a more consistent size regardless of the speed that you type.

If I do need to implement platform specific behavior here, and it's more complicated than just different Durations, then my general plan would be to make UndoRedoMac etc. widgets.

this can be private

Good catch, that's why I should autogenerate these things.

I guess it's too late for this topic and you've probably had the discussion, but UndoRedo feels too "real" for how abstract its functionality is. Have we considered something like TextHistoryProvider?

Actually I think you're probably right... Maybe TextEditingHistory?

Maybe?

Yeah that's more accurate 👍

👍 I think "these" was a typo.

I understand that UndoRedo is split for code modularity and is meant to be private but testable. But can we just test EditableText in all of the following tests? The behavior of EditableText is what the users care and we maintain backward compatibility of. By making UndoRedo a private class, you're saying it is subject to refactory or breaking changes in the future, but at that time these tests will have to be migrated.

I usually write small building blocks like this and then test them to convince myself that they work the way that I expect them to. It's useful for me personally to have the tests even if we refactor or change behavior. I don't want to burden everyone else if that's not Flutter's style though...

The problem is mostly for visibleForTesting since it's not enforced by the compiler but by the linter, which the developer can turn off. The last time (which is a long time ago) I was told that it was discouraged (but not actually banned.) But I do think it reveals a problem where you're testing something that the users won't see, so might not be kept compatible during future development at all, i.e. adding unnecessary compatibility requirements. Anyway, just a soft suggestion.

Ok thanks for the explanation. I probably am going against Flutter trends a little bit. I'll keep an eye out for similar patterns and problems with this code and consider changing it then. It wouldn't be terrible to just test the widget.

+1 to what @dkwingsmt has said, it feels like these tests are testing implementation detail.

Alright I'm outnumbered, I'll remove the tests and make the class fully private. I'll also add another test to TextEditingHistory to make sure all of those cases are covered.

Can we keep this private?

Yes good catch, I'm no longer using this directly in the tests, so it can be fully private.

This can be reduced to value notifier according to the doc comment.

not too much of an issue if this is a private class though

Ah this is a comment that I forgot to update, I'll fix it.

Originally I tried to make this widget work with any type of ValueNotifier, but it turns out that there is a good amount of text-editing-specific logic that is needed to get undo/redo to work with EditableText. For example, ignoring selection and composing updates. I think it makes sense to just put that here since we have no use for a more generic undo/redo widget now.

if we are not modifying the value in controller in this widget a ValueNotifier of TextEditingValue should be enough, no?

but since this class can be private, I think it is probably fine as is

Maybe explain a bit why the state may be the same?

It looks like the code prevent duplicate history in _push method.

if (widget.controller.value == TextEditingValue.empty
        || widget.controller.text == _stack.currentValue?.text) {
      return;
    }

It does, but it's still possible to be called without a state change, such as when the user presses cmd+shift+z to redo when there is nothing to redo. I'll make this clear in the docs, good catch.

This comment is redundant

Good call, I think I was thinking this was public and needed a comment.

I am curious whether we may want to reset the history in this case?

If we don't, we may need to call _push() here

Good catch. I'm going to clear the stack here. I'm not sure if there is a real use case for ever changing the controller like this, but I'm guessing that if there is, the whole field is being reset and the history should reset too.

_list is a private property, we should avoid mention it in public doc

It feels weird that it has three different behavior.

1. returns null if empty
2. returns entry if undo
3. return same entry if can't undo

I feel 1 and 3 should be treated the same so that the caller doesn't need to handle three cases

same for redo

I also went back and forth on this when I wrote this code. I think of it like this:

1. returns the current value (after any operation) in all cases.

If the stack is empty, then there is no current value, so it returns null. Otherwise it returns what the state should be after the undo/redo, even if nothing changed.

I'll make this a little more clear in the comments and the code.

+1 to what @dkwingsmt has said, it feels like these tests are testing implementation detail.

I also went back and forth on this when I wrote this code. I think of it like this:

returns the current value (after any operation) in all cases.
If the stack is empty, then there is no current value, so it returns null. Otherwise it > returns what the state should be after the undo/redo, even if nothing changed.

This is also related the onChange called with the same state. why the onChange needs to be fired if the state is the same? what is the difference to the caller for return null vs returning the same state?

also just noticed the same state will get short-circuited in _update method

Ah good catch. I think it should only be called when the value changes so I'll update the docs.

continue on the called with the same state.

Isn't the _update method short-circuited if the state is the same (line 4233)?

Updated the docs.

also just noticed the same state will get short-circuited in _update method

Do we need to expose the throttle and its related function signature? I think we can testing the edtiabletext direction should be good enough

Yeah it's the same argument as the other comment thread. I'll remove it.