readline bug

[Martin Bähr]

hi,

after being annoyed by this for a while i finally managed to track down
the following issue:

try:
void main()
{
  object rl=Stdio.Readline();
  rl->write("first");
  rl->write("second");
  rl->newline();
}

notice how the second clobbers the first?
this is particularely annoying in hilfe where i see it often.

the problem is in the readline write function which first resets the
cursor to the beginning of the line.

this can be fixed in two possible ways:
force a newline if the cursor is not in column 0,
or don't reset the cursor.

i prefer adding the newline.

the one thing i am concerned about is that someone might expect this
behaviour by now and a compatibility thing is needed.

thoughts?
any other reason not to fix this now?

patches attached.

greetings, martin.

---

>From 5d13d8e85eaf34a37c936718fa3a1af79afcccb6 Mon Sep 17 00:00:00 2001
From: Martin Baehr <mbaehr@olpc>
Date: Sun, 20 Jul 2008 00:05:45 +0800
Subject: [PATCH] don't clobber previous output on same line, but write in next line

---
 lib/modules/Stdio.pmod/Readline.pike |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/lib/modules/Stdio.pmod/Readline.pike b/lib/modules/Stdio.pmod/Readline.pike
index a48ec70..617891b 100644
--- a/lib/modules/Stdio.pmod/Readline.pike
+++ b/lib/modules/Stdio.pmod/Readline.pike
@@ -159,6 +159,12 @@ class OutputController
     return columns;
   }
 
+  //! Returns the cursorposition.
+  int getcursorpos()
+  {
+    return xpos;
+  }
+
   protected string escapify(string s, void|int hide)
   {
 #if 1
@@ -1543,6 +1549,8 @@ void message(string msg)
 //!   Document this function
 void write(string msg,void|int word_wrap)
 {
+  if (output_controller->getcursorpos())
+    output_controller->newline();
   int p = cursorpos;
   setcursorpos(0);
   if(!input_controller->dumb) {
--
1.5.3.3

---

---

>From d6fbf34f1553fbf1ffcf7d3c105df0e92bc7a5cd Mon Sep 17 00:00:00 2001
From: Martin Baehr <mbaehr@olpc>
Date: Sun, 20 Jul 2008 00:11:34 +0800
Subject: [PATCH] don't clobber previous output on same line, but write after it

---
 lib/modules/Stdio.pmod/Readline.pike |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/lib/modules/Stdio.pmod/Readline.pike b/lib/modules/Stdio.pmod/Readline.pike
index a48ec70..045049d 100644
--- a/lib/modules/Stdio.pmod/Readline.pike
+++ b/lib/modules/Stdio.pmod/Readline.pike
@@ -1544,11 +1544,6 @@ void message(string msg)
 void write(string msg,void|int word_wrap)
 {
   int p = cursorpos;
-  setcursorpos(0);
-  if(!input_controller->dumb) {
-    output_controller->bol();
-    output_controller->clear(1);
-  }
   array(string) tmp=msg/"\n";
   foreach(tmp[..<1],string l)
   {
--
1.5.3.3

---

[Martin Bähr]

hi,

i'd like to commit the patch in 16650521

please select any that apply:
( ) it's a feature, not a bug (don't commit anything)
( ) wait for 7.9
( ) commit to 7.8 (after release)
( ) commit to 7.7

[ ] add a compatibility workaround for older versions.
[ ] commit 16650522 instead
[ ] <your comment here>

greetings, martin.

I'm not a big consumer of readline, but wouldn't compatibility glue
be needed?

[Martin Bähr]

On Wed, Jul 23, 2008 at 08:40:02AM +0000, Peter Bortas @ Pike  developers forum wrote:
> I'm not a big consumer of readline, but wouldn't compatibility glue
> be needed?

actually, thinking about it some more, i think not.

there are two situations:
either the last line is overwritten, (like in hilfe) which is clearly
wrong, and applying one of the patches fixes that.

or the application using readline already contains a workaround which would
normally be to to add an extra newline before the app allows readline to
continue in which case the buxfix will be a noop since it only changes
behaviour if there is no newline at the end od the last line.

so this means:
applications affected by the bug will be fixed, and
applications containing a workaround will not notice any difference.
hence, compatibility glue is not needed.

greetings, martin.

Fair enough. Commit.

[Martin Bähr]

thanks,

remains the question, which one of the two fixes is better.

arguably the added newline produces nicer output, supporting the current
expectation that each readline->write starts at the beginning of the
line, but writing something without a newline in the end is usually
intentional, so adding a newline would break that.

currently it is not possible to write something without a newline
because it would be overwritten, so this is only talking about future
applications.

consider hilfe: with a newline the results of this:
> write("foo");
foo
(1) Result: 3
> write("foo\n");
foo
(1) Result: 4
would be indistinguishable (except for the value write returns)

with removing the clear it would instead be:
> write("foo");
foo(1) Result: 3

this may be considered ugly but would somehow be more correct.

note that once a choice for either patch is made, it can't be changed
without introducing compatibility issues.

greetings, martin.

I'd say the latter is better.

aol

[Martin Bähr]

while i have been leaning towards adding the newline, i just came tothe
conclusion that removing the clear is better because if you don't like
the ugly output you can always force a newline (such code could be added
to hilfe for example), but if you don't like the added newline, then
there is no option to go back.

hence removing the clear is clearly ;-) the better option.
i'll commit 16650522 tomorrow (it is past 3 am, need sleep)

greetings, martin.

[Dan Nelson]

In the last episode (Jul 23), Martin Baehr said:

> arguably the added newline produces nicer output, supporting the
> current expectation that each readline->write starts at the beginning
> of the line, but writing something without a newline in the end is
> usually intentional, so adding a newline would break that.
>
> currently it is not possible to write something without a newline
> because it would be overwritten, so this is only talking about future
> applications.
>
> consider hilfe: with a newline the results of this:
> > write("foo");
> foo
> (1) Result: 3
> > write("foo\n");
> foo
> (1) Result: 4
> would be indistinguishable (except for the value write returns)
>
> with removing the clear it would instead be:
> > write("foo");
> foo(1) Result: 3
>
> this may be considered ugly but would somehow be more correct.

A few years ago zsh added a nice feature where it sends the following
string just after a command exits and before printing its prompt:

<bold><standout>%<endbold><endstandout><COLS-1 spaces><CR>

If the previous command printed a partial final line, you would end up
seeing a bold inverse % at the end of the output, and the shell prompt
on the next line.  If the command did exit with a newline as the last
output character, the % gets overwritten with the shell prompt so you
don't see it.

http://zsh.cvs.sourceforge.net/zsh/zsh/Src/utils.c?revision=HEAD&view=markup#l_1179

--
        Dan Nelson
        dnelson@...