Re: vi-backward-kill-word

Re: vi-backward-kill-word

[Jun T.]

Another minor bug in vi mode.

If you enter a combined char and delete it in vi-insert mode by ctrl-H
(bound to vi-backward-delete-char), then only the base char is deleted.

In this case, vibackwarddeletechar() calls backkill() without setting
CUT_RAW flag. backkill() correclty finds the new zlecs but does not
re-calculate the number of raw chars to be killed (the variable ct).
A possible fix may be the following:


Index: Src/Zle/zle_utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_utils.c,v
retrieving revision 1.52
diff -u -r1.52 zle_utils.c
--- Src/Zle/zle_utils.c	21 Apr 2008 17:58:59 -0000	1.52
+++ Src/Zle/zle_utils.c	24 Apr 2008 09:48:38 -0000
@@ -589,20 +589,18 @@
 mod_export void
 backkill(int ct, int flags)
 {
-    int i;
-
     UNMETACHECK();
     if (flags & CUT_RAW) {
-	i = (zlecs -= ct);
+	zlecs -= ct;
     } else {
-	int n = ct;
-	while (n--)
+	int origcs = zlecs;
+	while (ct--)
 	    DECCS();
-	i = zlecs;
+	ct = origcs - zlecs;
     }
 
-    cut(i, ct, flags);
-    shiftchars(i, ct);
+    cut(zlecs, ct, flags);
+    shiftchars(zlecs, ct);
     CCRIGHT();
 }
 

------
Jun

vi-backward-kill-word

[Jun T.]

Since you wrote in your first post:
 >Known gaps at the moment:
 >- Word tests need improving (a whole smattering HEREs)
 >- Vi "r" needs some work (another HERE)

I guess you already know that vi "r" and "R" do not work properly
on combined character(s).
Another minor problem is, when  in vi-insert mode, ^W
(vi-backward-kill-word) kills wrong part of the line if the word
to be killed contains combined character.

zsh -f
bindkey -v
echo é<^W>
echó
(Here é is in decomposed form: e + U+0301).

The word "é" must be killed, but what is actually killed is " e"
(a space and "e"), leaving the combining character.

It *seems* it can be fixed by replacing the line 44 of zle_word.c
     backkill(zlecs - x, CUT_FRONT);
by
     backkill(zlecs - x, CUT_FRONT|CUT_RAW);

In the present case, the comgining character(s) are already taken
accout of by vibackwardkillword(), and 'zlecs - x' is the number
of wchar_t to be killed. So I *guess* CUT_RAW should be used here.
Am I right?

# I guess you are busy working on many problems.
# I fear I'm disturbing you...

Re: vi-backward-kill-word

[Peter Stephenson]

On Tue, 22 Apr 2008 01:10:40 +0900
"Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
> Since you wrote in your first post:
>  >Known gaps at the moment:
>  >- Word tests need improving (a whole smattering HEREs)
>  >- Vi "r" needs some work (another HERE)
> 
> I guess you already know that vi "r" and "R" do not work properly
> on combined character(s).

I would have if I'd thought about it.  The problem with R was actually
generic to overstrike (non-insert) mode.  I think there's still a problem
in overstrike mode when you enter a combining character; it'll advance too
far right.  I'll look at that separately.

It's still the case that you can't use r and R with characters with
combining characters as arguments.  That's quite tough, since you don't
know they're are going to be any to read after the base character when
reading from the terminal.  We could use a timeout, I suppose, but even
so it would need a whole new layer of character input.

> Another minor problem is, when  in vi-insert mode, ^W
> (vi-backward-kill-word) kills wrong part of the line if the word
> to be killed contains combined character.
>...
> It *seems* it can be fixed by replacing the line 44 of zle_word.c
>      backkill(zlecs - x, CUT_FRONT);
> by
>      backkill(zlecs - x, CUT_FRONT|CUT_RAW);

Yep, that looks right... the CUT_RAW is needed any time the first argument
is a number of raw characters in the buffer, which it must be if it's
a difference of positions.  In fact, CUT_RAW should really be the default
because there are only very few places where the number comes directly from
the user where it isn't wanted.

I also found that vi-put-after (p) wasn't working for the usual reason to
do with character positions.

> # I guess you are busy working on many problems.
> # I fear I'm disturbing you...

Actually, I'd quite like to get the combining characters sorted out as soon
as possible.

Index: Src/Zle/zle_misc.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_misc.c,v
retrieving revision 1.49
diff -u -r1.49 zle_misc.c
--- Src/Zle/zle_misc.c	20 Apr 2008 21:23:14 -0000	1.49
+++ Src/Zle/zle_misc.c	21 Apr 2008 17:19:56 -0000
@@ -47,14 +47,27 @@
     iremovesuffix(c1, 0);
     invalidatelist();
 
-    if(insmode)
+    if (insmode)
 	spaceinline(m * len);
-    else if(zlecs + m * len > zlell)
-	spaceinline(zlecs + m * len - zlell);
-    while(m--)
+    else {
+	int pos = zlecs, count = m * len, i = count, diff;
+	/*
+	 * Ensure we replace a complete combining character
+	 * for each character we overwrite.
+	 */
+	while (pos < zlell && i--) {
+	    INCPOS(pos);
+	}
+	diff = pos - zlecs - count;
+	if (diff < 0) {
+	    spaceinline(-diff);
+	} else if (diff > 0)
+	    foredel(diff, CUT_RAW);
+    }
+    while (m--)
 	for(s = zstr, count = len; count; s++, count--)
 	    zleline[zlecs++] = *s;
-    if(neg)
+    if (neg)
 	zlecs += zmult * len;
     /* if we ended up on a combining character, skip over it */
     CCRIGHT();
Index: Src/Zle/zle_vi.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_vi.c,v
retrieving revision 1.19
diff -u -r1.19 zle_vi.c
--- Src/Zle/zle_vi.c	20 Apr 2008 21:17:30 -0000	1.19
+++ Src/Zle/zle_vi.c	21 Apr 2008 17:19:59 -0000
@@ -498,18 +498,19 @@
 vireplacechars(UNUSED(char **args))
 {
     ZLE_INT_T ch;
-    int n = zmult, origcs = zlecs, fail = 0;
+    int n = zmult, fail = 0, newchars = 0;
 
     if (n > 0) {
+	int pos = zlecs;
 	while (n-- > 0) {
-	    if (zlecs == zlell || zleline[zlell] == ZWC('\n')) {
+	    if (pos == zlell || zleline[pos] == ZWC('\n')) {
 		fail = 1;
 		break;
 	    }
-	    INCCS();
+	    newchars++;
+	    INCPOS(pos);
 	}
-	n = zlecs - origcs;
-	zlecs = origcs;
+	n = pos - zlecs;
     }
     startvichange(1);
     /* check argument range */
@@ -535,8 +536,16 @@
 	backkill(n - 1, CUT_RAW);
 	zleline[zlecs++] = '\n';
     } else {
-	/* HERE: we shouldn't replace combining chars, we should delete them */
-	while (n--)
+	/*
+	 * Make sure we delete displayed characters, including
+	 * attach combining characters. n includes this as a raw
+	 * buffer offset.
+	 */
+	if (n > newchars)
+	    foredel(n - newchars, CUT_RAW);
+	else if (n < newchars)
+	    spaceinline(newchars - n);
+	while (newchars--)
 	    zleline[zlecs++] = ch;
 	zlecs--;
     }
@@ -792,14 +801,14 @@
 	vifirstnonblank(zlenoargs);
     } else {
 	if (zlecs != findeol())
-	    zlecs++;
+	    INCCS();
 	while (n--) {
 	    spaceinline(buf->len);
 	    ZS_memcpy(zleline + zlecs, buf->buf, buf->len);
 	    zlecs += buf->len;
 	}
 	if (zlecs)
-	    zlecs--;
+	    DECCS();
     }
     return 0;
 }
Index: Src/Zle/zle_word.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_word.c,v
retrieving revision 1.13
diff -u -r1.13 zle_word.c
--- Src/Zle/zle_word.c	20 Apr 2008 21:17:30 -0000	1.13
+++ Src/Zle/zle_word.c	21 Apr 2008 17:20:00 -0000
@@ -441,7 +441,7 @@
 	    }
 	}
     }
-    backkill(zlecs - x, CUT_FRONT);
+    backkill(zlecs - x, CUT_FRONT|CUT_RAW);
     return 0;
 }
 
@@ -475,7 +475,7 @@
 	    x = pos;
 	}
     }
-    backkill(zlecs - x, CUT_FRONT);
+    backkill(zlecs - x, CUT_FRONT|CUT_RAW);
     return 0;
 }
 
-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: vi-backward-kill-word

[Peter Stephenson]

On Mon, 21 Apr 2008 18:27:28 +0100
Peter Stephenson <pws@csr.com> wrote:
> I think there's still a problem
> in overstrike mode when you enter a combining character; it'll advance too
> far right.  I'll look at that separately.

I think this deals with it.

I saw some oddities with undo, but that may simply be because in a version
I hadn't quite got right the cursor position was screwy.  I didn't see them
with the final version.

Index: Src/Zle/zle_misc.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_misc.c,v
retrieving revision 1.50
diff -u -r1.50 zle_misc.c
--- Src/Zle/zle_misc.c	21 Apr 2008 17:30:35 -0000	1.50
+++ Src/Zle/zle_misc.c	21 Apr 2008 17:53:42 -0000
@@ -50,19 +50,43 @@
     if (insmode)
 	spaceinline(m * len);
     else {
-	int pos = zlecs, count = m * len, i = count, diff;
+	int pos = zlecs, diff, i;
+
+	/*
+	 * Calculate the number of character positions we are
+	 * going to be using.  The algorithm is that
+	 * anything that shows up as a logical single character
+	 * (i.e. even if control, or double width, or with combining
+	 * characters) is treated as 1 for the purpose of replacing
+	 * what's there already.
+	 */
+	for (i = 0, count = 0; i < len; i++) {
+	    int width = wcwidth(zstr[i]);
+	    count += (width != 0) ? 1 : 0;
+	}
 	/*
 	 * Ensure we replace a complete combining character
 	 * for each character we overwrite.
 	 */
-	while (pos < zlell && i--) {
+	for (i = count; pos < zlell && i--; ) {
 	    INCPOS(pos);
 	}
-	diff = pos - zlecs - count;
+	/*
+	 * Calculate how many raw line places we need.
+	 * pos - zlecs is the raw line distance we're replacing,
+	 * m * len the number we're inserting.
+	 */
+	diff = pos - zlecs - m * len;
 	if (diff < 0) {
 	    spaceinline(-diff);
-	} else if (diff > 0)
-	    foredel(diff, CUT_RAW);
+	} else if (diff > 0) {
+	    /*
+	     * We use shiftchars() here because we don't
+	     * want combining char alignment fixed up: we
+	     * are going to write over any that remain.
+	     */
+	    shiftchars(zlecs, diff);
+	}
     }
     while (m--)
 	for (s = zstr, count = len; count; s++, count--)
Index: Src/Zle/zle_utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_utils.c,v
retrieving revision 1.51
diff -u -r1.51 zle_utils.c
--- Src/Zle/zle_utils.c	21 Apr 2008 10:13:31 -0000	1.51
+++ Src/Zle/zle_utils.c	21 Apr 2008 17:53:42 -0000
@@ -450,7 +450,7 @@
 }
 
 /**/
-static void
+void
 shiftchars(int to, int cnt)
 {
     if (mark >= to + cnt)

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: vi-backward-kill-word

[Jun T.]

Vi "r" seems to still have some problems.

Suppose you have a command line like:

   echo aéo

and move the cursor to é and type "rb". Then the command line
will become

   echo áb

In vireplacechars(), line 545 (zle_vi.c),
             foredel(n - newchars, CUT_RAW);

this will remove the base character "e" at zlecs, shift the
combining character, and move zlecs to the next base char "o".

     a e comb o
       |
       zlecs

will become

     a comb o
            |
            zlecs

and then "o" is replaced by "b".

Re: vi-backward-kill-word

[Peter Stephenson]

"Jun T." wrote:
> In vireplacechars(), line 545 (zle_vi.c),
>              foredel(n - newchars, CUT_RAW);
> 
> this will remove the base character "e" at zlecs, shift the
> combining character, and move zlecs to the next base char "o".

Yes, that needs to use shiftchars(), as I've already done in overwrite
mode.

Index: Src/Zle/zle_vi.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_vi.c,v
retrieving revision 1.20
diff -u -r1.20 zle_vi.c
--- Src/Zle/zle_vi.c	21 Apr 2008 17:30:36 -0000	1.20
+++ Src/Zle/zle_vi.c	23 Apr 2008 08:31:48 -0000
@@ -540,9 +540,11 @@
 	 * Make sure we delete displayed characters, including
 	 * attach combining characters. n includes this as a raw
 	 * buffer offset.
+	 * Use shiftchars so as not to adjust the cursor position;
+	 * we are overwriting anything that remains directly.
 	 */
 	if (n > newchars)
-	    foredel(n - newchars, CUT_RAW);
+	    shiftchars(zlecs, n - newchars);
 	else if (n < newchars)
 	    spaceinline(newchars - n);
 	while (newchars--)

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070