This widget implementation feels a bit clunky (edit-quoted-word)

This widget implementation feels a bit clunky (edit-quoted-word)

[Mikael Magnusson]

autoload -U narrow-to-region
function _narrow_to_region_marked()
{ #I had this function since before
  local right
  local left
  local OLDMARK=MARK
  local wasregion=1
  if ((REGION_ACTIVE == 0)); then
    MARK=CURSOR
    wasregion=0
  fi
  REGION_ACTIVE=0
  if ((MARK < CURSOR)); then
    left="$LBUFFER[0,$((MARK-CURSOR-1))]"
    right="$RBUFFER"
  else
    left="$LBUFFER"
    right="$BUFFER[$((MARK+1)),-1]"
  fi
  narrow-to-region -p "$left>>|" -P "|<<$right"
  _line_redraw #update highlight stuff doesn't happen here otherwise
  MARK=OLDMARK
  if ((wasregion)); then
    REGION_ACTIVE=1
  fi
}

function _edit_quoted_word()
{
  local -a reply
  local REPLY REPLY2 len
  _split_shell_arguments_intuitive
  local STARTPOS ENDPOS
  local OLDCURSOR=$CURSOR OLDMARK=$MARK wasregion=$REGION_ACTIVE
  STARTPOS=${#${(j::)reply[1,REPLY-1]}}
  reply[REPLY]=${(Q)reply[REPLY]}
  ENDPOS=${#${(j::)reply[1,REPLY]}}
  CURSOR=$STARTPOS
  MARK=$ENDPOS
  REGION_ACTIVE=1
  BUFFER=${(j::)reply}
  len=$#BUFFER
  _narrow_to_region_marked
  CURSOR=$STARTPOS
  MARK=$((ENDPOS+$#BUFFER-len))
  zle quote-region
  #the following line is also my quote-unquote-word widget
  modify-current-argument '${(q-)${(Q)ARG}}'
  CURSOR=$OLDCURSOR
  MARK=$OLDMARK
  REGION_ACTIVE=$wasregion
}
zle -N edit-quoted-word _edit_quoted_word
bindkey '^_q' edit-quoted-word

autoload -U modify-current-argument
autoload -U split-shell-arguments

function _split_shell_arguments_intuitive()
{ #Had this since before too
  split-shell-arguments
  #have to duplicate some of modify-current-argument to get the word
  #_under_ the cursor, not after.
  setopt localoptions noksharrays multibyte
  if (( REPLY > 1 )); then
    if (( REPLY & 1 )); then
      (( REPLY-- ))
    fi
  fi
}

I'm not exactly sure how often I would use this, but I had the thought
and set off on an adventure. This lets you be on a thing like
% echo I\''d (like) to "have\" some better example?'
turns into
% echo >>|I'd (like) to "have\" some better example?|<<
which is then somewhat easier to edit, then you maybe type this instead
% echo >>|I'd (like) to "have\" '${(q-)${(Q)ARG}}' some better example?|<<
after pressing enter, it turns back into
% echo I\''d (like) to "have\" ''${(q-)${(Q)ARG}}'' some better example?'

I feel like it's unnecessarily hard to get the resulting text from a
narrow-to-region session.
It would be nice if quote-region let you specify the type of quoting.
Urk?

-- 
Mikael Magnusson

Re: This widget implementation feels a bit clunky (edit-quoted-word)

[Peter Stephenson]

On Sat, 20 Jun 2015 11:16:20 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> I feel like it's unnecessarily hard to get the resulting text from a
> narrow-to-region session.

Would you want something like this?

diff --git a/Functions/Zle/narrow-to-region b/Functions/Zle/narrow-to-region
index 86fd7ac..d449540 100644
--- a/Functions/Zle/narrow-to-region
+++ b/Functions/Zle/narrow-to-region
@@ -11,6 +11,8 @@
 # Either or both may be empty.
 #   -n           Only replace the text before or after the region with
 #                the -p or -P options if the text was not empty.
+#   -l lbufvar   ) $lbufvar and $rbufvar will contain the value of $LBUFFER and
+#   -r rbufvar   ) $RBUFFER resulting from any recursive edit (i.e. not with -S or -R)
 #   -S statevar
 #   -R statevar
 # Save or restore the state in/from the parameter named statevar.  In
@@ -28,16 +30,20 @@ integer _ntr_start _ntr_end _ntr_swap _ntr_cursor=$CURSOR _ntr_mark=$MARK
 integer _ntr_stat
 
 local _ntr_opt _ntr_pretext _ntr_posttext _ntr_usepretext _ntr_useposttext
-local _ntr_nonempty _ntr_save _ntr_restore
+local _ntr_nonempty _ntr_save _ntr_restore _ntr_lbuffer _ntr_rbuffer
 
-while getopts "np:P:R:S:" _ntr_opt; do
+while getopts "l:np:P:r:R:S:" _ntr_opt; do
   case $_ntr_opt in
+    (l) _ntr_lbuffer=$OPTARG
+	;;
     (n) _ntr_nonempty=1
 	;;
     (p) _ntr_pretext=$OPTARG _ntr_usepretext=1
 	;;
     (P) _ntr_posttext=$OPTARG _ntr_useposttext=1
 	;;
+    (r) _ntr_rbuffer=$OPTARG
+	;;
     (R) _ntr_restore=$OPTARG
         ;;
     (S)	_ntr_save=$OPTARG
@@ -101,6 +107,9 @@ fi
 if [[ -z $_ntr_save && -z $_ntr_restore ]]; then
   zle recursive-edit
   _ntr_stat=$?
+
+  [[ -n $_ntr_lbuffer ]] && eval "${_ntr_lbuffer}=\${LBUFFER}"
+  [[ -n $_ntr_rbuffer ]] && eval "${_ntr_rbuffer}=\${RBUFFER}"
 fi
 
 if [[ -n $_ntr_restore || -z $_ntr_save ]]; then

Re: This widget implementation feels a bit clunky (edit-quoted-word)

[Bart Schaefer]

On Jun 20, 11:16am, Mikael Magnusson wrote:
}
} I feel like it's unnecessarily hard to get the resulting text from a
} narrow-to-region session.

Hmm.  If you used "narrow-to-region -S state ..." then I think the
resulting text would be $BUFFER between the exit from recursive-edit
and the call to "narrow-to-region -R state" ?

Except that I'm having a little trouble with -S / -R.  The value of
$MARK never seems to be correctly restored.  Is it supposed to be?

} It would be nice if quote-region let you specify the type of quoting.
} Urk?

The difficulty there is that it's a widget so there's no direct way to
pass the type of quote.  I suppose you could use $NUMERIC.

Re: This widget implementation feels a bit clunky (edit-quoted-word)

[Mikael Magnusson]

On Sat, Jun 20, 2015 at 7:06 PM, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
> On Sat, 20 Jun 2015 11:16:20 +0200
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> I feel like it's unnecessarily hard to get the resulting text from a
>> narrow-to-region session.
>
> Would you want something like this?

That does look very promising, yeah. :)
I suppose I should volunteer for writing the doc patch for this.

On Sat, Jun 20, 2015 at 7:21 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jun 20, 11:16am, Mikael Magnusson wrote:
> }
> } I feel like it's unnecessarily hard to get the resulting text from a
> } narrow-to-region session.
>
> Hmm.  If you used "narrow-to-region -S state ..." then I think the
> resulting text would be $BUFFER between the exit from recursive-edit
> and the call to "narrow-to-region -R state" ?
>
> Except that I'm having a little trouble with -S / -R.  The value of
> $MARK never seems to be correctly restored.  Is it supposed to be?

I'll admit I didn't actually look at the impl of the widget, only the
documentation.

> } It would be nice if quote-region let you specify the type of quoting.
> } Urk?
>
> The difficulty there is that it's a widget so there's no direct way to
> pass the type of quote.  I suppose you could use $NUMERIC.

I thought widgets can take an arbitrary amount of arguments, although
it is true that almost none do. The newly added bracketed-paste takes
an argument that names the parameter to store pasted text in for
example. If you mean that you can't do it directly via bindkey then
yeah, that's true, you'd still need a wrapper widget. For my use-case
I could put the argument right in with the zle call though. (But with
PWS' suggested change, I can just insert (q-)LBUFFER/RBUFFER directly,
I think).

-- 
Mikael Magnusson

Re: This widget implementation feels a bit clunky (edit-quoted-word)

[Peter Stephenson]

On Sun, 21 Jun 2015 09:09:29 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> On Sat, Jun 20, 2015 at 7:06 PM, Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
> > On Sat, 20 Jun 2015 11:16:20 +0200
> > Mikael Magnusson <mikachu@gmail.com> wrote:
> >> I feel like it's unnecessarily hard to get the resulting text from a
> >> narrow-to-region session.
> >
> > Would you want something like this?
> 
> That does look very promising, yeah. :)
> I suppose I should volunteer for writing the doc patch for this.

I've remembered to commit it on the master branch.

pws

PATCH: Document narrow-to-region -l and -r.

[Mikael Magnusson]

---
 Doc/Zsh/contrib.yo | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index 323bf0f..09ac5c8 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -2272,7 +2272,8 @@ tindex(narrow-to-region)
 tindex(narrow-to-region-invisible)
 redef(SPACES)(0)(tt(ifztexi(NOTRANS(@ @ @ @ @ @ @ @ @ @ @ @ @ @ @ @ @ ))ifnztexi(                 )))
 xitem(tt(narrow-to-region )[ tt(-p) var(pre) ] [ tt(-P) var(post) ])
-xitem(SPACES()[ tt(-S) var(statepm) | tt(-R) var(statepm) ] [ tt(-n) ] [ var(start) var(end) ])
+xitem(SPACES()[ tt(-S) var(statepm) | tt(-R) var(statepm) | [ tt(-l) var(lbufvar) ] [ tt(-r) var(rbufvar) ] ])
+xitem(SPACES()[ tt(-n) ] [ var(start) var(end) ])
 item(tt(narrow-to-region-invisible))(
 Narrow the editable portion of the buffer to the region between the cursor
 and the mark, which may be in either order.  The region may not be empty.
@@ -2308,9 +2309,15 @@ parameter, except that parameters beginning with the prefix tt(_ntr_) are
 reserved for use within tt(narrow-to-region).  Typically the parameter will
 be local to the calling function.
 
+The options tt(-l) var(lbufvar) and tt(-r) var(rbufvar) may be used to
+specify parameters where the widget will store the resulting text from
+the operation.  The parameter var(lbufvar) will contain var(LBUFFER)
+and var(rbufvar) will contain var(RBUFFER).  Neither of these two options
+may be used with tt(-S) or tt(-R).
+
 tt(narrow-to-region-invisible) is a simple widget which calls
 tt(narrow-to-region) with arguments which replace any text outside the
-region with `tt(...)'.
+region with `tt(...)'.  It does not take any arguments.
 
 The display is restored (and the widget returns) upon any zle command
 which would usually cause the line to be accepted or aborted.  Hence an
-- 
2.4.0

Re: This widget implementation feels a bit clunky (edit-quoted-word)

[Mikael Magnusson]

On Sun, Jun 21, 2015 at 7:17 PM, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
> On Sun, 21 Jun 2015 09:09:29 +0200
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> On Sat, Jun 20, 2015 at 7:06 PM, Peter Stephenson
>> <p.w.stephenson@ntlworld.com> wrote:
>> > On Sat, 20 Jun 2015 11:16:20 +0200
>> > Mikael Magnusson <mikachu@gmail.com> wrote:
>> >> I feel like it's unnecessarily hard to get the resulting text from a
>> >> narrow-to-region session.
>> >
>> > Would you want something like this?
>>
>> That does look very promising, yeah. :)
>> I suppose I should volunteer for writing the doc patch for this.
>
> I've remembered to commit it on the master branch.

This feels more reasonable now (some of the improvements might be
applied to the previous version too, so not an entirely fair
comparison), (the business with the :+ is because if you delete the
whole string, ${(q-)lbuf} when lbuf is empty results in '', and so
does rbuf, thus concatenated to '''' which with rc_quotes enabled
leaves a single ' that should not be there).

function _edit_quoted_word()
{
  local -a reply
  local REPLY REPLY2 len left right lbuf rbuf
  _split_shell_arguments_intuitive
  left=${(j::)reply[1,REPLY-1]}
  right=${(j::)reply[REPLY+1,-1]}
  (( CURSOR-=$#left ))
  BUFFER=${(Q)reply[REPLY]}
  REGION_ACTIVE=0
  narrow-to-region -l lbuf -r rbuf -p "$left>>|" -P "|<<$right" 0 $#BUFFER
  LBUFFER=$left${lbuf:+${(q-)lbuf}}
  RBUFFER=${rbuf:+${(q-)rbuf}}$right
}
zle -N _edit_quoted_word
bindkey '^_q' _edit_quoted_word

autoload -U split-shell-arguments

function _split_shell_arguments_intuitive()
{
  split-shell-arguments
  # borrowed logic from modify-current-argument to get word under cursor
  (( REPLY > 1 && REPLY & 1 )) && (( REPLY-- ))
}

-- 
Mikael Magnusson

PATCH: narrow-to-region (was Re: This widget implementation feels a bit clunky)

[Bart Schaefer]

On Jun 20, 10:21am, Bart Schaefer wrote:
} 
} [...]  If you used "narrow-to-region -S state ..." then I think the
} resulting text would be $BUFFER between the exit from recursive-edit
} and the call to "narrow-to-region -R state" ?
} 
} Except that I'm having a little trouble with -S / -R.  The value of
} $MARK never seems to be correctly restored.  Is it supposed to be?

What was happening:

In order to narrow the editable buffer to just the current region, the
lesser of MARK and CURSOR is calculated and assigned to _ntr_start.
Then the leading $_ntr_start characters are removed from BUFFER and
MARK and CURSOR are shifted to correspond, so one or the other of them
always ends up being set to 0.

Upon exit from recursive-edit (or calling "narrow-to-region -R state")
the values of LBUFFER and RBUFFER are reset, which automatically moves
CURSOR to the beginning of the new RBUFFER, but does not change MARK.
This guarantees that MARK will be wrong, because it will either be left
as zero, or at a now-irrelevant position in the middle of the buffer,
possibly even past the end of it.

So this patch:

- cleans up white space in the documentary comment

- checks that the -l and -r buffer variables also don't use _ntr_

- consistently uses "zle -M ... >&2" even though I don't know why

- does away with a bunch of unnecessary "eval"s

- declares the -l -r and -S variables with "typeset -g"

- forces MARK and CURSOR to point to opposite ends of the region after
  the function returns, which is really the point of all this


diff --git a/Functions/Zle/narrow-to-region b/Functions/Zle/narrow-to-region
index 8b88da4..293f89b 100644
--- a/Functions/Zle/narrow-to-region
+++ b/Functions/Zle/narrow-to-region
@@ -5,21 +5,26 @@
 # Optionally accepts exactly two arguments, which are used instead of
 # $CURSOR and $MARK as limits to the range.
 #
+# Upon exit, $MARK is always the start of the edited range and $CURSOR
+# the end of the range, even if they began in the opposite order.
+#
 # Other options:
-#   -p pretext   show `pretext' instead of the buffer text before the region.
-#   -P posttext  show  `posttext' instead of the buffer text after the region.
-# Either or both may be empty.
+#   -p pretext   show "pretext" instead of the buffer text before the region.
+#   -P posttext  show "posttext" instead of the buffer text after the region.
+#                Either or both may be empty.
 #   -n           Only replace the text before or after the region with
 #                the -p or -P options if the text was not empty.
-#   -l lbufvar   ) $lbufvar and $rbufvar will contain the value of $LBUFFER and
-#   -r rbufvar   ) $RBUFFER resulting from any recursive edit (i.e. not with -S or -R)
+#   -l lbufvar   $lbufvar is assigned the value of $LBUFFER and
+#   -r rbufvar   $rbufvar is assigned the value of $RBUFFER
+#                from any recursive edit (i.e. not with -S or -R).  Neither
+#                lbufvar nor rbufvar may begin with the prefix "_ntr_".
 #   -S statevar
 #   -R statevar
-# Save or restore the state in/from the parameter named statevar.  In
-# either case no recursive editing takes place; this will typically be
-# done within the calling function between calls with -S and -R.  The
-# statevar may not begin with the prefix _ntr_ which is reserved for
-# parameters within narrow-to-region.
+#      Save or restore the state in/from the parameter named statevar.  In
+#      either case no recursive editing takes place; this will typically be
+#      done within the calling function between calls with -S and -R.  The
+#      statevar may not begin with the prefix "_ntr_" which is reserved for
+#      parameters within narrow-to-region.
 
 emulate -L zsh
 setopt extendedglob
@@ -55,7 +60,8 @@ while getopts "l:np:P:r:R:S:" _ntr_opt; do
 done
 (( OPTIND > 1 )) && shift $(( OPTIND - 1 ))
 
-if [[ $_ntr_restore = _ntr_* || $_ntr_save = _ntr_* ]]; then
+if [[ $_ntr_restore = _ntr_* || $_ntr_save = _ntr_* ||
+      $_ntr_lbuf_return = _ntr_* || $ntr_rbuf_return = _ntr_* ]]; then
   zle -M "$0: _ntr_ prefix is reserved" >&2
   return 1
 fi
@@ -64,7 +70,7 @@ if [[ -n $_ntr_save || -z $_ntr_restore ]]; then
 
   if (( $# )); then
     if (( $# != 2 )); then
-      zle -M "$0: supply zero or two arguments"
+      zle -M "$0: supply zero or two arguments" >&2
       return 1
     fi
     _ntr_start=$1
@@ -94,39 +100,44 @@ if [[ -n $_ntr_save || -z $_ntr_restore ]]; then
   fi
   PREDISPLAY="$_ntr_predisplay$_ntr_pretext"
   POSTDISPLAY="$_ntr_posttext$_ntr_postdisplay"
-  BUFFER=${BUFFER[_ntr_start+1,_ntr_end-1]}
-  CURSOR=$_ntr_cursor
-  MARK=$_ntr_mark
 
   if [[ -n $_ntr_save ]]; then
-    eval "$_ntr_save=(${(qq)_ntr_predisplay} ${(qq)_ntr_postdisplay}
-${(qq)_ntr_lbuffer} ${(qq)_ntr_rbuffer})" || return 1
+    builtin typeset -ga $_ntr_save
+    set -A $_ntr_save "${_ntr_predisplay}" "${_ntr_postdisplay}" \
+	"${_ntr_lbuffer}" "${_ntr_rbuffer}" || return 1
   fi
+
+  BUFFER=${BUFFER[_ntr_start+1,_ntr_end-1]}
+  CURSOR=$_ntr_cursor
+  MARK=$_ntr_mark
 fi
 
 if [[ -z $_ntr_save && -z $_ntr_restore ]]; then
   zle recursive-edit
   _ntr_stat=$?
 
-  [[ -n $_ntr_lbuf_return ]] && eval "${_ntr_lbuf_return}=\${LBUFFER}"
-  [[ -n $_ntr_rbuf_return ]] && eval "${_ntr_rbuf_return}=\${RBUFFER}"
+  [[ -n $_ntr_lbuf_return ]] &&
+    builtin typeset -g ${_ntr_lbuf_return}="${LBUFFER}"
+  [[ -n $_ntr_rbuf_return ]] &&
+    builtin typeset -g ${_ntr_rbuf_return}="${RBUFFER}"
 fi
 
 if [[ -n $_ntr_restore || -z $_ntr_save ]]; then
   if [[ -n $_ntr_restore ]]; then
-    if ! eval "_ntr_predisplay=\${${_ntr_restore}[1]}
-_ntr_postdisplay=\${${_ntr_restore}[2]}
-_ntr_lbuffer=\${${_ntr_restore}[3]}
-_ntr_rbuffer=\${${_ntr_restore}[4]}"; then
-      zle -M Failed.
+    if ! { _ntr_predisplay="${${(@P)_ntr_restore}[1]}"
+           _ntr_postdisplay="${${(@P)_ntr_restore}[2]}"
+           _ntr_lbuffer="${${(@P)_ntr_restore}[3]}"
+           _ntr_rbuffer="${${(@P)_ntr_restore}[4]}" }; then
+      zle -M Failed. >&2
       return 1
     fi
   fi
 
   PREDISPLAY=$_ntr_predisplay
   POSTDISPLAY=$_ntr_postdisplay
-  LBUFFER="$_ntr_lbuffer$LBUFFER"
-  RBUFFER="$RBUFFER$_ntr_rbuffer"
+  LBUFFER="$_ntr_lbuffer$BUFFER"
+  RBUFFER="$_ntr_rbuffer"
+  MARK=${#_ntr_lbuffer}
 fi
 
 return $_ntr_stat

Re: PATCH: narrow-to-region (was Re: This widget implementation feels a bit clunky)

[Oliver Kiddle]

>  emulate -L zsh
>  setopt extendedglob

These lines (not introduced by Bart's patch) have the effect of
resetting lots of options when inside the recursive-edit. That 
affects the behaviour of some things such as completion.

How much of emulate zsh is actually needed by narrow-to-region itself?
Can we get by with just setopt localoptions and a few extras? Or use an
anonymous function around everything but recursive-edit?

It'd also be good for the function to use the new undo limit features.

Oliver

Re: PATCH: narrow-to-region (was Re: This widget implementation feels a bit clunky)

[Bart Schaefer]

On Jul 19,  5:55am, Oliver Kiddle wrote:
}
} How much of emulate zsh is actually needed by narrow-to-region itself?

It uses a bunch of unquoted variable references without braces so it
clearly expects no-shwordsplit and no-ksharrays.  I don't see any need
for extendedglob, on another quick read-through.

} It'd also be good for the function to use the new undo limit features.

I was going to send a follow-up message about undo-ing and whether a call
to split-undo is needed before (and/or after) the recursive-edit.

Related, should -S / -R implicitly set undo points?

Re: PATCH: narrow-to-region (was Re: This widget implementation feels a bit clunky)

[Oliver Kiddle]

Bart wrote:
> } It'd also be good for the function to use the new undo limit features.
> 
> I was going to send a follow-up message about undo-ing and whether a call
> to split-undo is needed before (and/or after) the recursive-edit.

It is needed immediately after you've setup PREDISPLAY and emptied
BUFFER.

I would have been fairly sure of that answer but my own attempts to make
it work had failed so I went back to see again first. I now realise that
the problem I had is elsewhere. I've attached a very rudimentary patch to
narrow-to-region.

My main use of narrow-to-region is to retrieve history lines into the
middle of the current line. And it is just this situation that is
breaking it. When doing:
  zle undo $_ntr_changeno
it is falling foul of the first line of unapplychange which is:
  if(ch->hist != histline) {
and not undoing enough changes.

This is a consequence of the fix in 10328. If I comment out the two
lines of undo() which do else break; it appears to work fine and the
problem that 10328 targetted doesn't seem to come back. But it is hard
to know how that was failing at the time.

> Related, should -S / -R implicitly set undo points?

I've never used those two options and am not sure I quite understand
what they're for. At least not without digging in the archives. With the
patch below, it is fairly obviously broken for them because
_ntr_changeno is set in a different block from the one which does zle
undo to revert to it.

Oliver

diff --git a/Functions/Zle/narrow-to-region b/Functions/Zle/narrow-to-region
index 293f89b..ae4addd 100644
--- a/Functions/Zle/narrow-to-region
+++ b/Functions/Zle/narrow-to-region
@@ -29,8 +29,9 @@
 emulate -L zsh
 setopt extendedglob
 
-local _ntr_lbuf_return _ntr_rbuf_return
+local _ntr_new_lbuf _ntr_newrbuf _ntr_lbuf_return _ntr_rbuf_return
 local _ntr_predisplay=$PREDISPLAY _ntr_postdisplay=$POSTDISPLAY
+integer _ntr_savelim=$UNDO_LIMIT_NO _ntr_changeno
 integer _ntr_start _ntr_end _ntr_swap _ntr_cursor=$CURSOR _ntr_mark=$MARK
 integer _ntr_stat
 
@@ -98,6 +99,7 @@ if [[ -n $_ntr_save || -z $_ntr_restore ]]; then
   then
     _ntr_posttext=$_ntr_rbuffer
   fi
+  _ntr_changeno=$UNDO_CHANGE_NO
   PREDISPLAY="$_ntr_predisplay$_ntr_pretext"
   POSTDISPLAY="$_ntr_posttext$_ntr_postdisplay"
 
@@ -110,16 +112,25 @@ if [[ -n $_ntr_save || -z $_ntr_restore ]]; then
   BUFFER=${BUFFER[_ntr_start+1,_ntr_end-1]}
   CURSOR=$_ntr_cursor
   MARK=$_ntr_mark
+  zle split-undo
 fi
 
 if [[ -z $_ntr_save && -z $_ntr_restore ]]; then
-  zle recursive-edit
-  _ntr_stat=$?
-
-  [[ -n $_ntr_lbuf_return ]] &&
-    builtin typeset -g ${_ntr_lbuf_return}="${LBUFFER}"
-  [[ -n $_ntr_rbuf_return ]] &&
-    builtin typeset -g ${_ntr_rbuf_return}="${RBUFFER}"
+  {
+    UNDO_LIMIT_NO=$UNDO_CHANGE_NO
+    zle recursive-edit
+    _ntr_stat=$?
+    _ntr_newlbuf="$LBUFFER"
+    _ntr_newrbuf="$RBUFFER"
+
+    [[ -n $_ntr_lbuf_return ]] &&
+      builtin typeset -g ${_ntr_lbuf_return}="${LBUFFER}"
+    [[ -n $_ntr_rbuf_return ]] &&
+      builtin typeset -g ${_ntr_rbuf_return}="${RBUFFER}"
+  } always {
+    zle undo $_ntr_changeno
+    UNDO_LIMIT_NO=_ntr_savelim
+  }
 fi
 
 if [[ -n $_ntr_restore || -z $_ntr_save ]]; then
@@ -135,8 +146,8 @@ if [[ -n $_ntr_restore || -z $_ntr_save ]]; then
 
   PREDISPLAY=$_ntr_predisplay
   POSTDISPLAY=$_ntr_postdisplay
-  LBUFFER="$_ntr_lbuffer$BUFFER"
-  RBUFFER="$_ntr_rbuffer"
+  LBUFFER="$_ntr_lbuffer$_ntr_newlbuf"
+  RBUFFER="$_ntr_newrbuf$_ntr_rbuffer"
   MARK=${#_ntr_lbuffer}
 fi
 

Re: PATCH: narrow-to-region (was Re: This widget implementation feels a bit clunky)

[Bart Schaefer]

On Jul 20, 11:19am, Oliver Kiddle wrote:
} Subject: Re: PATCH: narrow-to-region (was Re: This widget implementation f
}
} Bart wrote:
} > } It'd also be good for the function to use the new undo limit features.
} > 
} > I was going to send a follow-up message about undo-ing and whether a call
} > to split-undo is needed before (and/or after) the recursive-edit.
} 
} It is needed immediately after you've setup PREDISPLAY and emptied
} BUFFER.

The sourceforge outage is getting a bit annoying with respect to passing
these patches back and forth.

} > Related, should -S / -R implicitly set undo points?
} 
} I've never used those two options and am not sure I quite understand
} what they're for. At least not without digging in the archives. With the
} patch below, it is fairly obviously broken for them because
} _ntr_changeno is set in a different block from the one which does zle
} undo to revert to it.

The -S and -R options are basically a hack to because there's no good way
to do a callback mechansim.  What you'd like is to replace recursive-edit
with a call back into the invoking scope, but you can't do that without
real closures or at least real namerefs, so "narrow-to-region -S" does
everything up to recursive-edit and then saves state, followed by -R
which restores state and picks up where -S left off.

If you introduce new variables _ntr_savelim and _ntr_changeno those are
going to have to be added to the state save/restore, or else they have
to be localized to the part where recursive-edit is called and NOT the
parts where state is saved/restored.

Also this --

}    PREDISPLAY=$_ntr_predisplay
}    POSTDISPLAY=$_ntr_postdisplay
} -  LBUFFER="$_ntr_lbuffer$BUFFER"
} -  RBUFFER="$_ntr_rbuffer"
} +  LBUFFER="$_ntr_lbuffer$_ntr_newlbuf"
} +  RBUFFER="$_ntr_newrbuf$_ntr_rbuffer"
}    MARK=${#_ntr_lbuffer}

-- breaks MARK and CURSOR again, you need

LBUFFER="$_ntr_lbuffer$_ntr_newlbuf$_ntr_newrbuf"
RBUFFER="$_ntr_rbuffer"

to get CURSOR in the right place.  Which means you may as well not save
the left and right buffers separately to begin with.

Undo and narrow-to-region (was Re: PATCH: narrow-to-region (was ...))

[Bart Schaefer]

On Jul 20, 11:19am, Oliver Kiddle wrote:
} Subject: Re: PATCH: narrow-to-region (was Re: This widget implementation f
}
} My main use of narrow-to-region is to retrieve history lines into the
} middle of the current line. And it is just this situation that is
} breaking it. When doing:
}   zle undo $_ntr_changeno
} it is falling foul of the first line of unapplychange which is:
}   if(ch->hist != histline) {
} and not undoing enough changes.
} 
} This is a consequence of the fix in 10328. If I comment out the two
} lines of undo() which do else break; it appears to work fine and the
} problem that 10328 targetted doesn't seem to come back. But it is hard
} to know how that was failing at the time.

So you mean that part of undo becomes

        if (unapplychange(prev))
            curchange = prev;
    } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));

??

What must be happening is that unapplychange() alters the history line,
returns zero, and then is called again with exactly the same struct,
which now succeeds on the adjusted history line.  Pre-10328, the call
to unapplychange() would fail but curchange was moved to ->prev either
way, skipping over the change that failed.

So the "else break;" looks like paranoia to prevent an infinite loop in
the event that unapplychange() fails every time.  Which could happen if
quietgethist() is a no-op?  Perhaps when a line has fallen out of the
history because of HISTSIZE?

Anyway modulo infinite-loop-prevention, it looks to me as if removing
that else-clause is the right thing to do, and similarly in redo() a
bit further along.  I'll let Oliver do the patch if we all agree.

Re: Undo and narrow-to-region (was Re: PATCH: narrow-to-region (was ...))

[Oliver Kiddle]

On 22 Jul, Bart wrote:
> So you mean that part of undo becomes
> 
>         if (unapplychange(prev))
>             curchange = prev;
>     } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
> 
> ??
> 
> What must be happening is that unapplychange() alters the history line,
> returns zero, and then is called again with exactly the same struct,

Yes. Changes to the history number are not collected up as change events
in nextchanges. This means that several history line changes are undone
in a single go.

It also has the, perhaps less desirable, effect that you can't use redo
to revert a change to the history line. Is that worth fixing?

> So the "else break;" looks like paranoia to prevent an infinite loop in
> the event that unapplychange() fails every time.  Which could happen if
> quietgethist() is a no-op?  Perhaps when a line has fallen out of the
> history because of HISTSIZE?

I tried to find a way to induce such an infinite loop and couldn't. Even
so, how about the following patch which checks histline had changed to
detect a failure to set the history line.

> Anyway modulo infinite-loop-prevention, it looks to me as if removing
> that else-clause is the right thing to do, and similarly in redo() a
> bit further along.  I'll let Oliver do the patch if we all agree.

Not sure it's necessary for redo because there's no option to redo more
than one change.

Oliver

diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 8b55403..d512c7a 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1590,8 +1590,8 @@ undo(char **args)
 	    return 1;
 	if (unapplychange(prev))
 	    curchange = prev;
-	else
-	    break;
+	else if (histline != prev->hist)
+	    break; /* avoid infinite loop if history line unchanged */
     } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
     setlastline();
     return 0;

PATCH: Re: Undo and narrow-to-region (was ...)

[Oliver Kiddle]

Here is the narrow-to-region with support for undo limits along with
a related change to the undo mechanism.

It'd be good if this gets some wider testing because I only use
narrow-to-region in limited ways. This is showing up issues with
$UNDO_CHANGE_NO and zle undo to undo multiple changes.

The change to zle_utils.c here is for $UNDO_CHANGE_NO. 
If you consider this function:
  undostuff() {
    LBUFFER+='a'
    LBUFFER+='b'
    zle split-undo
    LBUFFER+='c'
    _undo_here=$UNDO_CHANGE_NO
    LBUFFER+='d'
  }

A subsequent zle undo $_undo_here should take you back to 'abc' while a
default undo should skip straight to 'ab' without stopping at 'abc'.

The following adds a call to mkundoent() when $UNDO_CHANGE_NO is
referenced so you get a clear change number marking the current
state. I think the result is simpler than the current tricks with
undo_set_by_variable.

This is still not perfect with history changes. I'm not really happy
with the way history line changes are managed in the undo system in
general. The old fix in 10328 seems rather ugly. It might be better to
create full undo entries for history changes though that would need
hacks to ensure that multiple history changes are undone in a single
step.

This patch also replaces 35935.

Oliver

diff --git a/Functions/Zle/narrow-to-region b/Functions/Zle/narrow-to-region
index 293f89b..0ef28a8 100644
--- a/Functions/Zle/narrow-to-region
+++ b/Functions/Zle/narrow-to-region
@@ -26,11 +26,13 @@
 #      statevar may not begin with the prefix "_ntr_" which is reserved for
 #      parameters within narrow-to-region.
 
-emulate -L zsh
-setopt extendedglob
+# set the minimum of options to avoid changing behaviour away from
+# user preferences from within recursive-edit
+setopt localoptions noshwordsplit noksharrays
 
-local _ntr_lbuf_return _ntr_rbuf_return
+local _ntr_newbuf _ntr_lbuf_return _ntr_rbuf_return
 local _ntr_predisplay=$PREDISPLAY _ntr_postdisplay=$POSTDISPLAY
+integer _ntr_savelim=UNDO_LIMIT_NO _ntr_changeno
 integer _ntr_start _ntr_end _ntr_swap _ntr_cursor=$CURSOR _ntr_mark=$MARK
 integer _ntr_stat
 
@@ -61,7 +63,7 @@ done
 (( OPTIND > 1 )) && shift $(( OPTIND - 1 ))
 
 if [[ $_ntr_restore = _ntr_* || $_ntr_save = _ntr_* ||
-      $_ntr_lbuf_return = _ntr_* || $ntr_rbuf_return = _ntr_* ]]; then
+      $_ntr_lbuf_return = _ntr_* || $_ntr_rbuf_return = _ntr_* ]]; then
   zle -M "$0: _ntr_ prefix is reserved" >&2
   return 1
 fi
@@ -86,30 +88,34 @@ if [[ -n $_ntr_save || -z $_ntr_restore ]]; then
     _ntr_end=_ntr_swap
   fi
 
-  (( _ntr_end++, _ntr_cursor -= _ntr_start, _ntr_mark -= _ntr_start ))
+  (( _ntr_cursor -= _ntr_start, _ntr_mark -= _ntr_start ))
   
   _ntr_lbuffer=${BUFFER[1,_ntr_start]}
   if [[ -z $_ntr_usepretext || ( -n $_ntr_nonempty && -z $_ntr_lbuffer ) ]]
   then
     _ntr_pretext=$_ntr_lbuffer
   fi
-  _ntr_rbuffer=${BUFFER[_ntr_end,-1]}
+  _ntr_rbuffer=${BUFFER[_ntr_end+1,-1]}
   if [[ -z $_ntr_useposttext || ( -n $_ntr_nonempty && -z $_ntr_rbuffer ) ]]
   then
     _ntr_posttext=$_ntr_rbuffer
   fi
+  _ntr_changeno=$UNDO_CHANGE_NO
   PREDISPLAY="$_ntr_predisplay$_ntr_pretext"
   POSTDISPLAY="$_ntr_posttext$_ntr_postdisplay"
 
   if [[ -n $_ntr_save ]]; then
     builtin typeset -ga $_ntr_save
     set -A $_ntr_save "${_ntr_predisplay}" "${_ntr_postdisplay}" \
-	"${_ntr_lbuffer}" "${_ntr_rbuffer}" || return 1
+	"${_ntr_savelim}" "${_ntr_changeno}" \
+	"${_ntr_start}" "${_ntr_end}" || return 1
   fi
 
-  BUFFER=${BUFFER[_ntr_start+1,_ntr_end-1]}
+  BUFFER=${BUFFER[_ntr_start+1,_ntr_end]}
   CURSOR=$_ntr_cursor
   MARK=$_ntr_mark
+  zle split-undo
+  UNDO_LIMIT_NO=$UNDO_CHANGE_NO
 fi
 
 if [[ -z $_ntr_save && -z $_ntr_restore ]]; then
@@ -126,18 +132,22 @@ if [[ -n $_ntr_restore || -z $_ntr_save ]]; then
   if [[ -n $_ntr_restore ]]; then
     if ! { _ntr_predisplay="${${(@P)_ntr_restore}[1]}"
            _ntr_postdisplay="${${(@P)_ntr_restore}[2]}"
-           _ntr_lbuffer="${${(@P)_ntr_restore}[3]}"
-           _ntr_rbuffer="${${(@P)_ntr_restore}[4]}" }; then
+	   _ntr_savelim="${${(@P)_ntr_restore}[3]}"
+	   _ntr_changeno="${${(@P)_ntr_restore}[4]}"
+	   _ntr_start="${${(@P)_ntr_restore}[5]}"
+	   _ntr_end="${${(@P)_ntr_restore}[6]}" }; then
       zle -M Failed. >&2
       return 1
     fi
   fi
 
+  _ntr_newbuf="$BUFFER"
+  zle undo $_ntr_changeno
   PREDISPLAY=$_ntr_predisplay
   POSTDISPLAY=$_ntr_postdisplay
-  LBUFFER="$_ntr_lbuffer$BUFFER"
-  RBUFFER="$_ntr_rbuffer"
-  MARK=${#_ntr_lbuffer}
+  BUFFER[_ntr_start+1,_ntr_end]="$_ntr_newbuf"
+  (( MARK = _ntr_start, CURSOR = _ntr_start + ${#_ntr_newbuf} ))
+  UNDO_LIMIT_NO=_ntr_savelim
 fi
 
 return $_ntr_stat
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 8b55403..d1d3206 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1409,10 +1409,6 @@ zlong undo_changeno;
 
 zlong undo_limitno;
 
-/* If non-zero, the last increment to undo_changeno was for the variable */
-
-static int undo_set_by_variable;
-
 /**/
 void
 initundo(void)
@@ -1423,7 +1419,6 @@ initundo(void)
     curchange->del = curchange->ins = NULL;
     curchange->dell = curchange->insl = 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));
     lastcs = zlecs;
@@ -1549,7 +1544,6 @@ mkundoent(void)
 	ch->prev = NULL;
     }
     ch->changeno = ++undo_changeno;
-    undo_set_by_variable = 0;
     endnextchanges = ch;
 }
 
@@ -1584,14 +1578,13 @@ undo(char **args)
 	struct change *prev = curchange->prev;
 	if(!prev)
 	    return 1;
-	if (prev->changeno < last_change)
+	if (prev->changeno <= last_change)
 	    break;
-	if (prev->changeno < undo_limitno && !*args)
+	if (prev->changeno <= undo_limitno && !*args)
 	    return 1;
-	if (unapplychange(prev))
-	    curchange = prev;
-	else
-	    break;
+	if (!unapplychange(prev) && last_change >= 0)
+	    unapplychange(prev);
+	curchange = prev;
     } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
     setlastline();
     return 0;
@@ -1741,18 +1734,10 @@ zlecallhook(char *name, char *arg)
 zlong
 get_undo_current_change(UNUSED(Param pm))
 {
-    if (undo_set_by_variable) {
-	/* We were the last to increment this, doesn't need another one. */
-	return undo_changeno;
-    }
-    undo_set_by_variable = 1;
-    /*
-     * 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
-     */
-    return ++undo_changeno;
+    /* add entry for any pending changes */
+    mkundoent();
+    setlastline();
+    return undo_changeno;
 }
 
 /**/

Re: PATCH: Re: Undo and narrow-to-region (was ...)

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Oliver,

this patch breaks the following widget I am using (via http://chneukirchen.org/blog/archive/2013/03/10-fresh-zsh-tricks-you-may-not-know.html):

autoload -Uz narrow-to-region
function _history-incremental-preserving-pattern-search-backward
{
  local state
  MARK=CURSOR  # magick, else multiple ^R don't work
  narrow-to-region -p "$LBUFFER${BUFFER:+>>}" -P "${BUFFER:+<<}$RBUFFER" -S state
  zle end-of-history
  zle history-incremental-pattern-search-backward
  narrow-to-region -R state
}
zle -N _history-incremental-preserving-pattern-search-backward
bindkey "^R" _history-incremental-preserving-pattern-search-backward
bindkey -M isearch "^R" history-incremental-pattern-search-backward
bindkey "^S" history-incremental-pattern-search-forward


After ^R, pressing Enter will add the same line to the end again.
This also happens when pressing a cursor key etc.

TEST CASE:
% echo foo
foo
% ^Rfoo^M
fooecho foo

Commit: 832130c57dedc532191512045096180657a049f3


Regards,
Daniel.

On 12.08.2015 19:00, Oliver Kiddle wrote:
> Here is the narrow-to-region with support for undo limits along with
> a related change to the undo mechanism.
> 
> It'd be good if this gets some wider testing because I only use
> narrow-to-region in limited ways. This is showing up issues with
> $UNDO_CHANGE_NO and zle undo to undo multiple changes.
> 
> The change to zle_utils.c here is for $UNDO_CHANGE_NO. 
> If you consider this function:
>   undostuff() {
>     LBUFFER+='a'
>     LBUFFER+='b'
>     zle split-undo
>     LBUFFER+='c'
>     _undo_here=$UNDO_CHANGE_NO
>     LBUFFER+='d'
>   }
> 
> A subsequent zle undo $_undo_here should take you back to 'abc' while a
> default undo should skip straight to 'ab' without stopping at 'abc'.
> 
> The following adds a call to mkundoent() when $UNDO_CHANGE_NO is
> referenced so you get a clear change number marking the current
> state. I think the result is simpler than the current tricks with
> undo_set_by_variable.
> 
> This is still not perfect with history changes. I'm not really happy
> with the way history line changes are managed in the undo system in
> general. The old fix in 10328 seems rather ugly. It might be better to
> create full undo entries for history changes though that would need
> hacks to ensure that multiple history changes are undone in a single
> step.
> 
> This patch also replaces 35935.
> 
> Oliver
> 
> diff --git a/Functions/Zle/narrow-to-region b/Functions/Zle/narrow-to-region
> index 293f89b..0ef28a8 100644
> --- a/Functions/Zle/narrow-to-region
> +++ b/Functions/Zle/narrow-to-region
> @@ -26,11 +26,13 @@
>  #      statevar may not begin with the prefix "_ntr_" which is reserved for
>  #      parameters within narrow-to-region.
>  
> -emulate -L zsh
> -setopt extendedglob
> +# set the minimum of options to avoid changing behaviour away from
> +# user preferences from within recursive-edit
> +setopt localoptions noshwordsplit noksharrays
>  
> -local _ntr_lbuf_return _ntr_rbuf_return
> +local _ntr_newbuf _ntr_lbuf_return _ntr_rbuf_return
>  local _ntr_predisplay=$PREDISPLAY _ntr_postdisplay=$POSTDISPLAY
> +integer _ntr_savelim=UNDO_LIMIT_NO _ntr_changeno
>  integer _ntr_start _ntr_end _ntr_swap _ntr_cursor=$CURSOR _ntr_mark=$MARK
>  integer _ntr_stat
>  
> @@ -61,7 +63,7 @@ done
>  (( OPTIND > 1 )) && shift $(( OPTIND - 1 ))
>  
>  if [[ $_ntr_restore = _ntr_* || $_ntr_save = _ntr_* ||
> -      $_ntr_lbuf_return = _ntr_* || $ntr_rbuf_return = _ntr_* ]]; then
> +      $_ntr_lbuf_return = _ntr_* || $_ntr_rbuf_return = _ntr_* ]]; then
>    zle -M "$0: _ntr_ prefix is reserved" >&2
>    return 1
>  fi
> @@ -86,30 +88,34 @@ if [[ -n $_ntr_save || -z $_ntr_restore ]]; then
>      _ntr_end=_ntr_swap
>    fi
>  
> -  (( _ntr_end++, _ntr_cursor -= _ntr_start, _ntr_mark -= _ntr_start ))
> +  (( _ntr_cursor -= _ntr_start, _ntr_mark -= _ntr_start ))
>    
>    _ntr_lbuffer=${BUFFER[1,_ntr_start]}
>    if [[ -z $_ntr_usepretext || ( -n $_ntr_nonempty && -z $_ntr_lbuffer ) ]]
>    then
>      _ntr_pretext=$_ntr_lbuffer
>    fi
> -  _ntr_rbuffer=${BUFFER[_ntr_end,-1]}
> +  _ntr_rbuffer=${BUFFER[_ntr_end+1,-1]}
>    if [[ -z $_ntr_useposttext || ( -n $_ntr_nonempty && -z $_ntr_rbuffer ) ]]
>    then
>      _ntr_posttext=$_ntr_rbuffer
>    fi
> +  _ntr_changeno=$UNDO_CHANGE_NO
>    PREDISPLAY="$_ntr_predisplay$_ntr_pretext"
>    POSTDISPLAY="$_ntr_posttext$_ntr_postdisplay"
>  
>    if [[ -n $_ntr_save ]]; then
>      builtin typeset -ga $_ntr_save
>      set -A $_ntr_save "${_ntr_predisplay}" "${_ntr_postdisplay}" \
> -	"${_ntr_lbuffer}" "${_ntr_rbuffer}" || return 1
> +	"${_ntr_savelim}" "${_ntr_changeno}" \
> +	"${_ntr_start}" "${_ntr_end}" || return 1
>    fi
>  
> -  BUFFER=${BUFFER[_ntr_start+1,_ntr_end-1]}
> +  BUFFER=${BUFFER[_ntr_start+1,_ntr_end]}
>    CURSOR=$_ntr_cursor
>    MARK=$_ntr_mark
> +  zle split-undo
> +  UNDO_LIMIT_NO=$UNDO_CHANGE_NO
>  fi
>  
>  if [[ -z $_ntr_save && -z $_ntr_restore ]]; then
> @@ -126,18 +132,22 @@ if [[ -n $_ntr_restore || -z $_ntr_save ]]; then
>    if [[ -n $_ntr_restore ]]; then
>      if ! { _ntr_predisplay="${${(@P)_ntr_restore}[1]}"
>             _ntr_postdisplay="${${(@P)_ntr_restore}[2]}"
> -           _ntr_lbuffer="${${(@P)_ntr_restore}[3]}"
> -           _ntr_rbuffer="${${(@P)_ntr_restore}[4]}" }; then
> +	   _ntr_savelim="${${(@P)_ntr_restore}[3]}"
> +	   _ntr_changeno="${${(@P)_ntr_restore}[4]}"
> +	   _ntr_start="${${(@P)_ntr_restore}[5]}"
> +	   _ntr_end="${${(@P)_ntr_restore}[6]}" }; then
>        zle -M Failed. >&2
>        return 1
>      fi
>    fi
>  
> +  _ntr_newbuf="$BUFFER"
> +  zle undo $_ntr_changeno
>    PREDISPLAY=$_ntr_predisplay
>    POSTDISPLAY=$_ntr_postdisplay
> -  LBUFFER="$_ntr_lbuffer$BUFFER"
> -  RBUFFER="$_ntr_rbuffer"
> -  MARK=${#_ntr_lbuffer}
> +  BUFFER[_ntr_start+1,_ntr_end]="$_ntr_newbuf"
> +  (( MARK = _ntr_start, CURSOR = _ntr_start + ${#_ntr_newbuf} ))
> +  UNDO_LIMIT_NO=_ntr_savelim
>  fi
>  
>  return $_ntr_stat
> diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
> index 8b55403..d1d3206 100644
> --- a/Src/Zle/zle_utils.c
> +++ b/Src/Zle/zle_utils.c
> @@ -1409,10 +1409,6 @@ zlong undo_changeno;
>  
>  zlong undo_limitno;
>  
> -/* If non-zero, the last increment to undo_changeno was for the variable */
> -
> -static int undo_set_by_variable;
> -
>  /**/
>  void
>  initundo(void)
> @@ -1423,7 +1419,6 @@ initundo(void)
>      curchange->del = curchange->ins = NULL;
>      curchange->dell = curchange->insl = 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));
>      lastcs = zlecs;
> @@ -1549,7 +1544,6 @@ mkundoent(void)
>  	ch->prev = NULL;
>      }
>      ch->changeno = ++undo_changeno;
> -    undo_set_by_variable = 0;
>      endnextchanges = ch;
>  }
>  
> @@ -1584,14 +1578,13 @@ undo(char **args)
>  	struct change *prev = curchange->prev;
>  	if(!prev)
>  	    return 1;
> -	if (prev->changeno < last_change)
> +	if (prev->changeno <= last_change)
>  	    break;
> -	if (prev->changeno < undo_limitno && !*args)
> +	if (prev->changeno <= undo_limitno && !*args)
>  	    return 1;
> -	if (unapplychange(prev))
> -	    curchange = prev;
> -	else
> -	    break;
> +	if (!unapplychange(prev) && last_change >= 0)
> +	    unapplychange(prev);
> +	curchange = prev;
>      } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
>      setlastline();
>      return 0;
> @@ -1741,18 +1734,10 @@ zlecallhook(char *name, char *arg)
>  zlong
>  get_undo_current_change(UNUSED(Param pm))
>  {
> -    if (undo_set_by_variable) {
> -	/* We were the last to increment this, doesn't need another one. */
> -	return undo_changeno;
> -    }
> -    undo_set_by_variable = 1;
> -    /*
> -     * 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
> -     */
> -    return ++undo_changeno;
> +    /* add entry for any pending changes */
> +    mkundoent();
> +    setlastline();
> +    return undo_changeno;
>  }
>  
>  /**/
> 
-----BEGIN PGP SIGNATURE-----

iEYEARECAAYFAlXSTt8ACgkQfAK/hT/mPgBQFwCfbI+trLonGkP6845YlVrNZPoF
XM4AoKynpdr5Yf7wTReGSog4HMTdjAOl
=bq0S
-----END PGP SIGNATURE-----

Re: PATCH: Re: Undo and narrow-to-region (was ...)

[Bart Schaefer]

On Aug 17, 11:15pm, Daniel Hahler wrote:
} Subject: Re: PATCH: Re: Undo and narrow-to-region (was ...)
}
} this patch breaks the following widget I am using (via
} http://chneukirchen.org/blog/archive/2013/03/10-fresh-zsh-tricks-you-may-not-know.html):

Oliver may not be able to get to this for a while.  Is it possible for
you to try reverting just the change to the narrow-to-region shell code
and see if that fixes things, or if both the shell and C code changes
need to be reverted?

There's been difficulty before with the save/restore state feature of
narrow-to-region.

Re: PATCH: Re: Undo and narrow-to-region (was ...)

[Bart Schaefer]

On Aug 17,  2:58pm, I wrote:
}
} Oliver may not be able to get to this for a while.  Is it possible for
} you to try reverting just the change to the narrow-to-region shell code
} and see if that fixes things, or if both the shell and C code changes
} need to be reverted?

I was able to reproduce the broken behavior with 832130c and then back
that out to determine that Daniel's example works correctly with the
previous version of narrow-to-region.

Oliver said:
> This is still not perfect with history changes.

In fact it's actually horribly broken with respect to history changes,
at least with -R / -S, as far as I can tell ...

In Daniel's example, we start with BUFFER='', record $UNDO_CHANGE_NO,
wander about in the history a bit, then do "zle undo $_ntr_changeno"
with the expectation that will return us to the previous state, that
is, an empty BUFFER.  But it doesn't.

It mostly works as long as BUFFER isn't empty when $UNDO_CHANGE_NO is
saved.  This goes back to Oliver's grumbling that undo decides what to
do based on whether the text on the line matches the text at the undo
point,  In this case there is no text at the undo point, so anything
will match and nothing gets undone.

The following brute-forces the history back to the same line where the
undo point was set before performing the undo.  This works for Daniel's
example but I have no idea what it will do for other cases.  It can't
make things any worse, in theory.


diff --git a/Functions/Zle/narrow-to-region b/Functions/Zle/narrow-to-region
index 0ef28a8..261d821 100644
--- a/Functions/Zle/narrow-to-region
+++ b/Functions/Zle/narrow-to-region
@@ -32,7 +32,7 @@ setopt localoptions noshwordsplit noksharrays
 
 local _ntr_newbuf _ntr_lbuf_return _ntr_rbuf_return
 local _ntr_predisplay=$PREDISPLAY _ntr_postdisplay=$POSTDISPLAY
-integer _ntr_savelim=UNDO_LIMIT_NO _ntr_changeno
+integer _ntr_savelim=UNDO_LIMIT_NO _ntr_changeno _ntr_histno=HISTNO
 integer _ntr_start _ntr_end _ntr_swap _ntr_cursor=$CURSOR _ntr_mark=$MARK
 integer _ntr_stat
 
@@ -108,7 +108,7 @@ if [[ -n $_ntr_save || -z $_ntr_restore ]]; then
     builtin typeset -ga $_ntr_save
     set -A $_ntr_save "${_ntr_predisplay}" "${_ntr_postdisplay}" \
 	"${_ntr_savelim}" "${_ntr_changeno}" \
-	"${_ntr_start}" "${_ntr_end}" || return 1
+	"${_ntr_start}" "${_ntr_end}" "${_ntr_histno}" || return 1
   fi
 
   BUFFER=${BUFFER[_ntr_start+1,_ntr_end]}
@@ -135,13 +135,15 @@ if [[ -n $_ntr_restore || -z $_ntr_save ]]; then
 	   _ntr_savelim="${${(@P)_ntr_restore}[3]}"
 	   _ntr_changeno="${${(@P)_ntr_restore}[4]}"
 	   _ntr_start="${${(@P)_ntr_restore}[5]}"
-	   _ntr_end="${${(@P)_ntr_restore}[6]}" }; then
+	   _ntr_end="${${(@P)_ntr_restore}[6]}"
+	   _ntr_histno="${${(@P)_ntr_restore}[7]}" }; then
       zle -M Failed. >&2
       return 1
     fi
   fi
 
   _ntr_newbuf="$BUFFER"
+  HISTNO=_ntr_histno
   zle undo $_ntr_changeno
   PREDISPLAY=$_ntr_predisplay
   POSTDISPLAY=$_ntr_postdisplay

-- 
Barton E. Schaefer

Re: PATCH: Re: Undo and narrow-to-region (was ...)

[Bart Schaefer]

On Aug 17, 11:15pm, Daniel Hahler wrote:
}
} function _history-incremental-preserving-pattern-search-backward
} {
}   local state
}   MARK=CURSOR  # magick, else multiple ^R don't work
}   narrow-to-region -p "$LBUFFER${BUFFER:+>>}" -P "${BUFFER:+<<}$RBUFFER" -S state
}   zle end-of-history
}   zle history-incremental-pattern-search-backward
}   narrow-to-region -R state
} }

Incidentally, the MARK=CURSOR "magick" doesn't seem to be needed any
more with the most recent revision of narrow-to-region.  However, if
you remove that, then the retrieved history will *replace* whatever
text starts out in the region between MARK and CURSOR.   (This is
consistent with what bracketed-paste does, if that matters.)

-- 
Barton E. Schaefer