[SuperTextField] - Create a controller that supports undo/redo (Resolves #189) #704
Conversation
matthew-carroll
changed the title
|
@venkatd - would you like to give this PR a try in your app and see if this undo/redo works for you? |
|
Hi @matthew-carroll I really like the direction of this. We would want the ability to compose/group actions in order to replace our internal implementation. Having commands produce edits (or changes/events depending on ones preference) gives us undo and grouping for free in many cases. If your time allows a quick call this month could let us compare notes on the approach and how we might be able to drop our hand-rolled undo/redo.
Could you give an example of what you mean by this? |
|
|
||
| group("deletes text", () { | ||
| test("between the caret and the beginning of the line", () { | ||
| // TODO: |
There was a problem hiding this comment.
Should these tests be implemented in this PR?
| }); | ||
|
|
||
| test("pastes text from clipboard", () { | ||
| // TODO: |
There was a problem hiding this comment.
Should this test be implemented in this PR?
| _previousComposingRange = previousValue.composingRegion; | ||
|
|
||
| return AttributedTextEditingValue( | ||
| text: previousValue.text, |
There was a problem hiding this comment.
Shouldn't the text be copied here like in the other methods?
There was a problem hiding this comment.
I don't think it matters because we're not changing the text, but I see that I copied the text in the "select all" command, so I'll add a copy() here, too.
| extension on AttributedText { | ||
| AttributedText copy() { | ||
| return AttributedText( | ||
| text: text, | ||
| spans: spans.copy(), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn't it be better if this extension was placed after all the commands instead of being placed between two commands?
There was a problem hiding this comment.
Yeah, I'll move it down.
|
@venkatd it sounds like what you need is the ability for a command to look at recent history and re-group them, right? In terms of what's introduced in this PR, are there any API decisions here that you think would prevent adoption of this approach? Should we merge in and then expand the behavior, or do you think we need to revisit what's here so far? |
matthew-carroll
force-pushed
the
189_supertextfield-undo-redo
branch
from
cd0e532
to
e2be77d
Compare
|
@angelosilvestre - I updated the PR for your comments. I implemented one of those empty tests and deleted the other two. I also found a bug in the Markdown serialization. It was breaking on a single-character styles. I wrote a test suite and re-organized the code. |
…ng to port the existing controller API to commands.
matthew-carroll
force-pushed
the
189_supertextfield-undo-redo
branch
from
e2be77d
to
2401593
Compare
|
FYI - I extracted the markdown serialization fixes from this PR and merged them separately. So now they've disappeared from this PR's file changes. |
[SuperTextField] - Create a controller that supports undo/redo.
This PR creates an
EventSourcedAttributedTextEditingValue, which holds text, a selection, and a composing region, along with a history of commands that were run to produce the current value. The value canundo()andredo()those commands.The commands are based on an interface called
AttributedTextEditingValueCommand.I ported the existing
AttributedTextEditingControllerinterface over to a new one calledEventSourcedAttributedTextEditingController. I implemented the interface using the new event sourced value and a group of new commands. I also wrote tests for nearly every operation in the controller, and their ability to undo an operation.Some things that are not addressed in this PR: