kitty breaks soft-wrap on vertical cursor motion

[ridiculousfish]

(This issue was first reported against kitty, then that got reported to fish shell, I am bringing it back to kitty with a reduced test case.)

"Soft wrap" refers to the behavior where lines which wrapped because they exceeded the width of the terminal, may reflow and "unwrap" when the terminal's width is increased. This follows the behavior of text editors.

Kitty supports soft wrap, but unlike other terminals the soft-wrap gets "broken" when a newline is emitted on a previously wrapped line. (Note that newline is the typical value of cursor_down in ncurses.)

To reproduce:

1. Compile the following C code (e.g. gcc test.c). This code emits a line of 600 Xs, which will soft wrap. It then moves the cursor into and out of those lines.
2. Run it (./a.out) in a terminal whose width is around 100 columns
3. Increase the width of the terminal.

Observed behavior: the lines of Xs do not reflow.
Expected behavior: the lines of Xs reflow, as they do in other terminals (tested with Terminal.app, iTerm2, alacritty)

Comment out the "Cursor motion" region and the soft-wrap is restored.

C code follows:

#include <stdio.h>

int main(int argc, char **argv) {
  // Emit 600 Xs.
  for (int i = 0; i < 600; i++)
    printf("X");

  // Cursor motion: go to beginning of line, then up 6, then down 6.
  printf("\r");
  for (int i = 0; i < 6; i++)
    printf("\033[A");
  for (int i = 0; i < 6; i++)
    printf("\n");

  return 0;
}

Environment details

Tested with kitty master (fefafda) run as kitty -c NONE

kitty 0.26.5 (fefafda9a0) created by Kovid Goyal
Darwin stars-to-fill-the-skies.local 22.1.0 Darwin Kernel Version 22.1.0: Sun Oct  9 20:15:09 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T6000 arm64
ProductName:		macOS ProductVersion:		13.0 BuildVersion:		22A380
Frozen: False
Paths:
  kitty: /Users/peter/github/kitty/kitty/kitty/launcher/kitty.app/Contents/MacOS/kitty
  base dir: /Users/peter/github/kitty/kitty
  extensions dir: /Users/peter/github/kitty/kitty/kitty
  system shell: /usr/local/bin/fish

Config options different from defaults:

Important environment variables seen by the kitty process:
	PATH                                /Users/peter/github/kitty/kitty/kitty/launcher/kitty.app/Contents/MacOS:/Users/peter/github/kitty/env/bin:/Users/peter/.poetry/bin:/opt/homebrew/opt/ruby/bin:/opt/homebrew/lib/ruby/gems/3.0.0/bin:/opt/homebrew/bin:/Users/peter/.cargo/bin:/Users/peter/.fzf/bin:/Users/peter/bin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Library/Apple/usr/bin:/Applications/Little Snitch.app/Contents/Components:/Applications/VMware Fusion Tech Preview.app/Contents/Public:/usr/local/share/dotnet:/Users/peter/.dotnet/tools:/usr/local/go/bin:/Library/Frameworks/Mono.framework/Versions/Current/Commands
	LANG                                en_US.UTF-8
	EDITOR                              nvim
	SHELL                               /usr/local/bin/fish
	USER                                peter
	LC_TERMINAL                         iTerm2
	LC_TERMINAL_VERSION                 3.4.12

[kovidgoyal]

\n is not move the cursor down. \n is newline. If you want to move the
cursor down without inserting newlines, you use the "cud" capability
from terminfo which in kitty and every other terminal I know, is CSI
B. In your case it means emitting <ESC>[B instead of
\n to move the cursor down one line.

A newline is emphatically not the same as cursor down. cursor down does
not cause scrolling even if the cursor is on the last line of the
screen, newline does. fish should not be using newline for cursor down.

It is very unfortunate that these other terminals appear to change the
meaning of \n depending on whether there is text after the current
cursor position or not. IMO it is extremely bad design to do that. It
means that you can never know what a newline will do without also
knowing the contents of the screen. In kitty \n unconditionally marks
the current line as being hard wrapped, regardless of where the cursor
is on the line and what is after the cursor.

As such, I am not inclined to copy this bad design in kitty. fish should
use the cursor movement escape codes to move the cursor
See for example, the xterm reference:
https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Functions-using-CSI-_-ordered-by-the-final-character_s_
(search for CUD, defined as CSI Ps B)

I am closing this issue, but feel free to post if you have further
concerns.

[page-down]

... change the meaning of \n depending on whether there is text after the current cursor position or not ...
... never know what a newline will do without also knowing the contents of the screen ...

I've also noticed multiple feedbacks here about copying fish commands (selecting text with the mouse) in kitty being inserted with extra \n newlines.
This seems to be the same problem.

If I remember correctly, kitty will copy the text content as is, without doing some magic to the line breaks at the edge of the screen.

@ridiculousfish

I personally think a terminal with fully predictable behavior would be better, for cli program developers.
Do you think there is a proper way to solve this problem in fish?

[ridiculousfish]

@kovidgoyal I would like to share some context which you may have missed, in hopes you will reconsider. First off: I very much agree that newline is a silly choice for cursor_down. The ANSI escape sequence you mention makes way more sense.

fish does not actively choose to emit newlines: rather newline is the sequence published by the terminfo database for cursor_down. fish wants to respect the terminfo database, to avoid emitting unrecognized escape sequences, and in this case cursor_down maps to newline.

For those following along at home, one can confirm this by running terminfo and looking for cud1. On my Linux box, with $TERM as xterm-256color, it reports cud1=\n. On my Mac it reports cud1=^J which is also newline.

This doesn't just affect fish. Any app which uses ncurses or even terminfo may be affected, by simply doing what the terminfo says.

It's a dumb state of affairs, but we are all stuck with it, and I am politely requesting you to grit your teeth and implement newline as cursor_down similar to other terminals. The alternative for fish is to detect kitty as the terminal and specially implement a workaround by using a different sequence.

[kovidgoyal]

Lookup cud not cud1 and you will be fine.

[page-down]

I noticed in the fish issue faho replied.

https://github.com/fish-shell/fish-shell/issues/9409#issuecomment-1355006398

... the solution the kitty maintainer recommends ("cud" instead of "cud1") doesn't work.
It won't push down the cursor if it is at the end of the screen. ...

So I don't believe this is fixable on our end. ... If someone finds a better solution we'll try it.

@kovidgoyal
Also you have already mentioned that cud will not scroll, I tried to implement an example to explore how to solve this problem.

... cursor down does not cause scrolling even if the cursor is on the last line of the screen, newline does.

• kitty wants fish to re-render the current prompt when the window size changes (fish_handle_reflow 1).
• Use \n to add new lines to be rendered.
• Use cud to move the cursor.
• Re-print the prompt in a single line before executing the command.

#!/bin/bash
  
# Print some lines to fill the window
echo prev-1; echo prev-2; echo prev-3; echo prev-4;
# Since cud doesn't scroll on the last line, use `\n` line breaks for two lines first.
echo -en "line-1\nline-2\r\x1b[A";                                                                        
# Then calculate and "reflow" the prompt based on the window width (here use a fixed width of 10 cells).
echo -en "\x1b[K0123456789"; echo -en "\r\x1b[B"; echo -en "\x1b[Kabc";      
# Press Enter to continue
read; echo;

# Since only the current (last) prompt is rendered by the shell,
# the previous prompt with the command needs to be reprinted at this point in a single line.
# Later when the window size changes, they will be wrapped by kitty.   
echo -en "\r\x1b[K\x1b[A\x1b[K"; echo "0123456789abc";                                                          
# Print the next prompt
echo -en "line-1\nline-2\r\x1b[A"; echo -en "\x1b[Knextprompt"; echo -en "\r\x1b[B"; echo -en "\x1b[K123";            
# Trying to move the cursor should not affect line wrapping.
echo -en "\x1b[4A\x1b[4B";
# Try changing the window size at this point and the first prompt output will soft-wrap normally.
# The second one is unchanged because it has not been redrawn in this example.
read;

kitty --config=NONE \
-o remember_window_size=no \
-o initial_window_width=10c \
-o initial_window_height=5c \
bash /path/to/example.sh

However, even with cud, selecting and copying the last line in kitty with the mouse will still have \n hard line breaks.
Expected: nextprompt123, Actual: nextprompt\n123
Do you have any suggestions?

Maybe I'm missing something and the above approach doesn't seem so practicable.

[kovidgoyal]

I dont follow. Why does fish need to move the cursor down while redrawing the prompt? It just starts at the top and prints out everything, there should be no need for cursor movement at all. Witness the fact that no other shell has this issue. Once the prompt is fully printed out, fish moves the cursor back to its required position, this is where cursor movement comes in, and here there is no need for newlines.

[ridiculousfish]

zsh is unaffected because they deliberately check for the case that down_cursor is newline and try to avoid using it, instead preferring the multi-byte parm_down_cursor sequence. This is precisely the workaround I hope to avoid implementing in fish: the sequence is longer so less efficient, plus shells should not be in the business of second-guessing termcap.

Nevertheless zsh exhibits the same bug when run in TERMs without parm_down_cursor. To reproduce in zsh:

1. TERM=linux zsh and ensure your window is narrow.

2. Paste in so it appears as a single multi-line command, which soft-wraps:

    echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    echo bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    echo cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
   

3. Use the arrow keys to move into each line in turn.

4. Hit enter to execute

5. Resize the window horizontally. Notice how the original command does not reflow in kitty; it does in other terminals.

If down_cursor is unreliable, then every termcap user will need to implement this same workaround, and as shown above the workaround is imperfect.

[kovidgoyal]

You dont need a workaround, simply use the terminfo cud property instead of cud1. If it doesn't exist fall back to cud1. It's two bytes longer, I seriously doubt that is going to cause anyone any issues. And as zsh shows you dont need down cursor to scroll lines.

Quoting from the terminfo man page

cursor_down                 cud1      do     down one line
If  there is a code to move the cursor one position to the left (such as backspace) that capability should be given as cub1.  Similarly,
       codes to move to the right, up, and down should be given as cuf1, cuu1, and cud1.  These local cursor motions should not alter the  text
       they pass over, for example, you would not normally use “cuf1= ” because the space would erase the character moved over.

       A very important point here is that the local cursor motions encoded in terminfo are undefined at the left and top edges of a CRT termi‐
       nal.  Programs should never attempt to backspace around the left edge, unless bw is given, and never attempt to go up  locally  off  the
       top.  In order to scroll text up, a program will go to the bottom left corner of the screen and send the ind (index) string.

You are relying on an undocumented side effect of cud1. If you need to scroll lines the correct terminfo parameter to use is ind not cud1. In fact according to the man page cud1 must not alter the text. So using newline for cud1 is actually a bug. I might change kitty's terminfo to not use newline for cud1. Since in kitty newline does affect the text, by adding a newline to it.

[kovidgoyal]

I have changed kitty's terminfo to use \EB for cud1. If fish relies on cud1 scrolling lines, it needs to stop doing that.

[kovidgoyal]

Actually I am going to revert the commit to change cud1 in kitty's terminfo. Given the current buggy use of cud1 in fish doing that fixes this issue but has the problem that fish cant render multiline text if the prompt is at the bottom of the screen, which is a much bigger issue. So I am afraid this can only be fixed in fish.

[kovidgoyal]

And just for completeness, using kitty --dump-commands this is how fish draws multi-line text at a prompt:

Typing the following: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab
c
Where c ends up on the next line. Starting from after pressing b:

screen_carriage_return
screen_cursor_forward 72
select_graphic_rendition 30 
screen_designate_charset 0 66
select_graphic_rendition 0 
select_graphic_rendition 38 2 255 0 0 
draw b
screen_carriage_return
screen_carriage_return
screen_linefeed
select_graphic_rendition 30 
screen_designate_charset 0 66
select_graphic_rendition 0 
screen_cursor_up2 1
screen_cursor_forward 71
select_graphic_rendition 38 2 255 0 0 
draw abc
screen_cursor_up2 1
screen_carriage_return
screen_carriage_return
screen_linefeed
screen_cursor_forward 1
select_graphic_rendition 30 
screen_designate_charset 0 66
select_graphic_rendition 0 

So what it does when it wants to draw a character that it believes will go onto the next line is (leaving out the style changes)

1. Emit two \r
2. Emit \n
3. Emit cursor up 1
4. Emit cursor right 71 (width of screen -1)
5. draw three characters, the last being the one it think will go onto the next line
6. Emit cursor up 1
7. Emit two \r
8. Emit \n
9. Emit cursor right 1

This is pretty weird to begin with but presumably it has good reasons for this complicated dance, who knows. To fix it what you need to do is in step 8 instead of emitting \n emit \e[B to move the cursor down. There is no need to use \n here because you have just moved the cursor up 1 in step 6. Therefore, the screen will never scroll in any case. And while you are about it since you say one of the reasons you dont want to use \e[B is the two extra bytes, you can remove the two redundant \r one in step 1 and one in step 7. That will save you two bytes.

[kovidgoyal]

As a point of comparison, this is how bash (aka readline) draws the same scenario

draw b 
screen_carriage_return
draw c

And this is how zsh draws it

draw b 
screen_carriage_return
screen_erase_in_line 0 0
draw c

[ridiculousfish]

fish has complicated logic for rendering because it supports features like autosuggestions, syntax highlighting, right prompt, etc, all of which require fish to calculate the actual terminal-width-aware rendering in a way that other shells do not. fish does emit excessive cursor positioning sequences; these are (often desperate) attempts to work around inconsistencies between terminals, which are a challenge for a shell to deal with.

[kovidgoyal]

On Thu, Dec 22, 2022 at 11:09:42PM -0800, Peter Ammon wrote: fish has complicated logic for rendering because it supports features like autosuggestions, syntax highlighting, right prompt, etc, all of which require fish to calculate the actual terminal-width-aware rendering in a way that other shells do not. fish does emit excessive cursor positioning sequences; these are (often desperate) attempts to work around inconsistencies between different terminal apps.

zsh has all those as well, just FYI. From looking at the traces, its quite obvious that what fish is doing is calculating when it expects the cursor to move to the next line, and when it thinks it has, it deliberately moves the cursor up, and then emits \r\r\n to move it down again. I am very curious to know what terminal bug this algorithm is circumventing. And my contention is that in this part of your rendering algorithm if you use \e[B instead of \n it will break nothing, since you arent actually expecting a scroll, and will fix this bug.

[ridiculousfish]

Sure. Our biggest challenge is calculating the width of rendered text, especially ambiguous-width characters and emoji. Terminals render these with inconsistent widths, sometimes even providing user settings to change how wide they are.

Here is an example using kitty. I paste in Hammer and Wrench (U+1F6E0) with variation selector 15 (U+FE0E), first with zsh and then fish.

zsh only renders one character, despite multiple pastes, and then allows deleting into the prompt, which it should never do. And when I hit enter it runs a command which was not visible on the command line. It does this because it does not conservately reposition the cursor; it has lost track of where the cursor is.

fish renders it fine without these problems. This is because, while fish also miscalculates the width (this one is really hard), it emits cursor movement sequences to defend against miscalculation so it doesn't get lost. That explains the strange and excessive cursor positioning escape sequences you identified. They're not for fun, they solve a real problem.

Screen.Recording.2022-12-23.at.12.04.39.AM.mov

[kovidgoyal]

Sure, but what does this have to do with using cursor down after cursor
up? As I said originally, there are probably good reasons for the dance
fish does. I am not suggesting you change your entire rendering
pipeline.

The change I proposed above, of using cursor down instead of \n when
it is immediately following a cursor up will fix this bug and not break
anything else. It is making your dance a little more robust.

[mqudsi]

Thanks, @kovidgoyal.