Undo is also confused with narrow-to-region

Undo is also confused with narrow-to-region

[Oliver Kiddle]

Within narrow-to-region, undo will put back the full BUFFER that is then
duplicated from PREDISPLAY/POSTDISPLAY. After narrow-to-region, the
opposite problem occurs with the part that was not part of the BUFFER
being lost.

This isn't that easy to solve. recursive-edit could save and restore the
undo structures but for some uses of recursive-edit, such as that shown
for it in the manual, that might not be the right thing anyway.

Oliver

Re: Undo is also confused with narrow-to-region

[Mikael Magnusson]

On Fri, Jul 3, 2015 at 5:26 PM, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Within narrow-to-region, undo will put back the full BUFFER that is then
> duplicated from PREDISPLAY/POSTDISPLAY. After narrow-to-region, the
> opposite problem occurs with the part that was not part of the BUFFER
> being lost.
>
> This isn't that easy to solve. recursive-edit could save and restore the
> undo structures but for some uses of recursive-edit, such as that shown
> for it in the manual, that might not be the right thing anyway.

Would it be possible to implement some kind of 'zle push-undo-stack'
and 'zle pop-undo-stack'? If you push it you would get a whole new
instance of undo, and popping it then throws all those entries away
again. I can't really think of an instance outside recursive-edit
where it would be useful, though.

-- 
Mikael Magnusson

Re: Undo is also confused with narrow-to-region

[Peter Stephenson]

On Fri, 3 Jul 2015 20:12:13 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> On Fri, Jul 3, 2015 at 5:26 PM, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> > Within narrow-to-region, undo will put back the full BUFFER that is then
> > duplicated from PREDISPLAY/POSTDISPLAY. After narrow-to-region, the
> > opposite problem occurs with the part that was not part of the BUFFER
> > being lost.
> >
> > This isn't that easy to solve. recursive-edit could save and restore the
> > undo structures but for some uses of recursive-edit, such as that shown
> > for it in the manual, that might not be the right thing anyway.
> 
> Would it be possible to implement some kind of 'zle push-undo-stack'
> and 'zle pop-undo-stack'? If you push it you would get a whole new
> instance of undo, and popping it then throws all those entries away
> again. I can't really think of an instance outside recursive-edit
> where it would be useful, though.

You can already basically do this: see read-from-minibuffer.

integer changeno=$UNDO_CHANGE_NO

remembers the current undo (the undo numbers are unique up to integer
wrap, which isn't supposed to happen unless you're editing infinite
numbers of editions of Shakespeare using monkeys).

zle undo $changeno

then winds the undos back to that point.

pws

Re: Undo is also confused with narrow-to-region

[Oliver Kiddle]

Peter wrote:
> > Would it be possible to implement some kind of 'zle push-undo-stack'
> > and 'zle pop-undo-stack'? If you push it you would get a whole new
> > instance of undo, and popping it then throws all those entries away
> > again. I can't really think of an instance outside recursive-edit
> > where it would be useful, though.
> 
> You can already basically do this: see read-from-minibuffer.
> 
> integer changeno=$UNDO_CHANGE_NO
  ...
> zle undo $changeno

That prevents redoing minibuffer changes from the main editor but
doesn't stop you undoing main editor changes within the minibuffer. It
might be sufficient to mark the current undo state as a point beyond
which an undo will not go unless passed a change number as a parameter.

Or should we do that directly from recursive-edit?

Oliver

Re: Undo is also confused with narrow-to-region

[Peter Stephenson]

On Mon, 06 Jul 2015 13:46:25 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> That prevents redoing minibuffer changes from the main editor but
> doesn't stop you undoing main editor changes within the minibuffer. It
> might be sufficient to mark the current undo state as a point beyond
> which an undo will not go unless passed a change number as a parameter.

The following is easy.

The example I've given shows an explicit save and restore.  It should
also be possible to use the double function scope trick, though I didn't
actually try.

I haven't imposed any arbitrary limits on the value in UNDO_LIMIT_NO as,
on thinking about it, it seemed to create hostages to fortune for no
obvious gain.

pws

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index d3f0670..da8ee47 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -960,6 +960,26 @@ A number representing the state of the undo history.  The only use
 of this is passing as an argument to the tt(undo) widget in order to
 undo back to the recorded point.  Read-only.
 )
+vindex(UNDO_LIMIT_NO)
+item(tt(UNDO_LIMIT_NO) (integer))(
+A number corresponding to an existing change in the undo history;
+compare tt(UNDO_CHANGE_NO).  If this is set to a value greater
+than zero, the tt(undo) command will not allow the line to
+be undone beyond the given change number.  It is still possible
+to use `tt(zle undo) var(change)' in a widget to undo beyond
+that point; in that case, it will not be possible to undo at
+all until tt(UNDO_LIMIT_NO) is reduced.  Set to 0 to disable the limit.
+
+A typical use of this variable in a widget function is as follows:
+
+example(integer save_limit=$UNDO_LIMIT_NO
+UNDO_LIMIT_NO=$UNDO_CHANGE_NO
+{
+  # Perform some form of recursive edit.
+} always {
+  UNDO_LIMIT_NO=save_limit
+})
+)
 vindex(WIDGET)
 item(tt(WIDGET) (scalar))(
 The name of the widget currently being executed; read-only.
@@ -2333,7 +2353,8 @@ item(tt(undo) (tt(^_ ^Xu ^X^U)) (tt(u)) (unbound))(
 Incrementally undo the last text modification.  When called from a
 user-defined widget, takes an optional argument indicating a previous state
 of the undo history as returned by the tt(UNDO_CHANGE_NO) variable;
-modifications are undone until that state is reached.
+modifications are undone until that state is reached, subject to
+any limit imposed by the tt(UNDO_LIMIT_NO) variable.
 
 Note that when invoked from vi command mode, the full prior change made in
 insert mode is reverted, the changes having been merged when command mode was
diff --git a/Src/Zle/zle_params.c b/Src/Zle/zle_params.c
index ce4b072..b84e720 100644
--- a/Src/Zle/zle_params.c
+++ b/Src/Zle/zle_params.c
@@ -95,6 +95,8 @@ static const struct gsu_integer region_active_gsu =
 { get_region_active, set_region_active, zleunsetfn };
 static const struct gsu_integer undo_change_no_gsu =
 { get_undo_current_change, NULL, zleunsetfn };
+static const struct gsu_integer undo_limit_no_gsu =
+{ get_undo_limit_change, set_undo_limit_change, zleunsetfn };
 
 static const struct gsu_array killring_gsu =
 { get_killring, set_killring, unset_killring };
@@ -137,6 +139,7 @@ static struct zleparam {
     { "region_highlight", PM_ARRAY, GSU(region_highlight_gsu), NULL },
     { "UNDO_CHANGE_NO", PM_INTEGER | PM_READONLY, GSU(undo_change_no_gsu),
       NULL },
+    { "UNDO_LIMIT_NO", PM_INTEGER, GSU(undo_limit_no_gsu), NULL },
     { "WIDGET", PM_SCALAR | PM_READONLY, GSU(widget_gsu), NULL },
     { "WIDGETFUNC", PM_SCALAR | PM_READONLY, GSU(widgetfunc_gsu), NULL },
     { "WIDGETSTYLE", PM_SCALAR | PM_READONLY, GSU(widgetstyle_gsu), NULL },
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 06e4581..198c0ba 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1405,6 +1405,10 @@ static struct change *nextchanges, *endnextchanges;
 /**/
 zlong undo_changeno;
 
+/* If positive, don't undo beyond this point */
+
+zlong undo_limitno;
+
 /* If non-zero, the last increment to undo_changeno was for the variable */
 
 static int undo_set_by_variable;
@@ -1418,7 +1422,7 @@ initundo(void)
     curchange->prev = curchange->next = NULL;
     curchange->del = curchange->ins = NULL;
     curchange->dell = curchange->insl = 0;
-    curchange->changeno = undo_changeno = 0;
+    curchange->changeno = undo_changeno = undo_limitno = 0;
     undo_set_by_variable = 0;
     lastline = zalloc((lastlinesz = linesz) * ZLE_CHAR_SIZE);
     ZS_memcpy(lastline, zleline, (lastll = zlell));
@@ -1582,6 +1586,8 @@ undo(char **args)
 	    return 1;
 	if (prev->changeno < last_change)
 	    break;
+	if (prev->changeno < undo_limitno && !*args)
+	    break;
 	if (unapplychange(prev))
 	    curchange = prev;
 	else
@@ -1744,7 +1750,21 @@ get_undo_current_change(UNUSED(Param pm))
      * Increment the number in case a change is in progress;
      * we don't want to back off what's already been done when
      * we return to this change number.  This eliminates any
-     * problem about the point where a change is numbered.
+     * problem about the point where a change is numbered
      */
     return ++undo_changeno;
 }
+
+/**/
+zlong
+get_undo_limit_change(UNUSED(Param pm))
+{
+    return undo_limitno;
+}
+
+/**/
+void
+set_undo_limit_change(UNUSED(Param pm), zlong value)
+{
+    undo_limitno = value;
+}

Re: Undo is also confused with narrow-to-region

[Oliver Kiddle]

Peter wrote:
> 
> The example I've given shows an explicit save and restore.  It should
> also be possible to use the double function scope trick, though I didn't
> actually try.
> 
> I haven't imposed any arbitrary limits on the value in UNDO_LIMIT_NO as,
> on thinking about it, it seemed to create hostages to fortune for no
> obvious gain.

Thanks. Trying to apply this in read-from-minibuffer, I found that it
was necessary to add a split-undo in after the BUFFER etc were setup,
otherwise it was possible to do one initial undo. Also, I wonder if the
if condition should do a return 1 rather than break so that it beeps -
consistently with an undo that has reached the very first change. Or is
the setlastline() call needed?

Oliver
 
diff --git a/Functions/Zle/read-from-minibuffer b/Functions/Zle/read-from-minibuffer
index 8fec110..2b96a36 100644
--- a/Functions/Zle/read-from-minibuffer
+++ b/Functions/Zle/read-from-minibuffer
@@ -20,7 +20,7 @@ done
 (( OPTIND > 1 )) && shift $(( OPTIND - 1 ))
 
 local readprompt="$1" lbuf_init="$2" rbuf_init="$3"
-integer changeno=$UNDO_CHANGE_NO
+integer savelim=$UNDO_LIMIT_NO changeno=$UNDO_CHANGE_NO
 
 {
 # Use anonymous function to make sure special values get restored,
@@ -43,6 +43,8 @@ integer changeno=$UNDO_CHANGE_NO
   else
     local NUMERIC
     unset NUMERIC
+    zle split-undo
+    UNDO_LIMIT_NO=$UNDO_CHANGE_NO
     zle recursive-edit -K main
     stat=$?
     (( stat )) || REPLY=$BUFFER
@@ -52,6 +54,7 @@ integer changeno=$UNDO_CHANGE_NO
   # This removes the edits relating to the read from the undo history.
   # These aren't useful once we get back to the main editing buffer.
   zle undo $changeno
+  UNDO_LIMIT_NO=save_limit
 }
 
 return $stat
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 198c0ba..8b55403 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1587,7 +1587,7 @@ undo(char **args)
 	if (prev->changeno < last_change)
 	    break;
 	if (prev->changeno < undo_limitno && !*args)
-	    break;
+	    return 1;
 	if (unapplychange(prev))
 	    curchange = prev;
 	else

Re: Undo is also confused with narrow-to-region

[Peter Stephenson]

On Wed, 08 Jul 2015 16:57:33 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Peter wrote:
> > 
> > The example I've given shows an explicit save and restore.  It should
> > also be possible to use the double function scope trick, though I didn't
> > actually try.
> > 
> > I haven't imposed any arbitrary limits on the value in UNDO_LIMIT_NO as,
> > on thinking about it, it seemed to create hostages to fortune for no
> > obvious gain.
> 
> Thanks. Trying to apply this in read-from-minibuffer, I found that it
> was necessary to add a split-undo in after the BUFFER etc were setup,
> otherwise it was possible to do one initial undo. Also, I wonder if the
> if condition should do a return 1 rather than break so that it beeps -
> consistently with an undo that has reached the very first change. Or is
> the setlastline() call needed?

Finally committed.  I don't see that you'd need setlastline() if you
tried a single undo interactively and it failed, so it ought to be OK to
return 1.

pws

Re: Undo is also confused with narrow-to-region

[Peter Stephenson]

On Wed, 8 Jul 2015 16:57:33 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> > UNDO_LIMIT_NO
> 
> Thanks. Trying to apply this in read-from-minibuffer, I found that it
> was necessary to add a split-undo in after the BUFFER etc were setup,
> otherwise it was possible to do one initial undo.

Coming back to this after the last commit... perhaps recursive-edit
should start a new undo anyway?  You'd have thought it was somewhat
illogical to turn up in the middle of an edit.  (You might have thought
it was logical to force an undo limit at that point, too.  Could be
options.)  However, I suppose you now have full control, so it doesn't
matter so much.

Also, there's an inconsistency with the variable name here...

> -integer changeno=$UNDO_CHANGE_NO
> +integer savelim=$UNDO_LIMIT_NO changeno=$UNDO_CHANGE_NO
>...
> +  UNDO_LIMIT_NO=save_limit

pws