Various screen redraw fixes for wide characters, narrow screens etc.

[tpodowd]

• Don't overwrite existing text on same line as the prompt
• Don't refresh screen when simply appending characters to buffer
• Don't refresh screen unnessarily when pressing enter key
• Handle prompts longer than screen width.
• Fix wide characters in prompt
• Fix screen edge issue when next character is wide.
• Fix screen edge issue for masked characters
• Fix narrow masked characteter, masking wide input
• Fix wide masked character, masking narrow input
• Reworked backspacesequence for index to use same algorithm as used for lineedge and reduce the control sequences to 2.
• Reworked cleanup to incorporate initial cursor column position and avoid overwriting existing text as well as simplifying the control sequences used.
• Fixed double width character detection and updated unit tests
• Handle emoji in text or prompts.
• Implement windows ANSI absolute horizonal position ansi code.
• Get windows cursor position directly and don't send ansi DSR code
• Don't write out empty mask runes
• Cleanup - removed unused hadCLean variable

[tpodowd]

Hi @chzyer,

I noticed you became active again. Please have a look at this and see if you disagree with anything. I have tested the fixes on linux and windows 11 (used MS developer VM). I ran all the demo programs to check. I tried them with narrow and wide window sizes (although windows doesn't have code for window resize detection). Looking at the list of existing pull requests, I believe this pull request probably fixes some of the reported issues also. In particular, this PR replaces PR 171 (which I used to use but it was flawed).

Thanks!

[wader]

@tpodowd Hey! great stuff, took your changes for a spin and experience lots of less redraws 👍 I try to maintain a fork of readline at https://github.com/wader/readline/tree/fq with various cherry-picked stuff and my own changes. Will let you know if i run into something strange.

Would be great if various forks of chzyer/readline could be merged into one somewhere for less conflicts and work. Best of course would be to get things into @chzyer repo if he agrees.

[tpodowd]

Thanks for trying out the PR @wader . Good to hear that it works for you so far. Do let me know if you find any issues. I'll also push back anything I find here but so far it is looking solid. @chzyer did a great job on this readline library so yes, it would be good to keep it all here.

[chzyer]

Thanks @tpodowd , So many fixes in your PR, I will try them one by one.

[wader]

@chzyer Great to see you active again and thanks a lot for the readline package, works very well and have been a joy to use

[tpodowd]

Hi @chzyer - sorry the PR is a bit large. I tried to keep it as focused as I could. I've been using the changes on linux for a while now and haven't noticed any issues so far. I've been talking to @wader separately and it seems they are using it successfully also on Windows which is promising :-)

[wader]

Yeap i've been using fq daily with the patches on macOS for 2 weeks or so and haven't noticed any issues

[tpodowd]

Note: I rebased the screenredraw branch against the latest master and did a force push. No other changes.

[slingamn]

This change appears to introduce a number of data races: https://gist.github.com/slingamn/fb0ed14d6a5471b83fce276d11f72856

[tpodowd]

Hi @slingamn - Thanks for reporting that. I'm a little swamped right now, but I'll try look at that this week. I was able to replicate by running.

go run -race readline-demo.go
» ==================
WARNING: DATA RACE
Read at 0x00c000090258 by goroutine 13:
  github.com/chzyer/readline.(*RuneBuffer).getSplitByLine()
      /home/tpodowd/work/readline/runebuf.go:449 +0x3c4
  github.com/chzyer/readline.(*RuneBuffer).isInLineEdge()
      /home/tpodowd/work/readline/runebuf.go:439 +0x8c
  github.com/chzyer/readline.(*RuneBuffer).append()
      /home/tpodowd/work/readline/runebuf.go:558 +0x20f
  github.com/chzyer/readline.(*RuneBuffer).WriteRunes()
      /home/tpodowd/work/readline/runebuf.go:175 +0x276
  github.com/chzyer/readline.(*RuneBuffer).WriteRune()
      /home/tpodowd/work/readline/runebuf.go:162 +0xa5a
  github.com/chzyer/readline.(*Operation).ioloop()
      /home/tpodowd/work/readline/operation.go:331 +0x9d1

Previous write at 0x00c000090258 by goroutine 15:
  github.com/chzyer/readline.(*RuneBuffer).setOffset()
      /home/tpodowd/work/readline/runebuf.go:525 +0x117
  github.com/chzyer/readline.(*RuneBuffer).setOffset-fm()
      /home/tpodowd/work/readline/runebuf.go:522 +0x5e
  github.com/chzyer/readline.(*Terminal).GetOffset.func1()
      /home/tpodowd/work/readline/terminal.go:81 +0x83

Goroutine 13 (running) created at:
  github.com/chzyer/readline.NewOperation()
      /home/tpodowd/work/readline/operation.go:88 +0x81a
  github.com/chzyer/readline.(*Terminal).Readline()
      /home/tpodowd/work/readline/terminal.go:95 +0x84
  github.com/chzyer/readline.NewEx()
      /home/tpodowd/work/readline/readline.go:169 +0x5f
  main.main()
      /home/tpodowd/work/readline/example/readline-demo/readline-demo.go:74 +0x270

Goroutine 15 (finished) created at:
  github.com/chzyer/readline.(*Terminal).GetOffset()
      /home/tpodowd/work/readline/terminal.go:80 +0x5a
  github.com/chzyer/readline.(*RuneBuffer).getAndSetOffset()
      /home/tpodowd/work/readline/runebuf.go:513 +0xe4
  github.com/chzyer/readline.(*Operation).Runes()
      /home/tpodowd/work/readline/operation.go:393 +0x1f6
  github.com/chzyer/readline.(*Operation).String()
      /home/tpodowd/work/readline/operation.go:376 +0x56
  github.com/chzyer/readline.(*Instance).Readline()
      /home/tpodowd/work/readline/readline.go:257 +0x2f
  main.main()
      /home/tpodowd/work/readline/example/readline-demo/readline-demo.go:99 +0x8af
==================

[slingamn]

Thanks! Sorry, the full test case I was using is in the issue description of #217, in case that helps.

[slingamn]

Given #198 and #219, I'm not sure it makes sense to "spot-fix" race conditions in this library. A large-scale concurrency redesign might be necessary (for example, putting most operations on internal datastructures behind a single broad mutex, releasing it for I/O operations).

[slingamn]

@tpodowd @wader also interested in your thoughts on #220 if you have time

[tpodowd]

Hi @slingamn - Have not had a lot of time yet. Still high on my list. I did do some review though. the race condition above is a simple fix. There are a few more though so I think I'll just spend a little more time and address them also. It will be next week though as I'm off for a few days.

[tpodowd]

Note: I rebased this PR on the latest master and also added a small commit that fixes the race condition mentioned above that this PR introduced.

I am also going to rebase my other PR208 on top of this branch to include this race condition fix.

[slingamn]

Thanks!

I tested this branch against the test case from #217. I could not reproduce the data race anymore. On the other hand:

1. The issue from #220 is still present
2. Combining the patch from #220 (7bb0e05674) with this branch results in a different redraw glitch, where the prompt can be drawn twice.

> a
received a
> b
received b
> c
received c
> > d
received d
> > e
received e
> f
received f
> g
received g

I'm uncertain about the cause, but maybe it's the operation.go changes in this branch?

[tpodowd]

Hi @slingamn. Just had a look at this. I think the patch to remove the IsReading check is probably not the right fix particularly combined with this PR.

So what is happening is that you have two go routines a and b. a) is calling readline and passing the string to b) before looping again and calling readline again. b) is looping printing the text a) sends it.

Everything works correctly if a) writes the prompt, reads the input and sends the input to b) which then prints the input and then a) writes the prompt again. However, if a) writes the prompt, reads input and sends input to b) and writes the prompt again before b) has time to write the input it received, you get the following:

> a
> received a
NEXTINPUTHERE

ie, the prompt "> " followed by "received a\n" If you simply type the "NEXTINPUTHERE" it goes at the start of the line as the prompt was already output and is now on the previous line.

If you press ctrl-a or something like that, the prompt will redraw as follows

> a
> received a
> NEXTINPUTHERE

The reason this happens is that this PR tries to avoid redrawing the whole line every time a key is pressed and simply appended to the current position.

I don't know if the library can/should fix "> received a" issue as it's simply a race writing to stdout? I can look into my fix for redrawing the prompt every append though which would at least mean that you will always get the prompt in the right place as you type without using ctrl-a or something to initiate the redraw. This would slow down every key press though (but I guess that is the way it is in the current mainline anyway). I'll play with it tomorrow if I have time. Let me know what you think.

[slingamn]

Everything works correctly if a) writes the prompt, reads the input and sends the input to b) which then prints the input and then a) writes the prompt again.

As I understand the API here, it explicitly allows asynchronously writes via (*Instance).Write while the prompt is displayed and input is being read. (Hence why (*wrapWriter).Write explicitly supports redrawing the prompt, by wrapping the write in (*RuneBuffer).Refresh.) The case for 7bb0e05674, i.e. removing the fast path that skips the refresh, is that this inherently involves a TOCTOU race, because IsReading() can become true at any time.

Asynchronous writes are part of my core use case for this library (a rudimentary IRC client).

I don't know if the library can/should fix "> received a" issue as it's simply a race writing to stdout?

It seems to me that it's one of the core functions of this library to draw the prompt correctly. Drawing it incorrectly is not pleasant at a UX level (it makes the program feel inconsistent or unreliable). I don't know what's considered normal in TUI applications (this is my first time getting this close to terminal emulation) but it seems to me that any optimization that sacrifices correctness here isn't worth it. The performance penalty (which, as you point out, is present in the current master branch) doesn't seem to be affecting current consumers of this library (although I have noticed a weird "flicker" on Windows, possibly because of Windows idiosyncrasies).

[tpodowd]

Cool. Thanks for the feedback. I'll see what I can do.

[tpodowd]

Hi @slingamn

I pushed another commit to this PR. I believe that this addresses your issues. I removed the race condition with the prompt redraw. I removed the terminal isReading bool field as it is not reliable. Instead I introduced an operation.isPrompting bool with some locking around it so that it is reliable. This also conflicts with your patch to remove the shortcut https://github.com/chzyer/readline/issues/220 and fixes the prompt rewrite also such that you can now write to rl reliably and the prompt will not race. I also did not have to remove my append redraw optimisation I mentioned earlier as part of this fix so that was nice.

I ran though all the examples in the example directory also and insured that they worked (on linux).

Please try this out and let me know if things are behaving better for you. I'll try to get a windows VM up also to test it but this may take a little time. I also have not tested non-interactive mode yet.

Anyway. give it a go and let me know.

[tpodowd]

Hi @slingamn - I tested this branch also on a windows VM this morning. Looks good to me. I tested all the examples as well as your test program in #217, so I believe I haven't broken anything on windows.

[slingamn]

@tpodowd thanks so much! I'm still testing, but I rebased my patchset https://github.com/slingamn/readline/commits/ircdog on your branch, and:

1. The prompt redraw race is gone
2. The flicker on Windows is considerably mitigated (probably because of your redraw optimization for the append case)
3. #219 appears to be fixed, probably by the additional locking you added in 62ab2cfd17947c6843ad015eb81c0c675ec22630

[Lekensteyn]

I ran into an issue with v1.5.1 where navigating the cursor over a long word-wrapped input line "moves the screen up". As if the library is trying to make sure that the prompt stays on the same line as the current cursor. Reproduced on macOS 14.0 with iTerm 3.4.21, TERM=xterm-256color.

This PR seems to fix that, so unless there are any other issues, I would recommend merging this PR.

In meantime, I am changing my application to point to this PR:

go mod edit -replace github.com/chzyer/[email protected]=github.com/cloudian/readline@screenredraw
go mod tidy

[wader]

@Lekensteyn you might want to look at https://github.com/ergochat/readline which i think has these changes plus a bunch of more things