~eliasnaur/gio#438: 
Undo/Redo in the editor

Hello, I had installed an undo/redo mechanism around the editor that worked both with shortcut keys and icons.

It was a bit complicated because I had to search for what had changed each time I changed the text and put it in a ring buffer (FYI I used to group the characters entered as long as there was no pause of more than 0.8s on the keyboard input, which gives bits of text that roughly correspond to the user's train of thought).

With this change, it's much more efficient.

However, for this to work with the icons in the new version of the editor, the two functions Undo and Redo would have to be exported, so that I could call them when the user clicks on the corresponding icons.
Is this possible?

Thanks in advance.

Status
REPORTED
Submitter
~fabien
Assigned to
No-one
Submitted
4 months ago
Updated
2 months ago
Labels
No labels applied.

~whereswaldon 4 months ago

I have no issue with it if you'd like to submit a patch. I didn't export them initially because I wasn't sure about how to handle other features I expect people to want, like measuring history depth or manipulating the history to save/restore it.

~fabien 4 months ago

You're right, these features are also needed in my case.
Let me think a bit more about it.
If other guys have ideas, please let me know.

~fabien 4 months ago*

My use case that requires saving the history is when I have several texts that I want to edit in turn with the same editor widget and for which I want to keep the history available separately.
To manage this case, I suggest adding the functions

  • (e *Editor) GetHistory() *HistoryData
  • (e *Editor) SetHistory(history *HistoryData)

With the possibility to assign nil to the history argument if you want to clear the history.
Is this the use case you had in mind?

~fabien 4 months ago

Actually, the ability to clear the history is vital because in the current situation, when the editor is displayed with text loaded from the start, the initial load is already in the history and a Ctrl-Z clears the initial text, which is annoying.

~whereswaldon 4 months ago

I was reluctant to expose this stuff because I didn't want to commit to the history representation, but I think you're right and your use case is valid.

We could export the current modification type, then provide:

func (e *Editor) History() []Modification
func (e *Editor) SetHistory(modifications []Modification)

Care to submit a patch adding these with appropriate tests?

~fabien 3 months ago

I assume that nextHistoryIdx also needs to be saved and that's why I proposed a HistoryData type that encompasses both.

If you are OK with that, I can submit a patch with tests.

~whereswaldon 3 months ago

Indeed, that index is important. Maybe just call the type History? Or perhaps EditHistory? But yes, I'd be happy for you to submit a patch with that.

~fabien 3 months ago

I submitted a patch yesterday, but I haven't received any feedback from the build yet.

~whereswaldon 3 months ago

Your email must not have gone through. I don't see it: https://lists.sr.ht/~eliasnaur/gio-patches

~fabien 3 months ago

I sent the patch back 2,5 hours ago, but again I didn't get a return.
However, I received the email as a copy, so it's gone.

~whereswaldon 3 months ago

I sent the patch back 2,5 hours ago, but again I didn't get a return. However, I received the email as a copy, so it's gone.

I'm sorry, I don't understand. Could you try sending your patch again or perhaps using the sr.ht GUI patchset mechanism?

To quote this page:

If you have a sourcehut account, you can also fork the Gio repository, push your changes to that and use the web-based flow for emailing the patch. Start the process by clicking the “Prepare a patchset” button on the front page of your fork.

~fabien 3 months ago

I give up, it's been 5 tries and still no answer.
However, I'm doing exactly the same thing as when I sent my previous patch which worked well.
And I can't do it with the web-based flow either.

If you are interested, I can send you the editor.go and editor_test.go files.

~whereswaldon 3 months ago

You could open a pull request on the GitHub mirror? https://github.com/gioui/gio

~fabien 3 months ago*

I tried on GitHub which gives more information about what is going on
The problem seems to come from this check

  • builds.sr.ht: apple.yml — builds.sr.ht job failed

test_macos

+ cd gio
+ export PATH=/home/build/appletools/bin:/home/build/sdk/go/bin:/home/build/go/bin:/usr/bin
+ PATH=/home/build/appletools/bin:/home/build/sdk/go/bin:/home/build/go/bin:/usr/bin
+ CC=/home/build/appletools/tools/clang-macos
+ GOOS=darwin
+ CGO_ENABLED=1
+ go build ./...
go: downloading golang.org/x/image v0.0.0-20210628002857-a66eb6448b8d
go: downloading github.com/benoitkugler/textlayout v0.1.1
go: downloading github.com/go-text/typesetting v0.0.0-20220411150340-35994bc27a7b
go: downloading github.com/gioui/uax v0.2.1-0.20220325163150-e3d987515a12
go: downloading gioui.org/cpu v0.0.0-20210817075930-8d6a761490d2
go: downloading gioui.org/shader v1.0.6
go: downloading golang.org/x/exp v0.0.0-20210722180016-6781d3edade3
go: downloading golang.org/x/text v0.3.7
# gioui.org/app
os_darwin.m:3:9: fatal error: 'Dispatch/Dispatch.h' file not found`

I'm not sure if this has anything to do with the patch I submitted

~whereswaldon 3 months ago

I'm pretty confident that macOS CI is broken for other reasons, so don't worry too much about that. Thanks for the PR, and especially for the effort of submitting the change so many times. We just need ~eliasnaur's feedback on it.

~fabien 3 months ago*

I realized that History() and SetHistory() had to make a deep copy, otherwise the copied versions of the history were also modified by the subsequent changes made in the editor.
I made the changes in GitHub but I don't know how to do /modify the pull request, knowing that the previous one has not been validated yet.
I've also added the corresponding tests

~eliasnaur 2 months ago

My use case that requires saving the history is when I have several texts that I want to edit in turn with the same editor widget and for which I want to keep the history available separately. To manage this case, I suggest adding the functions

(e *Editor) GetHistory() *HistoryData (e *Editor) SetHistory(history *HistoryData)

With the possibility to assign nil to the history argument if you want to clear the history. Is this the use case you had in mind?

What's the use of Get/SetHistory (or the History field) compared to replacing the underlying widget.Editor instance when switching between content? It seems to me an opaque widget.History (or Modifications) is bound to the current text, IME state etc in the Editor and is thus hard to separate.

~whereswaldon 2 months ago

One thought is that I'd really like to eventually be able to save/restore the undo history across application restarts. I figured that allowing it to be exported and swapped was incremental progress in that direction. I don't think I know enough about the IME side of this to comment there.

~fabien 2 months ago

Context: I have a large number of texts in a database.
The titles of these texts are displayed in a tree-list and the content of the text selected in this list is loaded from the database into the editor.
A modified text is written to the database as soon as another text is selected (or when the program exits). But if you reselect a text that has been modified since the opening of the application, the modification history is reassociated with the text.
I had developed this function outside of the editor, but the introduction of undo/redo inside the editor, forces me to modify my application.

Compared to replacing the underlying widget.Editor instance when switching between content, the use of the History field avoids to keep in memory the whole text and IME for all the modified texts (except for the one selected of course).

~eliasnaur 2 months ago

Thank you for the context.

Compared to replacing the underlying widget.Editor instance when switching between content, the use of the History field avoids to keep in memory the whole text and IME for all the modified texts (except for the one selected of course).

I see. It seems to me that History is a part fix for a general issue (that Editor don't work well with very large texts). It seems more natural to somehow abstract the underlying text storage so that the user can provide a storage method.

~fabien 2 months ago

It seems to me that History is a part fix for a general issue (that Editor don't work well with very large texts). It seems more natural to somehow abstract the underlying text storage so that the user can provide a storage method.

I understand. Unfortunately, I don't have the time to get into it.

~fabien 2 months ago

I did some tests and I can adapt my application by replacing the underlying widget.Editor instance and keeping in memory only those widget.Editor that have been modified.

To do this, I still need undo and redo to be exported and also

  • that Editor.SetText resets the modification history
  • a new Editor.Modified function (or whatever name you prefer) that returns a bool indicating if the text has been modified. If the history is not exported, we need to be able to detect if the text is back to its initial state even if it has been modified and the modifications have all been undone.

~eliasnaur if you agree with this approach, I will modify the PR accordingly.

~eliasnaur 2 months ago

SGTM. Perhaps Editor.Modified should be spelled CanUndo in case we ever want the same for redo.

~fabien 2 months ago*

Perhaps Editor.Modified should be spelled CanUndo in case we ever want the same for redo.

Good idea
Would-you like me to add the CanRedo as well ?

Register here or Log in to comment, or comment via email.