Add ctrl-_ undo support #13
Conversation
|
This one probably needs some cleanup |
|
Rebased on master and reworked it a bit. @slingamn what do you think about the approach? i feel like it sprinkle small undo calls a bit to much hmm. Any idea how to make it nicer? Also this implementation does not behave exactly as gnu readline undo etc but maybe it's close enough? |
|
Thanks for fixing the conflicts on this! I will definitely look at this but I may not have time until April 16th or so. |
|
👍 no hurry! |
|
Rebased on master |
runebuf.go
Outdated
| @@ -442,6 +445,9 @@ func (r *runeBuffer) Refresh(f func()) { | |||
| } | |||
|
|
|||
| func (r *runeBuffer) refresh(f func()) { | |||
| prevIdx := r.idx | |||
| prevBuf := append([]rune{}, r.buf...) | |||
|
|
|||
There was a problem hiding this comment.
Maybe this should be after interactive check? seems to work fine, moving it
|
Sorry I keep introducing conflicts in this :-) I'm not sure about how important this feature is to end users? Is it something that's important in the (I was also considering removing Vim mode, but there seems to be a fair amount of user interest in it, e.g. upstream's #36 and #120). At a code level this patch seems pretty good except that it increases implementation complexity, and also the size of the undo buffer is unbounded. (I'm thinking about writing a generic expandable ring buffer with a size limit that could replace uses of |
|
No worries. Don't know how many other use undo, the reason i added it is i'm used to it from other readline implementations. It would of course be nice to not have to maintain my own fork with some addition (only undo i think atm?) but basing it on ergochat/readline would still be a big improvement! |
TODO: better pos only changes TODO: config
|
If it's important to fq, then I'll get it merged in this cycle :-) |
That would be great of course 👍 what should be improved before merge, ring buffer? less complex somehow, better way to reset/init undo? BTW here is draft PR to use ergochat/readline with fq wader/fq#646 |
|
|
||
| if r.OnChange != nil { | ||
| if !runes.Equal(r.buf, prevBuf) { | ||
| if prevIdx != r.lastChangeIdx+1 { |
There was a problem hiding this comment.
Is the idea of this check to stop undo() from writing its own undo entry?
There was a problem hiding this comment.
If i remember correctly it's to skip triggering change when just typing "normally" and not moving around so that we don't add undo entries for each character change
There was a problem hiding this comment.
maybe it would be cleaner to move that kind of logic into opUndo by passing some more arguments to OnChange?
There was a problem hiding this comment.
but i'm not very happy with the this whole undo implementation, feels a bit adhoc hacky, so if you have some idea how to implement it in a nicer way please do!
|
@slingamn hey, just curious, any progress on rewrite things to resolve this in a nicer way? |
|
Sorry, work keeps getting in the way, but I'll put it on the list for this weekend. |
|
@slingamn no stress! Did a fq release and tags my forked deps and got thinking of this :) |
|
I promise I haven't forgotten about this :-) |
|
@slingamn no worries :) |
|
Done in #46 |
TODO: better pos only changes
TODO: config