Various screen redraw fixes for wide characters, narrow screens etc. #202
Conversation
|
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! |
Rebase on master Cherry-pick chzyer/readline#202
|
@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. |
|
Thanks @tpodowd , So many fixes in your PR, I will try them one by one. |
|
@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 |
|
Yeap i've been using fq daily with the patches on macOS for 2 weeks or so and haven't noticed any issues |
|
Note: I rebased the screenredraw branch against the latest master and did a force push. No other changes. |
|
This change appears to introduce a number of data races: https://gist.github.com/slingamn/fb0ed14d6a5471b83fce276d11f72856 |
|
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. |
|
Thanks! Sorry, the full test case I was using is in the issue description of #217, in case that helps. |
|
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. |
- 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
force-pushed
the
screenredraw
branch
from
ea50c44
to
586d8ee
Compare
|
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. |
|
Thanks! I tested this branch against the test case from #217. I could not reproduce the data race anymore. On the other hand:
I'm uncertain about the cause, but maybe it's the |
|
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: ie, the prompt "> " followed by "received a\n" If you press ctrl-a or something like that, the prompt will redraw as follows 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. |
As I understand the API here, it explicitly allows asynchronously writes via Asynchronous writes are part of my core use case for this library (a rudimentary IRC client).
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). |
|
Cool. Thanks for the feedback. I'll see what I can do. |
Introduce a new operation.isPrompting field which is true from when a prompt has been written until data is returned to the caller. When Write is called on the wrapWriter to write to stdout or stderr, check if we are currently prompting the user for input and if so clean up the prompt and write the data before redrawing the prompt at its new location after the written data. Previously terminal.IsReading() was used for this, but this had various race conditions and it was not correct to check this field to make prompt and buffer redrawing decisions. In turn, I removed all the isReading code also. The old isReading() check was actually checking if the terminal goroutine was actively waiting for more input.
|
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 #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 thanks so much! I'm still testing, but I rebased my patchset https://github.com/slingamn/readline/commits/ircdog on your branch, and:
|
| // Query cursor position before printing the prompt as there | ||
| // maybe existing text on the same line that ideally we don't | ||
| // want to overwrite and cause prompt to jump left. Note that | ||
| // this is not perfect but works the majority of the time. | ||
| o.buf.getAndSetOffset(o.t) |
There was a problem hiding this comment.
This aspect of the change seems to cause a race condition in my application during asynchronous Close(), where sometimes the response to this DSR query is not read correctly by the library and ends up printed on the screen. It looks like ^[[70;1R or similar.
I'll look further into mitigations from my side, just wanted to note this so I don't forget about it.
There was a problem hiding this comment.
This terminal will be asked for the current offset everytime you call Readline() to ask for text. Normally, since you call readline, input will be taking place so bytes will be read and readline will swallow this offset information from the terminal. However, if you call readline() and then close() in other goroutine (or exit the program abruptly) for example, it can happen that readline won't have time to read it and instead the program will exit and the next thing to read it will be your shell which will just output it. It doesn't happen in my use case because I'm not making exit decisions when readline is waiting for text. However, I can see in your example test case, it might have that issue.
It might be possible for the library to handle this better. Maybe in another PR if this is merged 😉
|
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: |
|
@Lekensteyn you might want to look at https://github.com/ergochat/readline which i think has these changes plus a bunch of more things |
for lineedge and reduce the control sequences to 2.
and avoid overwriting existing text as well as simplifying the
control sequences used.