zle: vi mode: wrong undo handling on fresh lines

zle: vi mode: wrong undo handling on fresh lines

[Hauke Petersen]

Insert operations should count as a single step in the undo history,
i.e. from command mode

    ifoo<ESC>u

should effectively be a no-op.

AFAICT, zsh handles this fine with the exception of fresh lines. For
example, typing

    foo<ESC>u

yields "fo" instead of the expected empty line. Expliduntantly setting

    function zle-line-init { zle vi-insert; }; zle -N zle-line-init

works around this misbehavior (and I ran with it for a while) but
leads to undesired behavior in places that I can't quite remember now.
As an aside, `zle -K viins' does not have the same effect as `zle
vi-insert' here, when I expected them to be functionally equivalent.

This is on 5.0.2 but has been the case for as long as I can remember.
I thought I had posted this to the ML before but apparently have not.

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Sep 22,  2:37pm, Hauke Petersen wrote:
}
} example, typing
} 
}     foo<ESC>u
} 
} yields "fo" instead of the expected empty line. Expliduntantly setting

Congratulations, google finds no occurrences of that outside of the zsh
archive for this thread.  You have officially coined a word.

}     function zle-line-init { zle vi-insert; }; zle -N zle-line-init
} 
} works around this misbehavior

Until the bug is fixed, try something like

  zle-line-init() { [[ -o vi ]] && { zle vi-cmd-mode; zle vi-insert } }

} As an aside, `zle -K viins' does not have the same effect as `zle
} vi-insert' here, when I expected them to be functionally equivalent.

Well, no.  "zle -K viins" means you're still in whatever "mode" you were
in before (emacs, vicmd, or viins), but have started using the keymap
normally used for viins mode.  Similarly "zle -K menuselect" would not
magically fling you into the completion menu.  "zle vi-insert" causes an
actual change of mode, just as "zle menu-select" begins completion.

Re: zle: vi mode: wrong undo handling on fresh lines

[Hauke Petersen]

On Sun, Sep 22, 2013 at 8:24 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Sep 22,  2:37pm, Hauke Petersen wrote:
> } yields "fo" instead of the expected empty line. Expliduntantly setting
>
> Congratulations, google finds no occurrences of that outside of the zsh
> archive for this thread.  You have officially coined a word.

Wish I'd spelled it right! (ATTN search engines: I also claim "explidundantly".)

> } `zle -K viins' does not have the same effect as `zle
> } vi-insert' here, when I expected them to be functionally equivalent.
>
> Well, no.  "zle -K viins" means you're still in whatever "mode" you were
> in before (emacs, vicmd, or viins), but have started using the keymap
> normally used for viins mode.  Similarly "zle -K menuselect" would not
> magically fling you into the completion menu.  "zle vi-insert" causes an
> actual change of mode, just as "zle menu-select" begins completion.

If there's an underlying difference, man zshzle(1) should probably say
the following differently:

       zle-line-init

              [...]

                     zle-line-init() { zle -K vicmd; }
                     zle -N zle-line-init

              (The  command inside the function sets the keymap directly; it
              is equivalent to zle vi-cmd-mode.)

> Until the bug is fixed, try something like
>
>   zle-line-init() { [[ -o vi ]] && { zle vi-cmd-mode; zle vi-insert } }

Curiously:

    % EDITOR=vi zsh -f
    % bindkey -lL main
    bindkey -A viins main
    % [[ -o vi ]] && echo yes
    % bindkey -v
    % [[ -o vi ]] && echo yes
    % set -o vi
    % [[ -o vi ]] && echo yes
    yes

Anyway I'll just wait for the eventual fix, it's just a minor
inconvenience. Thank you kindly.

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Sep 22, 10:27pm, Hauke Petersen wrote:
}
} > "zle -K viins" means you're still in whatever "mode" you were
} > in before (emacs, vicmd, or viins), but have started using the keymap
} > normally used for viins mode.
} > "zle vi-insert" causes an actual change of mode
} 
} If there's an underlying difference, man zshzle(1) should probably say
} the following differently:
} 
}                      zle-line-init() { zle -K vicmd; }

Hmm, it probably would in fact be better to use "zle vi-cmd-mode" in
that example, but the differences between emacs mode and vicmd mode
are less extreme than emacs and viins.

} > Until the bug is fixed, try something like
} >
} >   zle-line-init() { [[ -o vi ]] && { zle vi-cmd-mode; zle vi-insert } }
} 
} Curiously:
} 
}     % EDITOR=vi zsh -f
}     % bindkey -lL main
}     bindkey -A viins main
}     % [[ -o vi ]] && echo yes

Heh.  Hence "try something like" not "do exactly this" ... what you use
in place of [[ -o vi ]] depends on how you got into viins mode in the
first place.  A more universal test would be [[ $KEYMAP == viins ]],
and now that I think of it vi-cmd-mode may move the cursor back one
postion, so:

    zle-line-init() {
      [[ $KEYMAP == viins ]] && {
	zle vi-cmd-mode
	zle vi-add-next
      }
    }

Re: zle: vi mode: wrong undo handling on fresh lines

[Peter Stephenson]

On Sun, 22 Sep 2013 14:37:25 +0200
Hauke Petersen <hkptrsn@gmail.com> wrote:
> Insert operations should count as a single step in the undo history,
> i.e. from command mode
> 
>     ifoo<ESC>u
> 
> should effectively be a no-op.
> 
> AFAICT, zsh handles this fine with the exception of fresh lines.

Is it something like this, perhaps?

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 756ff11..5798e74 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1188,6 +1188,13 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     viinsbegin = 0;
     statusline = NULL;
     selectkeymap("main", 1);
+    /*
+     * If main is linked to the viins keymap, we need to register
+     * explicitly that we're now in vi insert mode as there's
+     * no user operation to indicate this.
+     */
+    if (openkeymap("main") == openkeymap("viins"))
+	viinsert(NULL);
     selectlocalmap(NULL);
     fixsuffix();
     if ((s = getlinknode(bufstack))) {

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

On 23 Sep, Peter Stephenson wrote:
> Hauke Petersen <hkptrsn@gmail.com> wrote:
> > Insert operations should count as a single step in the undo history,
> > i.e. from command mode
> > 
> >     ifoo<ESC>u
> > 
> > should effectively be a no-op.
> > 
> > AFAICT, zsh handles this fine with the exception of fresh lines.
> 
> Is it something like this, perhaps?

I find the effect of this change to undo to be very annoying. Typically
undo now wipes the whole buffer which isn't much use. In particular, the
old behaviour of undo after a completion is important to me. I
understand that it rightly should be marking the start of an insertion
at this point. However, even aside from that, the change doesn't seem to
be quite right: e.g. vi-repeat (dot) can't repeat the initial insertion.

My setup is something of a hybrid: vi-mode but with many emacs-mode
widgets bound in insert mode. In most cases where there are identically
named vi- widgets, the only difference is that the vi- ones honour the
viinsbegin position (so I use the emacs ones). Note that vim has relaxed
some of these so you can do things like backspace past the start of a
change. Rather usefully, a vim Ctrl-W (backward-kill-word), will stop at
the start position but go on if invoked a second time. I can't find a
way to implement that in a zsh user-defined widget mainly because the
return status of vi-backward-kill-word doesn't seem to be helping me.
  
It is no bad thing to group successive inserts into a single undo event
(most editors, emacs included do this). The question is how to provide
some control so something like a backward-delete-word or completion can
be treated as a separate undo event. vim solves this by letting you
break a change into two undo blocks with Ctrl-G u. This doesn't alter
the vi change start position, only the undo stack. Is a widget to do
that possible? Another possibility which comes to mind would be to add a
flag like ZLE_KEEPSUFFIX† to mark widgets that start a new undo block,
though I realise those flags aren't currently configurable.

†Incidentally, I bind escape to first do zle auto-suffix-retain so
vi-cmd-mode should perhaps have ZLE_KEEPSUFFIX set.

Oliver

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 25, 12:19am, Oliver Kiddle wrote:
}
} I find the effect of this change to undo to be very annoying. Typically
} undo now wipes the whole buffer which isn't much use. In particular, the
} old behaviour of undo after a completion is important to me. I
} understand that it rightly should be marking the start of an insertion
} at this point.

Yes, I think it's a bug that completion doesn't behave as if you've
left and then re-entered insert mode.  That's a bit different than what
you mentioned about the vim behavior.

} However, even aside from that, the change doesn't seem to
} be quite right: e.g. vi-repeat (dot) can't repeat the initial insertion.

Worse than that, vi-repeat forllowing the initial insertion repeats
the accept-line from the end of the previous command, thus attempting
to execute the (usually partial) current command line immediately.

Clearly more is needed to properly set up the vi-mode state at the start
of the buffer.

Re: zle: vi mode: wrong undo handling on fresh lines

[Peter Stephenson]

On Sat, 25 Jan 2014 11:15:30 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 25, 12:19am, Oliver Kiddle wrote:
> }
> } I find the effect of this change to undo to be very annoying. Typically
> } undo now wipes the whole buffer which isn't much use. In particular, the
> } old behaviour of undo after a completion is important to me. I
> } understand that it rightly should be marking the start of an insertion
> } at this point.
> 
> Yes, I think it's a bug that completion doesn't behave as if you've
> left and then re-entered insert mode.  That's a bit different than what
> you mentioned about the vim behavior.

So presumably startvichange() needs calling somewhere else?  It's not
clear where, since it's only used inside zle_vi.c, which has nothing to
do with completion.  Maybe there needs to be a hook from the outside
world.  There is some interaction with completion in the top-level
docomplete() call:

    /* For vi mode, reset the start-of-insertion pointer to the beginning *
     * of the word being completed, if it is currently later.  Vi itself  *
     * would never change the pointer in the middle of an insertion, but  *
     * then vi doesn't have completion.  More to the point, this is only  *
     * an emulation.                                                      */
    if (viinsbegin > ztrsub(zlemetaline + wb, zlemetaline))
	viinsbegin = ztrsub(zlemetaline + wb, zlemetaline);

but that doesn't look like it's relevant here.  The only other mention
down there that I can see is when expanding history, which is a
different matter.

> } However, even aside from that, the change doesn't seem to
> } be quite right: e.g. vi-repeat (dot) can't repeat the initial insertion.
> 
> Worse than that, vi-repeat forllowing the initial insertion repeats
> the accept-line from the end of the previous command, thus attempting
> to execute the (usually partial) current command line immediately.
> 
> Clearly more is needed to properly set up the vi-mode state at the start
> of the buffer.

That kind of suggests there's some stuff startvitext() or
startvichange() ought to be doing already but isn't, but we've got away
with it somehow.  inrepeat and vichgrepeat presumably have something to
do with it.

I'm not going to be much use smoothing out all the wrinkles, since I
never use vi mode.

pws

Re: zle: vi mode: wrong undo handling on fresh lines

[Peter Stephenson]

On Mon, 27 Jan 2014 12:43:01 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > } However, even aside from that, the change doesn't seem to
> > } be quite right: e.g. vi-repeat (dot) can't repeat the initial insertion.
> > 
> > Worse than that, vi-repeat forllowing the initial insertion repeats
> > the accept-line from the end of the previous command, thus attempting
> > to execute the (usually partial) current command line immediately.
> > 
> > Clearly more is needed to properly set up the vi-mode state at the start
> > of the buffer.
> 
> That kind of suggests there's some stuff startvitext() or
> startvichange() ought to be doing already but isn't, but we've got away
> with it somehow.  inrepeat and vichgrepeat presumably have something to
> do with it.

Or startvitext() and startvichange() may be buried too far in to want
any changing.

I think this is starting to get somewhere, but I suspect it needs
tweaking.  For example, should that synthesised 'i' really be an 'a'?
Is it OK to assume we're not in insert mode when vi-repeat is executed?
If it's not safe, I don't understand how it ever worked.

Some vi user will need to take over at this point.  All the action I'm
aware of is in the functions I've changed and if you've looked at the
code you understand it as well as I do.

I haven't thought further about the completion thing.  I've no idea
how that ever did anything useful.

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 040b7cb..a2b20df 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1204,7 +1204,7 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
      * no user operation to indicate this.
      */
     if (openkeymap("main") == openkeymap("viins"))
-	viinsert(NULL);
+	viinsert_init();
     selectlocalmap(NULL);
     fixsuffix();
     if ((s = getlinknode(bufstack))) {
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 173a49e..989998e 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -67,6 +67,13 @@ int viinsbegin;
 static struct modifier lastmod;
 static int inrepeat, vichgrepeat;
 
+/**
+ * im: >= 0: is an insertmode
+ *    -1: skip setting insert mode
+ *    -2: entering viins at start of editing from clean --- don't use
+ *        inrepeat or lastchar, synthesise an i to enter insert mode.
+ */
+
 /**/
 static void
 startvichange(int im)
@@ -75,7 +82,7 @@ startvichange(int im)
 	insmode = im;
 	vichgflag = 1;
     }
-    if (inrepeat) {
+    if (inrepeat && im != -2) {
 	zmod = lastmod;
 	inrepeat = vichgflag = 0;
 	vichgrepeat = 1;
@@ -84,7 +91,11 @@ startvichange(int im)
 	if (vichgbuf)
 	    free(vichgbuf);
 	vichgbuf = (char *)zalloc(vichgbufsz = 16);
-	vichgbuf[0] = lastchar;
+	if (im == -2) {
+	    vichgbuf[0] = 'i';
+	} else {
+	    vichgbuf[0] = lastchar;
+	}
 	vichgbufptr = 1;
 	vichgrepeat = 0;
     }
@@ -303,6 +314,18 @@ viinsert(UNUSED(char **args))
     return 0;
 }
 
+/*
+ * Go to vi insert mode when we first start the line editor.
+ * Iniialises some other stuff.
+ */
+
+/**/
+void
+viinsert_init(void)
+{
+    startvitext(-2);
+}
+
 /**/
 int
 viinsertbol(UNUSED(char **args))

pws

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 27, 12:43pm, Peter Stephenson wrote:
} Subject: Re: zle: vi mode: wrong undo handling on fresh lines
}
} On Sat, 25 Jan 2014 11:15:30 -0800
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > 
} > Yes, I think it's a bug that completion doesn't behave as if you've
} > left and then re-entered insert mode.
} 
} So presumably startvichange() needs calling somewhere else?

I'm not sure that's exactly it, since we're already in insert mode ...

} There is some interaction with completion in the top-level
} docomplete() call:
} 
}     /* For vi mode, reset the start-of-insertion pointer to the beginning *
}      * of the word being completed, if it is currently later.  Vi itself  *
}      * would never change the pointer in the middle of an insertion, but  *
}      * then vi doesn't have completion.  More to the point, this is only  *
}      * an emulation.                                                      */

That's kind of related, I think.  A single undo should not cross over
the insertion point, so if we've moved that, we should have started a
new undo event.

} Maybe there needs to be a hook from the outside world.  

Either that or (shudder) we need vi- wrapped versions of the completion
widgets.

} > Clearly more is needed to properly set up the vi-mode state at the start
} > of the buffer.
} 
} That kind of suggests there's some stuff startvitext() or
} startvichange() ought to be doing already but isn't, but we've got away
} with it somehow.

OH!  The problem seems to be that startvichange() always initializes
vichgbuf[0] = lastchar, and lastchar is still the trailing '\r' from
the previous command.  Is it safe to initialize lastchar = 0 when we
enter a new ZLE?

Re: zle: vi mode: wrong undo handling on fresh lines

[Peter Stephenson]

On Mon, 27 Jan 2014 16:11:24 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Mon, 27 Jan 2014 12:43:01 +0000
> Peter Stephenson <p.stephenson@samsung.com> wrote:
> > > } However, even aside from that, the change doesn't seem to
> > > } be quite right: e.g. vi-repeat (dot) can't repeat the initial insertion.
> > > 
> > > Worse than that, vi-repeat forllowing the initial insertion repeats
> > > the accept-line from the end of the previous command, thus attempting
> > > to execute the (usually partial) current command line immediately.
>
> I think this is starting to get somewhere, but I suspect it needs
> tweaking.  For example, should that synthesised 'i' really be an 'a'?
> Is it OK to assume we're not in insert mode when vi-repeat is executed?
> If it's not safe, I don't understand how it ever worked.

Well, I think I'm going to commit it with the "a" because (i) this
definitely seems better than what was there before in any case and (ii)
hitting "ESC" and "." after typing the start of the line, which is my
naive way of getting it to repeat, does repeat what I just inserted
because of vi's quirk of backing up a character when leaving insert
mode.  Obviously I'm not claiming this is the end of the matter.

> Some vi user will need to take over at this point.  All the action I'm
> aware of is in the functions I've changed and if you've looked at the
> code you understand it as well as I do.

This remains the case.  I will not be able to guess what vi mode's many
oddities are supposed to do.

I'm not hearing much in the way of knowledgeable direction for the
completion issue.  It probably needs some hook trying out to see
if it works.

pws

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 28,  2:58pm, Peter Stephenson wrote:
}
} > I think this is starting to get somewhere, but I suspect it needs
} > tweaking.  For example, should that synthesised 'i' really be an 'a'?
} 
} Well, I think I'm going to commit it with the "a" because (i) this
} definitely seems better than what was there before in any case and (ii)
} hitting "ESC" and "." after typing the start of the line, which is my
} naive way of getting it to repeat, does repeat what I just inserted
} because of vi's quirk of backing up a character when leaving insert
} mode.  Obviously I'm not claiming this is the end of the matter.

Sorry, I missed this when looking at your previous message.  I agree
that "a" is more correct than "i" here.  In fact "A" might even be a
reasonable choice.

Another thing to check is the interaction of this with push-line.  Is
the right thing done if the buffer isn't empty when entering ZLE?

} > Is it OK to assume we're not in insert mode when vi-repeat is executed?

Wow, good question.  vi-repeat is implemented by replaying (a subset
of) the input keystrokes, not the widget actions they are bound to ...

} > If it's not safe, I don't understand how it ever worked.

If someone were to bind vi-repeat to a keystroke in the viins keymap,
I imagine the wrong thing would happen and always has.  Yep, just now
confirmed this.  If that's a bug, it's not a directly related one.

} I'm not hearing much in the way of knowledgeable direction for the
} completion issue.  It probably needs some hook trying out to see
} if it works.

I'm not familiar enough with the undo mechanics to make and good stabs
at such a hook, I fear.

Re: zle: vi mode: wrong undo handling on fresh lines

[Peter Stephenson]

On Tue, 28 Jan 2014 08:28:34 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 28,  2:58pm, Peter Stephenson wrote:
> }
> } > I think this is starting to get somewhere, but I suspect it needs
> } > tweaking.  For example, should that synthesised 'i' really be an 'a'?
> } 
> } Well, I think I'm going to commit it with the "a" because (i) this
> } definitely seems better than what was there before in any case and (ii)
> } hitting "ESC" and "." after typing the start of the line, which is my
> } naive way of getting it to repeat, does repeat what I just inserted
> } because of vi's quirk of backing up a character when leaving insert
> } mode.  Obviously I'm not claiming this is the end of the matter.
> 
> Sorry, I missed this when looking at your previous message.  I agree
> that "a" is more correct than "i" here.  In fact "A" might even be a
> reasonable choice.

Except, of course, if someone has redefined "a" / "A" in vicmd to the
widget delete-all-my-files-and-self-detonate.  Like the viins / vicmd
repeat thing you commented on, this is fundamental to the limitations of
the vi change buffer.  A partial fix might be to use tokens (and so
tokenise & metafy the vi change buffer), but I think this can of worms
is somewhat larger than that.

pws

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 28,  4:47pm, Peter Stephenson wrote:
}
} > Sorry, I missed this when looking at your previous message.  I agree
} > that "a" is more correct than "i" here.  In fact "A" might even be a
} > reasonable choice.
} 
} Except, of course, if someone has redefined "a" / "A" in vicmd to the
} widget delete-all-my-files-and-self-detonate.

We could reverse-lookup the bindking for vi-add-next / vi-add-eol and
insert whatever we find ...

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

Peter Stephenson wrote:
> Or startvitext() and startvichange() may be buried too far in to want
> any changing.
> 
> I think this is starting to get somewhere, but I suspect it needs
> tweaking.  For example, should that synthesised 'i' really be an 'a'?

If someone has the overstrike option set, it should probably be 'R'.
Actually that option seems to be fully non-working for vi mode.

With a blank starting line, it really is much of a muchness. If pushed
for an answer, I'd say 'a'.

A non-blank starting line is possible with at least
accept-line-and-down-history and vi-push-input. In such cases, it
depends on the cursor position. If the cursor starts at the
beginning of the line then 'i' makes more sense.

> Is it OK to assume we're not in insert mode when vi-repeat is executed?

Yes. Of course someone can bind a key to the widget but that currently
just beeps which is fine. To do that in vi, you'd have to bind your key
to escape, followed by a dot.

> Some vi user will need to take over at this point.  All the action I'm
> aware of is in the functions I've changed and if you've looked at the
> code you understand it as well as I do.
> 
> I haven't thought further about the completion thing.  I've no idea
> how that ever did anything useful.

How about the following approach to the undo problem:
The variable undoing was serving as a flag to indicate whether each
change should be added as an undo event. In vi-mode this would be
skipped so that the whole vi change became a single undo change. What
this does is remove that handling and instead merge all the undo events
corresponding to the vi change in the vi-cmd-mode widget.

This means, undo done the vi way works as in vi. undo invoked in insert
mode works exactly as in emacs mode and exactly as it used to do (up
till the first time vi command mode is invoked after which there are
differences). It also avoids the need for any special extra
vi-complete-word etc widgets that call handleundo(). The main
disadvantage is that undo will appear to be doing different things in
insert verses command mode which would probably need documenting under
the undo widget even though the change is in vi-cmd-mode. Any thoughts?

Are there other ways to get into vi command mode besides vi-cmd-mode
that might get around this? In vim, cursor movement in insert mode works
as if you had switched to command mode so we have some differences
there.

Oliver

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index a2b20df..f5aec84 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -157,10 +157,10 @@ mod_export char *statusline;
 /**/
 int stackhist, stackcs;
 
-/* != 0 if we are making undo records */
+/* position in undo stack from when the current vi change started */
 
 /**/
-int undoing;
+zlong vistartchange;
 
 /* current modifier status */
 
@@ -1080,8 +1080,7 @@ zlecore(void)
 	    if (invicmdmode() && zlecs > findbol() &&
 		(zlecs == zlell || zleline[zlecs] == ZWC('\n')))
 		DECCS();
-	    if (undoing)
-		handleundo();
+	    handleundo();
 	} else {
 	    errflag = 1;
 	    break;
@@ -1190,7 +1189,7 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     zlereadflags = flags;
     zlecontext = context;
     histline = curhist;
-    undoing = 1;
+    vistartchange = 0;
     zleline = (ZLE_STRING_T)zalloc(((linesz = 256) + 2) * ZLE_CHAR_SIZE);
     *zleline = ZWC('\0');
     virangeflag = lastcmd = done = zlecs = zlell = mark = 0;
@@ -1198,6 +1197,7 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     viinsbegin = 0;
     statusline = NULL;
     selectkeymap("main", 1);
+    initundo();
     /*
      * If main is linked to the viins keymap, we need to register
      * explicitly that we're now in vi insert mode as there's
@@ -1222,7 +1222,6 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
 	    stackhist = -1;
 	}
     }
-    initundo();
     if (isset(PROMPTCR))
 	putc('\r', shout);
     if (tmout)
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 9d163ad..2689d0f 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -611,8 +611,7 @@ docomplete(int lst)
     active = 1;
     comprecursive = 0;
     makecommaspecial(0);
-    if (undoing)
-	setlastline();
+    setlastline();
 
     /* From the C-code's point of view, we can only use compctl as a default
      * type of completion. Load it if it hasn't been loaded already and
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index b82e54c..61ae85c 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1354,7 +1354,10 @@ handlesuffix(UNUSED(char **args))
 
 /* head of the undo list, and the current position */
 
-static struct change *changes, *curchange;
+/**/
+struct change *curchange;
+
+static struct change *changes;
 
 /* list of pending changes, not yet in the undo system */
 
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 31f2933..9e9cc2f 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -107,7 +107,7 @@ startvitext(int im)
 {
     startvichange(im);
     selectkeymap("main", 1);
-    undoing = 0;
+    vistartchange = (curchange && curchange->prev) ? curchange->prev->changeno : 0;
     viinsbegin = zlecs;
 }
 
@@ -399,7 +399,7 @@ vichange(UNUSED(char **args))
 	forekill(c2 - zlecs, CUT_RAW);
 	selectkeymap("main", 1);
 	viinsbegin = zlecs;
-	undoing = 0;
+	vistartchange = curchange->prev->changeno;
     }
     return ret;
 }
@@ -584,7 +584,13 @@ vicmdmode(UNUSED(char **args))
 {
     if (invicmdmode() || selectkeymap("vicmd", 0))
 	return 1;
-    undoing = 1;
+    struct change *current = curchange->prev;
+    while (current && current->changeno > vistartchange+1) {
+	current->flags |= CH_PREV;
+	current = current->prev;
+	if (!current) break;
+	current->flags |= CH_NEXT;
+    }
     vichgflag = 0;
     if (zlecs != findbol())
 	DECCS();

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 29, 12:00am, Oliver Kiddle wrote:
} Subject: Re: zle: vi mode: wrong undo handling on fresh lines
}
} A non-blank starting line is possible with at least
} accept-line-and-down-history and vi-push-input. In such cases, it
} depends on the cursor position. If the cursor starts at the
} beginning of the line then 'i' makes more sense.
} 
} > Is it OK to assume we're not in insert mode when vi-repeat is executed?
} 
} Yes. Of course someone can bind a key to the widget but that currently
} just beeps which is fine.

Actually it does more than beep; with --

    bindkey -M viins '#' vi-repeat-change
    bindkey -v

-- type (whitespace added for clarity)

   xxx ESC a yyy #

and you end up with xxxyyya because # has repeated the "a" keystroke.
The yyy hasn't yet been added to the change buffer, so that doesn't get
repeated.  If you keep trying to use # from insert-mode (prior to the
patch in 32308), eventually it'll insert a "#", repeat carriage-return
and accept the buffer, probably resulting in a command-not-found.

} How about the following approach to the undo problem:
} The variable undoing was serving as a flag to indicate whether each
} change should be added as an undo event. In vi-mode this would be
} skipped so that the whole vi change became a single undo change. What
} this does is remove that handling and instead merge all the undo events
} corresponding to the vi change in the vi-cmd-mode widget.

This sounds fine to me, and wouldn't have been possible prior to the
numbering of undo events.  Is there a well-defined place where a user
defined widget could read $UNDO_CHANGE_NO and be sure it was the same
as the internal vistartchange value?
 
} Are there other ways to get into vi command mode besides vi-cmd-mode
} that might get around this?

One doc example for zle-line-init still implies you can get into vi
command mode by "zle -K vicmd".  (This even moves one character left
when switching from viins to vicmd; I'm not sure how/where the code
that does that is being called.)

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

Bart wrote:
> On Jan 29, 12:00am, Oliver Kiddle wrote:
> } depends on the cursor position. If the cursor starts at the
> } beginning of the line then 'i' makes more sense.

It occurs to me that for a blank line 'o' is just as valid as either 'i'
or 'a' but possibly more useful.

> } > Is it OK to assume we're not in insert mode when vi-repeat is executed?
 
> Actually it does more than beep; with --

Having played with this in various combinations, I think we should just
start virepeatchange() with:
    if (!invicmdmode())
	return 1;
Unless someone can be bothered to make it do something useful.

> This sounds fine to me, and wouldn't have been possible prior to the
> numbering of undo events.  Is there a well-defined place where a user
> defined widget could read $UNDO_CHANGE_NO and be sure it was the same
> as the internal vistartchange value?

Logically, reading it from within zle-keymap-select would do that except
UNDO_CHANGE_NO doesn't seem to be set in there. That would provide a
roundabout way to get at the contents of viinsbegin from a widget.

> One doc example for zle-line-init still implies you can get into vi
> command mode by "zle -K vicmd".  (This even moves one character left
> when switching from viins to vicmd; I'm not sure how/where the code
> that does that is being called.)

That does skip the code I added and some other vi-change related code
but I'm somewhat inclined to think it should stay that way to allow more
control from a zle widget. It would seem odd for zle -K to have extra
side-effects. The cursor repositioning is probably necessary, however.

Oliver

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 29, 11:50am, Oliver Kiddle wrote:
}
} It occurs to me that for a blank line 'o' is just as valid as either 'i'
} or 'a' but possibly more useful.

I like that idea.
 
} > } > Is it OK to assume we're not in insert mode when vi-repeat is executed?
} 
} Having played with this in various combinations, I think we should just
} start virepeatchange() with:
}     if (!invicmdmode())
} 	return 1;

I tend to agree, but I wonder (below):

} > One doc example for zle-line-init still implies you can get into vi
} > command mode by "zle -K vicmd".
} 
} That does skip the code I added and some other vi-change related code
} but I'm somewhat inclined to think it should stay that way

Does "zle -K vicmd" also skip things that would make the invicmdmode()
test (above) either insufficient or incorrect?

Re: zle: vi mode: wrong undo handling on fresh lines

[Jun T.]

This is my "trial" to (somewhat) improve the behavior of 'undo' after completion.
The patch below is against (git-HEAD)+(patch in 32314 by Oliver Kiddle).

The change in zle_tricky.c is to start a new undo-block when entering a
completion.

The change in zle_main.c is to make 'undo' no to bring back the suffix added
by completion but erased when going back to the command mode.
(This will also change the behavior in emacs-mode.)

Or these are just matter of preference and must not be hard-coded but
configurable?



diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index f5aec84..72650a8 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1332,8 +1332,10 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	    eofsent = 1;
 	    ret = 1;
 	} else {
-	    if(!(wflags & ZLE_KEEPSUFFIX))
+	    if(!(wflags & ZLE_KEEPSUFFIX)) {
 		removesuffix();
+		handleundo();
+	    }
 	    if(!(wflags & ZLE_MENUCMP)) {
 		fixsuffix();
 		invalidatelist();
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 2689d0f..49dae8a 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -673,6 +673,9 @@ docomplete(int lst)
      * an emulation.                                                      */
     if (viinsbegin > ztrsub(zlemetaline + wb, zlemetaline))
 	viinsbegin = ztrsub(zlemetaline + wb, zlemetaline);
+    /* finish the current undo block and start a new one */
+    vimergeundo();
+    vistartchange = (curchange && curchange->prev) ? curchange->prev->changeno : 0;
     /* If we added chline to the line buffer, reset the original contents. */
     if (ol) {
 	zlemetacs -= chl;
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 9e9cc2f..e6c10a1 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -112,6 +112,19 @@ startvitext(int im)
 }
 
 /**/
+void
+vimergeundo(void)
+{
+    struct change *current = curchange->prev;
+    while (current && current->changeno > vistartchange+1) {
+	current->flags |= CH_PREV;
+	current = current->prev;
+	if (!current) break;
+	current->flags |= CH_NEXT;
+    }
+}
+
+/**/
 ZLE_INT_T
 vigetkey(void)
 {
@@ -584,13 +597,7 @@ vicmdmode(UNUSED(char **args))
 {
     if (invicmdmode() || selectkeymap("vicmd", 0))
 	return 1;
-    struct change *current = curchange->prev;
-    while (current && current->changeno > vistartchange+1) {
-	current->flags |= CH_PREV;
-	current = current->prev;
-	if (!current) break;
-	current->flags |= CH_NEXT;
-    }
+    vimergeundo();
     vichgflag = 0;
     if (zlecs != findbol())
 	DECCS();

Re: zle: vi mode: wrong undo handling on fresh lines

[Peter Stephenson]

On Thu, 30 Jan 2014 23:51:10 +0900
"Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
> This is my "trial" to (somewhat) improve the behavior of 'undo' after
> completion.  The patch below is against (git-HEAD)+(patch in 32314 by
> Oliver Kiddle).

Thanks.

> The change in zle_tricky.c is to start a new undo-block when entering a
> completion.
> 
> The change in zle_main.c is to make 'undo' no to bring back the suffix added
> by completion but erased when going back to the command mode.
> (This will also change the behavior in emacs-mode.)
> 
> Or these are just matter of preference and must not be hard-coded but
> configurable?

I think this is probably OK generally since it seems to reflect fairly
intuitively what counts as a change.  With the ability to undo
repeatedly I certainly don't think this needs any more configuration.

It looks very much along the right lines, but I'll let the others
comment.

pws

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 30,  3:38pm, Peter Stephenson wrote:
}
} I think this is probably OK generally since it seems to reflect fairly
} intuitively what counts as a change.  With the ability to undo
} repeatedly I certainly don't think this needs any more configuration.
} 
} It looks very much along the right lines, but I'll let the others
} comment.

I don't use vi mode much, so I won't comment on that beyond saying that
this sounds like the approach I was advocating earlier.

As for emacs mode, I have no objection to changing the undo behavior in
the way described.

Re: zle: vi mode: wrong undo handling on fresh lines

[Jun T.]

Unconditionally calling vimergeundo() from docomplete() makes the
behavior of 'undo' in emacs mode. For example,

$ ls f<tab>
$ ls foo         # assume there is a file 'foo'
$ ls foo<undo>
$ ls f<undo>

now <undo> will remove the entire command line (the original behavior
is to remove only 'f').

If this is a problem, then consider the following patch
(this patch assume my patch in 32324 has been already applied):


diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 72650a8..316115a 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1189,7 +1189,7 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     zlereadflags = flags;
     zlecontext = context;
     histline = curhist;
-    vistartchange = 0;
+    vistartchange = -1;
     zleline = (ZLE_STRING_T)zalloc(((linesz = 256) + 2) * ZLE_CHAR_SIZE);
     *zleline = ZWC('\0');
     virangeflag = lastcmd = done = zlecs = zlell = mark = 0;
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 49dae8a..99cdb8f 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -674,8 +674,11 @@ docomplete(int lst)
     if (viinsbegin > ztrsub(zlemetaline + wb, zlemetaline))
 	viinsbegin = ztrsub(zlemetaline + wb, zlemetaline);
     /* finish the current undo block and start a new one */
-    vimergeundo();
-    vistartchange = (curchange && curchange->prev) ? curchange->prev->changeno : 0;
+    if (vistartchange >= 0) {
+	vimergeundo();
+	vistartchange = (curchange && curchange->prev) ?
+			    curchange->prev->changeno : 0;
+    }
     /* If we added chline to the line buffer, reset the original contents. */
     if (ol) {
 	zlemetacs -= chl;
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index e6c10a1..64bdbd8 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -116,6 +116,8 @@ void
 vimergeundo(void)
 {
     struct change *current = curchange->prev;
+    if (vistartchange < 0)
+	return;
     while (current && current->changeno > vistartchange+1) {
 	current->flags |= CH_PREV;
 	current = current->prev;

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 31,  9:00pm, Jun T. wrote:
} Subject: Re: zle: vi mode: wrong undo handling on fresh lines
}
} $ ls f<tab>
} $ ls foo         # assume there is a file 'foo'
} $ ls foo<undo>
} $ ls f<undo>
} 
} now <undo> will remove the entire command line (the original behavior
} is to remove only 'f').

Hmm.  I suspect we want the additional patch (32330) to restore the old
behavior, but emacs itself considers a string of consecutive self-insert
to be a single undo event (rather than undoing each individual character)
unless there are intervening motions or deletions, so whether this is a
significant problem may depend on exactly how many and what kind of undo
events get merged.

Re: zle: vi mode: wrong undo handling on fresh lines

[Peter Stephenson]

On Fri, 31 Jan 2014 07:19:44 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Jan 31,  9:00pm, Jun T. wrote:
> } Subject: Re: zle: vi mode: wrong undo handling on fresh lines
> }
> } $ ls f<tab>
> } $ ls foo         # assume there is a file 'foo'
> } $ ls foo<undo>
> } $ ls f<undo>
> } 
> } now <undo> will remove the entire command line (the original behavior
> } is to remove only 'f').
> 
> Hmm.  I suspect we want the additional patch (32330) to restore the old
> behavior, but emacs itself considers a string of consecutive self-insert
> to be a single undo event (rather than undoing each individual character)
> unless there are intervening motions or deletions, so whether this is a
> significant problem may depend on exactly how many and what kind of undo
> events get merged.

Yes, I'm going to assume both patches are wanted if I don't hear
disagreement.

pws

Re: zle: vi mode: wrong undo handling on fresh lines

[Jun T.]

2014/01/31 22:45, Oliver Kiddle wrote:

>  How about we instead add a split-undo zle
> widget? It is then easy for someone to add that before completion or
> other widgets like backward-delete-word.

I have no objection to this.

> I'm fairly convinced that we should handle the suffix with the simple
> patch below. I've long had vi-cmd-mode set to first use
> auto-suffix-retain. I could be wrong but would suspect most vi users
> would prefer it this way. However, it'd be good to have the opinion of
> another vi-mode user. What do you think?

In some cases the suffix is purely optional; for example

$ chown m<tab>
will complete to
$ chown myname:

or (on Mac OS X)

$ df -Th<tab>
completes to
$ df -Thfs,

I believe at least for these cases it would be better to erase the suffix
when <esc> is hit.


>> (This will also change the behavior in emacs-mode.)
> 
> I can't quite work out what the emacs-mode behaviour change is.

This is just a minor difference.
Suppose you have directories 'foo' and 'bar'. By hitting <tab> and <space>
a few times after 'ls ' the command line would look like

$ ls foo bar/

Now type <undo> a few times; without the change in zle_main.c, at some
point you get

$ ls foo/

but with my patch '/' will not be appended.

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

I sent this earlier but it hasn't arrived so resending. Sorry if it
appears twice.

"Jun T." wrote:
> This is my "trial" to (somewhat) improve the behavior of 'undo' after completion.
> The patch below is against (git-HEAD)+(patch in 32314 by Oliver Kiddle).

> The change in zle_tricky.c is to start a new undo-block when entering a
> completion.

I hate to be negative bit if we now split the undo block at all the
points where someone prefers it split. We'll end up back in the
situation Hauke Petersen complained about in September. It is also not
easy to recombine them. How about we instead add a split-undo zle
widget? It is then easy for someone to add that before completion or
other widgets like backward-delete-word.

I like the fact that a vi undo will now undo a whole change but I'm used
to using the emacs mode undo key to revert a completion. Also, as I
mentioned before, completion in vim doesn't split the undo.
 
> The change in zle_main.c is to make 'undo' no to bring back the suffix added
> by completion but erased when going back to the command mode.

I'm fairly convinced that we should handle the suffix with the simple
patch below. I've long had vi-cmd-mode set to first use
auto-suffix-retain. I could be wrong but would suspect most vi users
would prefer it this way. However, it'd be good to have the opinion of
another vi-mode user. What do you think?

> (This will also change the behavior in emacs-mode.)

I can't quite work out what the emacs-mode behaviour change is.

Oliver

PS. I have now committed 32314. I've put some tests together but will
hold off on posting to them until we've finalised the behaviour. I
looked at merging Felix's zle tests but as he basically indicates in the
e-mail, he wasn't really finished. A number of his tests fail and I
haven't been able to work out why.

diff --git a/Src/Zle/iwidgets.list b/Src/Zle/iwidgets.list
index 4372fe3..0e4c479 100644
--- a/Src/Zle/iwidgets.list
+++ b/Src/Zle/iwidgets.list
@@ -125,7 +125,7 @@
 "vi-change", vichange, 0
 "vi-change-eol", vichangeeol, 0
 "vi-change-whole-line", vichangewholeline, 0
-"vi-cmd-mode", vicmdmode, 0
+"vi-cmd-mode", vicmdmode, ZLE_KEEPSUFFIX
 "vi-delete", videlete, ZLE_KILL | ZLE_KEEPSUFFIX
 "vi-delete-char", videletechar, ZLE_KEEPSUFFIX
 "vi-digit-or-beginning-of-line", vidigitorbeginningofline, 0

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

I wrote:
> How about we instead add a split-undo zle
> widget? It is then easy for someone to add that before completion or
> other widgets like backward-delete-word.

I've attached a patch for this split-undo widget to demonstrate. As
you can see, it is very much based on Jun's changes.
If you want undo from vi command mode to stop at the start of a
completion, you would do:
  undo-complete-word() {
    zle split-undo
    zle complete-word
  }
  zle -N undo-complete-word
  bindkey '^I' undo-complete-word

That's not as simple as you might ideally like but it is at least
configurable. If we want to make this way default for completion, then
someone wanting to configure it needs to be able to reset the vi undo
start position.

split-undo can also be used in the middle of a user-defined zle widget if for
whatever reason you want to allow undos to stop half-way through the
actions of that widget.

> I can't quite work out what the emacs-mode behaviour change is.

That was because I had the second patch applied. Only the first patch is
not good because it combines more than just inserted characters.

Oliver

diff --git a/Src/Zle/iwidgets.list b/Src/Zle/iwidgets.list
index 4372fe3..a9791ed 100644
--- a/Src/Zle/iwidgets.list
+++ b/Src/Zle/iwidgets.list
@@ -102,6 +102,7 @@
 "self-insert-unmeta", selfinsertunmeta, ZLE_MENUCMP | ZLE_KEEPSUFFIX
 "send-break", sendbreak, 0
 "set-mark-command", setmarkcommand, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL
+"split-undo", splitundo, ZLE_KEEPSUFFIX
 "spell-word", spellword, 0
 "set-local-history", setlocalhistory, 0
 "transpose-chars", transposechars, 0
@@ -125,7 +126,7 @@
 "vi-change", vichange, 0
 "vi-change-eol", vichangeeol, 0
 "vi-change-whole-line", vichangewholeline, 0
-"vi-cmd-mode", vicmdmode, 0
+"vi-cmd-mode", vicmdmode, ZLE_KEEPSUFFIX
 "vi-delete", videlete, ZLE_KILL | ZLE_KEEPSUFFIX
 "vi-delete-char", videletechar, ZLE_KEEPSUFFIX
 "vi-digit-or-beginning-of-line", vidigitorbeginningofline, 0
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index f5aec84..ed8577b 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1189,7 +1189,7 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     zlereadflags = flags;
     zlecontext = context;
     histline = curhist;
-    vistartchange = 0;
+    vistartchange = -1;
     zleline = (ZLE_STRING_T)zalloc(((linesz = 256) + 2) * ZLE_CHAR_SIZE);
     *zleline = ZWC('\0');
     virangeflag = lastcmd = done = zlecs = zlell = mark = 0;
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 61ae85c..68d27f8 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1632,6 +1632,32 @@ viundochange(char **args)
 	return undo(args);
 }
 
+/**/
+int
+splitundo(char **args)
+{
+    if (vistartchange >= 0) {
+	mergeundo();
+	vistartchange = (curchange && curchange->prev) ?
+	    curchange->prev->changeno : 0;
+    }
+    handleundo();
+    return 0;
+}
+
+/**/
+void
+mergeundo(void)
+{
+    struct change *current = curchange->prev;
+    while (current && current->changeno > vistartchange+1) {
+	current->flags |= CH_PREV;
+	current = current->prev;
+	if (!current) break;
+	current->flags |= CH_NEXT;
+    }
+}
+
 /*
  * Call a ZLE hook: a user-defined widget called at a specific point
  * within the line editor.
@@ -1658,7 +1684,6 @@ zlecallhook(char *name, char *arg)
     args[1] = NULL;
     execzlefunc(thingy, args, 1);
     unrefthingy(thingy);
-
     errflag = saverrflag;
     retflag = savretflag;
 }
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 9e9cc2f..79b8cb9 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -584,13 +584,7 @@ vicmdmode(UNUSED(char **args))
 {
     if (invicmdmode() || selectkeymap("vicmd", 0))
 	return 1;
-    struct change *current = curchange->prev;
-    while (current && current->changeno > vistartchange+1) {
-	current->flags |= CH_PREV;
-	current = current->prev;
-	if (!current) break;
-	current->flags |= CH_NEXT;
-    }
+    mergeundo();
     vichgflag = 0;
     if (zlecs != findbol())
 	DECCS();

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Jan 31, 11:32pm, Oliver Kiddle wrote:
} Subject: Re: zle: vi mode: wrong undo handling on fresh lines
}
} I wrote:
} > How about we instead add a split-undo zle
} > widget? It is then easy for someone to add that before completion or
} > other widgets like backward-delete-word.
} 
} I've attached a patch for this split-undo widget to demonstrate.

This all seems fine to me.

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

On 29 Jan, I wrote:
> If someone has the overstrike option set, it should probably be 'R'.
> Actually that option seems to be fully non-working for vi mode.

It seems overstrike in vi mode is something we only just broke. This
fixes it. Also, having collectively put some thought into what the
synthesised vi command should be when repeating the first change, I
thought it'd be a waste not to do something – even though it is a
feature that nobody has ever missed. This uses 'o' for blank lines,
mainly because it is the most useful, 'R' if overstrike is set, 'a' if
the cursor is at the end of a non-blank line and 'i' otherwise.

Oliver

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index ed8577b..b0010fc 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1198,14 +1198,6 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     statusline = NULL;
     selectkeymap("main", 1);
     initundo();
-    /*
-     * If main is linked to the viins keymap, we need to register
-     * explicitly that we're now in vi insert mode as there's
-     * no user operation to indicate this.
-     */
-    if (openkeymap("main") == openkeymap("viins"))
-	viinsert_init();
-    selectlocalmap(NULL);
     fixsuffix();
     if ((s = getlinknode(bufstack))) {
 	setline(s, ZSL_TOEND);
@@ -1222,6 +1214,14 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
 	    stackhist = -1;
 	}
     }
+    /*
+     * If main is linked to the viins keymap, we need to register
+     * explicitly that we're now in vi insert mode as there's
+     * no user operation to indicate this.
+     */
+    if (openkeymap("main") == openkeymap("viins"))
+	viinsert_init();
+    selectlocalmap(NULL);
     if (isset(PROMPTCR))
 	putc('\r', shout);
     if (tmout)
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 79b8cb9..994b44f 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -79,8 +79,9 @@ static void
 startvichange(int im)
 {
     if (im != -1) {
-	insmode = im;
 	vichgflag = 1;
+	if (im > -1)
+	    insmode = im;
     }
     if (inrepeat && im != -2) {
 	zmod = lastmod;
@@ -92,7 +93,8 @@ startvichange(int im)
 	    free(vichgbuf);
 	vichgbuf = (char *)zalloc(vichgbufsz = 16);
 	if (im == -2) {
-	    vichgbuf[0] = 'a';
+	    vichgbuf[0] =
+		zlell ? (insmode ? (zlecs < zlell ? 'i' : 'a') : 'R') : 'o';
 	} else {
 	    vichgbuf[0] = lastchar;
 	}

Re: zle: vi mode: wrong undo handling on fresh lines

[Jun T.]

2014/02/01 07:32, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:

> I've attached a patch for this split-undo widget to demonstrate.

I tried this split-undo patch, but if I bind <tab> to
undo-complete-word(), there seems to be at least two problems.

The fist is that AUTO_MENU doesn't work; when there are more than one
candidates (ambiguous), hitting <tab> repeatedly does not insert the
first (and next ...) candidate into the command line.

This may be fixed by the patch at the end of this post.

The second is that the zsh/complist module doesn't work properly. 
When the listscroll keymap is in use, complete-word widget is
equivalent to 'scroll forward one screenful', and when the menuselect
keymap is in use it is equivalent to 'moves the mark to the next match'.
But if <tab> is bound to undo-complete-word, it doesn't behave this way
but just accepts the current command line. I don't know how to overcome
this problem.


As for the KEEPSUFFIX flag, the undo works fine with this flag set to
split-undo widget. But I feel keeping the optional suffix like ':' or
',' when going back to the command mode may be sometimes annoying
(yes it can be deleted just by hitting 'x' but I'm so lazy...).



diff --git a/Src/Zle/iwidgets.list b/Src/Zle/iwidgets.list
index a9791ed..7872745 100644
--- a/Src/Zle/iwidgets.list
+++ b/Src/Zle/iwidgets.list
@@ -102,7 +102,7 @@
 "self-insert-unmeta", selfinsertunmeta, ZLE_MENUCMP | ZLE_KEEPSUFFIX
 "send-break", sendbreak, 0
 "set-mark-command", setmarkcommand, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL
-"split-undo", splitundo, ZLE_KEEPSUFFIX
+"split-undo", splitundo, ZLE_MENUCMP | ZLE_KEEPSUFFIX
 "spell-word", spellword, 0
 "set-local-history", setlocalhistory, 0
 "transpose-chars", transposechars, 0

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

"Jun T." wrote:
> 
> The second is that the zsh/complist module doesn't work properly. 
> When the listscroll keymap is in use, complete-word widget is

What have you got tab bound to in the listscroll and menuselect keymaps?
It is probably best to leave them bound to complete-word there. I've not
managed to reproduce the problem exactly as you describe so it is
probably fairly dependant on your exact configuration.

It really ought to be possible to redefine complete-word. The following
works with old-style completion but not with the new. Anyone have an
idea why?

complete-word() {
  zle .complete-word
}
zle -N complete-word

Other ideas such as calling zle split-undo in a complete function don't
work because you apparently can't call zle widgets from completion
widgets.

> As for the KEEPSUFFIX flag, the undo works fine with this flag set to
> split-undo widget. But I feel keeping the optional suffix like ':' or
> ',' when going back to the command mode may be sometimes annoying
> (yes it can be deleted just by hitting 'x' but I'm so lazy...).

I'll leave KEEPSUFFIX on vi-cmd-mode alone then. That's already
configurable so I'm not especially bothered.

Oliver

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On Mon, Feb 3, 2014 at 1:29 PM, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:

>
> It really ought to be possible to redefine complete-word. The following
> works with old-style completion but not with the new. Anyone have an
> idea why?
>
> complete-word() {
>   zle .complete-word
> }
> zle -N complete-word
>

The obvious reason is that "new completion" has already re-bound
complete-word to the _main_complete function.  You can't override the
.complete-word form, so you're always going to bypass new completion when
running "zle .complete-word".

The second possible problem is that you've changed complete-word from a
completion widget (defined with "zle -C") into a normal widget.  It mostly
works to call completion widgets from normal ones (though not the other way
around) but you have to be careful, and you might need to make calls to the
special auto-suffix-* widgets at the right times.

To get what I think you're after here, you'd more likely want

complete_word() {
  _main_complete "$@"
}
zle -C complete-word .complete-word complete-word

but I'm not certain.

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

Bart wrote:
> The obvious reason is that "new completion" has already re-bound
> complete-word to the _main_complete function.  You can't override the
> .complete-word form, so you're always going to bypass new completion when
> running "zle .complete-word".

Oh, of course. That makes sense. Thanks.

> To get what I think you're after here, you'd more likely want
> 
> complete_word() {
>   _main_complete "$@"
> }
> zle -C complete-word .complete-word complete-word

I was trying to find a way to call split-undo before completion without
binding tab to something that isn't named complete-word. This tends to
help for older parts of the shell like incremental history searches
that don't use their own keymap and I theorised that it might help on
the second problem mentioned by Jun. However, menuselect and listscroll
actually have their own keymaps so perhaps not.

Anyway with the help of your hint, perhaps this:
  zle -C undo-complete-word .complete-word _main_complete
  complete-word() {
    zle split-undo
    zle undo-complete-word
  }
  zle -N complete-word

Or perhaps the same but with a custom name for the function that is
then aliased to complete-word with zle -A.

With that, split-undo is interfering with AUTO_MENU. The following patch
seems to sort that out. At least in my setup, it now seems to work.

Oliver

diff --git a/Src/Zle/iwidgets.list b/Src/Zle/iwidgets.list
index 7872745..95cdca2 100644
--- a/Src/Zle/iwidgets.list
+++ b/Src/Zle/iwidgets.list
@@ -102,7 +102,7 @@
 "self-insert-unmeta", selfinsertunmeta, ZLE_MENUCMP | ZLE_KEEPSUFFIX
 "send-break", sendbreak, 0
 "set-mark-command", setmarkcommand, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL
-"split-undo", splitundo, ZLE_MENUCMP | ZLE_KEEPSUFFIX
+"split-undo", splitundo, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_NOTCOMMAND
 "spell-word", spellword, 0
 "set-local-history", setlocalhistory, 0
 "transpose-chars", transposechars, 0

Re: zle: vi mode: wrong undo handling on fresh lines

[Jun T.]

2014/02/04 08:26, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Anyway with the help of your hint, perhaps this:
>  zle -C undo-complete-word .complete-word _main_complete
>  complete-word() {
>    zle split-undo
>    zle undo-complete-word
>  }
>  zle -N complete-word

Thanks. It seems this solves my second problem.

There is another problem, however:

$ bindkey -v
$ echo xxx<esc>u     # type something and then undo
$                    # this is OK
$ echo yyy           # hit 'i' and type something more
$ echo yyy<esc>u     # undo again
$                    # also OK, but if I hit 'u' again
$ echo yyy           # then the command line goes back to this

Of course the expected behavior is that the command line remains
empty because there is nothing more to undo.
A possible patch is attached below. What the patch does is to
set CH_PREV in current->flags only if current->prev exists.

# If CH_PREV is set when current->prev is NULL, undo() returns
# without calling setlastline() (zle_utils.c, line 1543).
# This means lastline remains as "echo yyy" even if zleline is
# erased to "", and next call of handleundo() will add a wrong
# undo entry.



diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 68d27f8..3af8ed7 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1649,12 +1649,12 @@ splitundo(char **args)
 void
 mergeundo(void)
 {
-    struct change *current = curchange->prev;
-    while (current && current->changeno > vistartchange+1) {
+    struct change *current;
+    for (current = curchange->prev;
+	    current && current->prev && current->changeno > vistartchange+1;
+	    current = current->prev) {
 	current->flags |= CH_PREV;
-	current = current->prev;
-	if (!current) break;
-	current->flags |= CH_NEXT;
+	current->prev->flags |= CH_NEXT;
     }
 }
 

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

"Jun T." wrote:
> Thanks. It seems this solves my second problem.
> 
> There is another problem, however:

Thanks. Hopefully that's the end of it. Below is a patch for documentation and
test cases. I'll push the changes now.

Oliver

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index 2d77568..6d3bb4b 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -1293,8 +1293,11 @@ item(tt(redisplay))(
 Redisplay the command line, remaining in incremental search mode.
 )
 item(tt(vi-cmd-mode))(
-Toggle between the `tt(main)' and `tt(vicmd)' keymaps;
+Select the `tt(vicmd)' keymap;
 the `tt(main)' keymap (insert mode) will be selected initially.
+
+In addition, the modifications that were made while in vi insert mode are
+merged to form a single undo event.
 )
 xitem(tt(vi-repeat-search))
 item(tt(vi-rev-repeat-search))(
@@ -2191,6 +2194,13 @@ tindex(spell-word)
 item(tt(spell-word) (ESC-$ ESC-S ESC-s) (unbound) (unbound))(
 Attempt spelling correction on the current word.
 )
+tindex(split-undo)
+item(tt(split-undo))(
+Breaks the undo sequence at the current change.  This is useful in vi mode as
+changes made in insert mode are coalesced on entering command mode.  Similarly,
+tt(undo) will normally revert as one all the changes made by a user-defined
+widget.
+)
 tindex(undefined-key)
 item(tt(undefined-key))(
 This command is executed when a key sequence that is not bound to any
@@ -2202,6 +2212,10 @@ 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.
+
+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
+selected.
 )
 tindex(redo)
 item(tt(redo))(
diff --git a/Test/X02zlevi.ztst b/Test/X02zlevi.ztst
index d4a125f..fe55d8a 100644
--- a/Test/X02zlevi.ztst
+++ b/Test/X02zlevi.ztst
@@ -10,6 +10,63 @@
 
 %test
 
+  zletest $'word\euaend'
+0:undo initial change
+>BUFFER: end
+>CURSOR: 3
+
+  zletest $'text\e.'
+0:repeat initial edit
+>BUFFER: text
+>text
+>CURSOR: 8
+
+  comptesteval 'print -z before'
+  zletest $'after\e.'
+0:repeat initial edit with non-blank starting line
+>BUFFER: beforeafterafter
+>CURSOR: 15
+
+  comptesteval 'setopt overstrike;print -z bung'
+  zletest $'ing\e2|.'
+0:repeat initial edit with overstrike set
+>BUFFER: binging
+>CURSOR: 3
+
+  comptesteval 'bindkey "^_" undo'
+  zletest $'undoc\037e'
+0:use of undo in vi insert mode
+>BUFFER: undoe
+>CURSOR: 5
+
+  zletest $'one\euatwo\e0clthree'
+0:vi mode undo of initial and subsequent change
+>BUFFER: threewo
+>CURSOR: 5
+
+  zletest $'xxx\euiyyy\euAz'
+0:undo invoked twice
+>BUFFER: z
+>CURSOR: 1
+
+  comptesteval 'bindkey -a "^R" redo'
+  zletest $'123\C-_\e\C-r'
+0:undo in insert mode, redo in command
+>BUFFER: 123
+>CURSOR: 2
+
+  comptesteval 'bindkey "^Y" redo'
+  zletest $'pre\eA123\C-_\C-y\eu'
+0:undo and redo in insert mode, undo in command
+>BUFFER: pre
+>CURSOR: 2
+
+  comptesteval 'bindkey "^Gu" split-undo'
+  zletest $'one\C-gutwo\eu'
+0:split the undo sequence
+>BUFFER: one
+>CURSOR: 2
+
   zletest $'one two\ebmt3|`tx``'
 0:setting mark and returning to original position
 >BUFFER: one wo
diff --git a/Test/comptest b/Test/comptest
index 10814d6..f1c5af0 100644
--- a/Test/comptest
+++ b/Test/comptest
@@ -72,8 +72,8 @@ zle-finish () {
   print -lr "<WIDGET><finish>" "BUFFER: $BUFFER" "CURSOR: $CURSOR"
   (( region_active )) && print -lr "MARK: $MARK"
   zle -K main
-  zle kill-whole-line
   zle clear-screen
+  zle send-break
   zle -R
 }
 zle -N expand-or-complete-with-report

Re: zle: vi mode: wrong undo handling on fresh lines

[Oliver Kiddle]

On 29 Jan, I wrote:
> +++ b/Src/Zle/zle_tricky.c
> @@ -611,8 +611,7 @@ docomplete(int lst)

> -    if (undoing)
> -	setlastline();
> +    setlastline();

I have a widget that adds text to LBUFFER and invokes completion.
The text added to LBUFFER can't be undone with this change and with
emacs mode, the problem was there before. I think this line should just
be removed as it doesn't make much sense anyway.

It was added in workers/8590 (a reply to 8574). Removing it doesn't seem
to cause problems that I can find though the examples in 8574 seem
broken in other ways.

Oliver

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index a12fa08..3c7cff9 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -611,7 +611,6 @@ docomplete(int lst)
     active = 1;
     comprecursive = 0;
     makecommaspecial(0);
-    setlastline();
 
     /* From the C-code's point of view, we can only use compctl as a default
      * type of completion. Load it if it hasn't been loaded already and

Re: zle: vi mode: wrong undo handling on fresh lines

[Bart Schaefer]

On Feb 7,  3:43pm, Oliver Kiddle wrote:
}
} > -    if (undoing)
} > -	setlastline();
} > +    setlastline();
} 
} I think this line should just
} be removed as it doesn't make much sense anyway.

It's quite possible that whatever effect that was supposed to have has
been obsoleted by other code added in the last 14+ years!
 
} It was added in workers/8590 (a reply to 8574). Removing it doesn't seem
} to cause problems that I can find though the examples in 8574 seem
} broken in other ways.

I tried all the examples in 8574 except for the last (undo-related) few.
They all worked for me ... however I didn't go so far as to try them
with no styles at all.

One thing about the "ls /u/i/s*/f*.h<TAB>" example ... although it works,
on my machine there are two matches ("sys" and "selinux") for the "s*"
component, and when those are presented in a menu the cursor remains at
the end of the word so there's no simple way to chose one of the two
and then continue expanding "f*.h".  You have to hit left/right or
space/backspace to interrupt cycling between those two choices before
hitting TAB again.