"off by one fix in multiple prompts" breaks multiline prompt

"off by one fix in multiple prompts" breaks multiline prompt

[Guillaume Chazarain]

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

Hi all,

This commit[1] discussed here[2] breaks line editing when the prompt spans
multiple lines, that happens for me since I include the full CWD in the
prompt. See [3] for a demo. In the first "echo", the "j" is not visible in
the command line and then when I try to edit the command line by adding a
space, the command line does not match what's executed.

This is on Debian amd64, using gnome-terminal 3.26.0.

[1]
https://sourceforge.net/p/zsh/code/ci/d8d9fee137a5aa2cf9bf8314b06895bfc2a05518/
[2] http://www.zsh.org/mla/workers/2018/msg00091.html
[3] https://gfycat.com/DecimalAdmiredJumpingbean

-- 
Guillaume

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Peter Stephenson]

On Fri, 10 Aug 2018 09:26:17 +0200
Guillaume Chazarain <guichaz@gmail.com> wrote:

> This commit[1] discussed here[2] breaks line editing when the prompt
> spans multiple lines, that happens for me since I include the full
> CWD in the prompt. See [3] for a demo. In the first "echo", the "j"
> is not visible in the command line and then when I try to edit the
> command line by adding a space, the command line does not match
> what's executed.
> 
> This is on Debian amd64, using gnome-terminal 3.26.0.

The problem with this is all terminals are a bit different in generally
undetectable ways and off-by-one errors are endemic.  Zsh is
particularly sensitive as not many other tools have multiline effects
without taking over the output of the entire screen.

I think I replied to a previous report saying about the best we could
do to fix generally was add a user control, though I'm not sure what
for that would take --- it would just have to be "suck it and see"
but it's an obscure thing to have to look for.

pws
 
> [1]
> https://sourceforge.net/p/zsh/code/ci/d8d9fee137a5aa2cf9bf8314b06895bfc2a05518/
> [2] http://www.zsh.org/mla/workers/2018/msg00091.html
> [3] https://gfycat.com/DecimalAdmiredJumpingbean
> 

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Guillaume Chazarain]

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

On Mon, Aug 13, 2018 at 10:43 AM Peter Stephenson <p.stephenson@samsung.com>
wrote:

> The problem with this is all terminals are a bit different in generally
> undetectable ways


Is there a graphical terminal emulator that works on Linux and that does
not exhibit the multi-line issue
<https://gfycat.com/DecimalAdmiredJumpingbean>?
I could reproduce the issue on gnome-terminal, xterm, rxvt.
And I could also reproduce the issue that the commit fixed:
http://www.zsh.org/mla/workers/2018/msg00093.html
So I see a consistent behavior across these 3 terminals.

-- 
Guillaume

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Peter Stephenson]

On Mon, 13 Aug 2018 13:22:31 +0200
Guillaume Chazarain <guichaz@gmail.com> wrote:
> On Mon, Aug 13, 2018 at 10:43 AM Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> 
> > The problem with this is all terminals are a bit different in
> > generally undetectable ways
> 
> Is there a graphical terminal emulator that works on Linux and that
> does not exhibit the multi-line issue
> <https://gfycat.com/DecimalAdmiredJumpingbean>?
> I could reproduce the issue on gnome-terminal, xterm, rxvt.
> And I could also reproduce the issue that the commit fixed:
> http://www.zsh.org/mla/workers/2018/msg00093.html
> So I see a consistent behavior across these 3 terminals.

So you're saying the commit you say is causing the problem isn't
fixing the problem it was supposed to fix?  That implies something
else changed, since dana tried it out at the time and said it worked.

I will certainly not be jumping into this myself now --- does someone
else want to look?

pws

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Guillaume Chazarain]

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

On Mon, Aug 13, 2018 at 1:36 PM Peter Stephenson <p.stephenson@samsung.com>
wrote:

> So you're saying the commit you say is causing the problem isn't
> fixing the problem it was supposed to fix?  That implies something
> else changed, since dana tried it out at the time and said it worked.
>

Sorry, I was unclear.
The commit is indeed fixing the problem it was supposed to fix.
I meant that on all terminals I tested, I saw consistent behaviour:
the commit fixes the original problem but introduces a new one (mine).
So it looks like it should be possible to fix both problems.

-- 
Guillaume

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Peter Stephenson]

On Mon, 13 Aug 2018 13:42:50 +0200
Guillaume Chazarain <guichaz@gmail.com> wrote:
> On Mon, Aug 13, 2018 at 1:36 PM Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> 
> > So you're saying the commit you say is causing the problem isn't
> > fixing the problem it was supposed to fix?  That implies something
> > else changed, since dana tried it out at the time and said it
> > worked. 
> 
> Sorry, I was unclear.
> The commit is indeed fixing the problem it was supposed to fix.
> I meant that on all terminals I tested, I saw consistent behaviour:
> the commit fixes the original problem but introduces a new one (mine).
> So it looks like it should be possible to fix both problems.

Thaks for that.

I suspect dana was saying we need something like this?  Has this got
anything to do with it?  (Second guessing other people's fixes is really
hard work...)

pws

diff --git a/Src/prompt.c b/Src/prompt.c
index 959ed8e..758d5f2 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -1170,7 +1170,7 @@ countprompt(char *str, int *wp, int *hp, int overf)
      * This isn't easy to handle generally; just assume there's no
      * output.
      */
-    if(w >= zterm_columns && overf >= 0) {
+    if(w > zterm_columns && overf >= 0) {
 	if (!overf || w > zterm_columns) {
 	    w = 0;
 	    h++;

Re: "off by one fix in multiple prompts" breaks multiline prompt

[dana]

On 13 Aug 2018, at 07:58, Peter Stephenson <p.stephenson@samsung.com> wrote:
>I suspect dana was saying we need something like this?  Has this got
>anything to do with it?  (Second guessing other people's fixes is really
>hard work...)

Yeah, i probably should have tested the patch that was actually submitted,
sorry. We were kind of working on it in parallel after Wapire suggested on IRC
that that might be a fix to their problem.

The additional change you mentioned addresses the difference between the patch
as submitted and what i was testing. However, i just tried it and it does not
fix this issue (where the last 'hard' line of the prompt wraps).

Looking at that function again, i wonder if the original problem was actually
related to the fact that it compares the current column count (and
potentially increments the height) before checking whether the current character
is a new-line? That would explain why it was producing an extra cup when it
encountered a line that was exactly as wide as the screen.

dana

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Guillaume Chazarain]

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

On Mon, Aug 13, 2018 at 4:19 PM dana <dana@dana.is> wrote:

> The additional change you mentioned addresses the difference between the
> patch
> as submitted and what i was testing. However, i just tried it and it does
> not
> fix this issue (where the last 'hard' line of the prompt wraps).
>

Indeed, it does not fix the problem.

-- 
Guillaume

Re: "off by one fix in multiple prompts" breaks multiline prompt

[dana]

On 13 Aug 2018, at 09:19, dana <dana@dana.is> wrote:
>Looking at that function again, i wonder if the original problem was actually
>related to the fact that it compares the current column count (and
>potentially increments the height) before checking whether the current character
>is a new-line?

I think that was in fact the problem. So if we revert the previous change, and
then add an additional check for a last-column new-line, we end up with
something like the attached. This seems to fix both the original issue and the
new one for me, but obviously further testing would be nice

dana


diff --git a/Src/prompt.c b/Src/prompt.c
index 959ed8e3d..1a3765407 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -1074,10 +1074,10 @@ putstr(int d)
 mod_export void
 countprompt(char *str, int *wp, int *hp, int overf)
 {
-    int w = 0, h = 1;
+    int w = 0, h = 1, multi = 0;
     int s = 1;
 #ifdef MULTIBYTE_SUPPORT
-    int wcw, multi = 0;
+    int wcw;
     char inchar;
     mbstate_t mbs;
     wchar_t wc;
@@ -1086,7 +1086,12 @@ countprompt(char *str, int *wp, int *hp, int overf)
 #endif
 
     for (; *str; str++) {
-	if (w > zterm_columns && overf >= 0) {
+	/*
+	 * Avoid double-incrementing the height when there's a newline in the
+	 * prompt and the line it terminates takes up exactly the width of the
+	 * terminal
+	 */
+	if (w >= zterm_columns && overf >= 0 && !multi && *str != '\n') {
 	    w = 0;
 	    h++;
 	}

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Bart Schaefer]

On Mon, Aug 13, 2018 at 2:29 PM, dana <dana@dana.is> wrote:
>
> I think that was in fact the problem. So if we revert the previous change, and
> then add an additional check for a last-column new-line, we end up with
> something like the attached. This seems to fix both the original issue and the
> new one for me, but obviously further testing would be nice

There are terminals that immediately scroll upon a newline being
written after the last column, especially if it is the last column of
the last line (which is where the prompt most often is).  So sometimes
a double-increment may be necessary, and its hard to tell when.

This is why for years we tried to limit everything to 1 column less
than the exact width.

Presumably ZLE_RPROMPT_INDENT=0 helps in that case?

Re: "off by one fix in multiple prompts" breaks multiline prompt

[dana]

On 13 Aug 2018, at 16:45, Bart Schaefer <schaefer@brasslantern.com> wrote:
>This is why for years we tried to limit everything to 1 column less
>than the exact width.

Oh, yeah :/

On 13 Aug 2018, at 16:45, Bart Schaefer <schaefer@brasslantern.com> wrote:
>Presumably ZLE_RPROMPT_INDENT=0 helps in that case?

As far as i can tell, in Apple Terminal...

... when the check is > (as it is in the repo now), setting it to 0 causes the
editor to get lost as soon as i go past the last column. I think that's the same
behaviour Warepire found (can't remember in which terminal, but not Apple's)
when testing that setting at your suggestion before.

... when the check is >= (as it was before the previous change), setting it to 0
makes no difference. In the case of the last line wrapping, it works as expected
either way, and in the case of a new-line after the last column, the prompt
swallows the preceding line of input either way.

The change from >= to > seems bogus to me now — it only works by accident in
that one case. As far as the new-line check, i'm not sure how to determine if
that's a good direction, besides just trying it in different terminals. I can do
that later if it's not completely disproven already.

dana

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Peter Stephenson]

On Mon, 13 Aug 2018 17:44:32 -0500
dana <dana@dana.is> wrote:
> The change from >= to > seems bogus to me now — it only works by
> accident in that one case. As far as the new-line check, i'm not sure
> how to determine if that's a good direction, besides just trying it
> in different terminals. I can do that later if it's not completely
> disproven already.

Thanks very much --- this certainly sounds like a step in the right
direction, so I've committed it and we'll see if anyone turns up any
further problems we can actually deal with.

pws

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Guillaume Chazarain]

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

On Mon, Aug 13, 2018 at 11:29 PM dana <dana@dana.is> wrote:

> further testing would be nice
>

Tested that it fixes both issues in gnome-terminal, xterm, rxvt.
Thank you so much!

-- 
Guillaume

Re: "off by one fix in multiple prompts" breaks multiline prompt

[Peter Stephenson]

On Mon, 13 Aug 2018 12:36:39 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Mon, 13 Aug 2018 13:22:31 +0200
> Guillaume Chazarain <guichaz@gmail.com> wrote:
> > On Mon, Aug 13, 2018 at 10:43 AM Peter Stephenson
> > <p.stephenson@samsung.com> wrote:
> >   
> > > The problem with this is all terminals are a bit different in
> > > generally undetectable ways  
> > 
> > Is there a graphical terminal emulator that works on Linux and that
> > does not exhibit the multi-line issue
> > <https://gfycat.com/DecimalAdmiredJumpingbean>?
> > I could reproduce the issue on gnome-terminal, xterm, rxvt.
> > And I could also reproduce the issue that the commit fixed:
> > http://www.zsh.org/mla/workers/2018/msg00093.html
> > So I see a consistent behavior across these 3 terminals.  
> 
> So you're saying the commit you say is causing the problem isn't
> fixing the problem it was supposed to fix?  That implies something
> else changed, since dana tried it out at the time and said it worked.
> 
> I will certainly not be jumping into this myself now --- does someone
> else want to look?

or is this comment from dana about the original patch relevant?

> PS: When i was testing it i had changed the comparison in both places;
> as mentioned, the patch as submitted only changes the first one.

pws