Fix undo/redo for multi byte runes #5010
Conversation
widget/entry.go
Outdated
| @@ -792,7 +790,7 @@ func (e *Entry) Undo() { | |||
| } | |||
| pos := modify.Position | |||
| if modify.Delete { | |||
| pos += len(modify.Text) | |||
| pos += len([]rune(modify.Text)) | |||
There was a problem hiding this comment.
Here and anywhere else you want the length of a string in runes, it should be more efficient to use utf8.RuneCountInString(modify.Text) which avoids an allocation
|
Please base pull requests off the |
|
Thank you for your feedback. I implemented your proposals. Also I fixed text replacement that had similar issue. |
| @@ -2308,12 +2305,14 @@ func (i *entryModifyAction) Redo(s string) string { | |||
|
|
|||
| // Inserts Text | |||
| func (i *entryModifyAction) add(s string) string { | |||
There was a problem hiding this comment.
We have casting to strings that result in allocations here and in sub now. Would refactoring these to receive and return []rune result in fewer castings still?
There was a problem hiding this comment.
It looks like that opens a can of worms. as the undo/redo stack passes it up and down, with the source being Entry.Text which is string. I agree it would be a good improvement but probably could be another PR.
There was a problem hiding this comment.
Looks good to me, I think we need @dweymouth OK as well so I dropped a comment in.
Description:
The position in entry is the index in a rune slice. But the implementation of the undo/redo use it as an index in a string. This works for runes that are 1 byte long. But It was broken for longer runes.
I fixed the implementation of undo/redo to index a rune slices instead of the string directly
Fixes #5001
Checklist: