[PATCH] conhost: Fix wrapping when write_console finishes on line end.
Remember the fact we completed the entire line in cursor_x, but report it as usual in other cases as the last character on the line. When writing a new character, a newline is then written so it wraps properly. This can happen in practice when writing one character at a time to the console; right now it will keep overwriting the last character on the line. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- programs/conhost/conhost.c | 30 ++++++++-------- programs/conhost/conhost.h | 5 +++ programs/conhost/tests/tty.c | 67 ++++++++++++++++++++++++++++++++++++ programs/conhost/window.c | 8 ++--- 4 files changed, 91 insertions(+), 19 deletions(-) diff --git a/programs/conhost/conhost.c b/programs/conhost/conhost.c index fbee37d..e5ec84f 100644 --- a/programs/conhost/conhost.c +++ b/programs/conhost/conhost.c @@ -278,7 +278,7 @@ static void tty_sync( struct console *console ) if (console->active->cursor_visible) { - set_tty_cursor( console, console->active->cursor_x, console->active->cursor_y ); + set_tty_cursor( console, get_bounded_cursor_x(console->active), console->active->cursor_y ); if (!console->tty_cursor_visible) { tty_write( console, "\x1b[?25h", 6 ); /* show cursor */ @@ -307,11 +307,12 @@ static void scroll_to_cursor( struct screen_buffer *screen_buffer ) { int w = screen_buffer->win.right - screen_buffer->win.left + 1; int h = screen_buffer->win.bottom - screen_buffer->win.top + 1; + unsigned int cursor_x = get_bounded_cursor_x(screen_buffer); - if (screen_buffer->cursor_x < screen_buffer->win.left) - screen_buffer->win.left = min( screen_buffer->cursor_x, screen_buffer->width - w ); - else if (screen_buffer->cursor_x > screen_buffer->win.right) - screen_buffer->win.left = max( screen_buffer->cursor_x, w ) - w + 1; + if (cursor_x < screen_buffer->win.left) + screen_buffer->win.left = min( cursor_x, screen_buffer->width - w ); + else if (cursor_x > screen_buffer->win.right) + screen_buffer->win.left = max( cursor_x, w ) - w + 1; screen_buffer->win.right = screen_buffer->win.left + w - 1; if (screen_buffer->cursor_y < screen_buffer->win.top) @@ -1209,7 +1210,7 @@ static void update_read_output( struct console *console ) scroll_to_cursor( screen_buffer ); } if (console->is_unix) - set_tty_cursor_relative( screen_buffer->console, screen_buffer->cursor_x, screen_buffer->cursor_y ); + set_tty_cursor_relative( screen_buffer->console, get_bounded_cursor_x(screen_buffer), screen_buffer->cursor_y ); tty_sync( screen_buffer->console ); update_window_config( screen_buffer->console ); } @@ -1356,7 +1357,7 @@ static NTSTATUS read_console( struct console *console, unsigned int ioctl, size_ } console->edit_line.history_index = console->history_index; - console->edit_line.home_x = console->active->cursor_x; + console->edit_line.home_x = get_bounded_cursor_x(console->active); console->edit_line.home_y = console->active->cursor_y; console->edit_line.status = STATUS_PENDING; if (edit_line_grow( console, 1 )) console->edit_line.buf[0] = 0; @@ -1716,7 +1717,7 @@ static NTSTATUS get_output_info( struct screen_buffer *screen_buffer, size_t *ou info->cursor_size = screen_buffer->cursor_size; info->cursor_visible = screen_buffer->cursor_visible; - info->cursor_x = screen_buffer->cursor_x; + info->cursor_x = get_bounded_cursor_x(screen_buffer); info->cursor_y = screen_buffer->cursor_y; info->width = screen_buffer->width; info->height = screen_buffer->height; @@ -1917,10 +1918,12 @@ static NTSTATUS write_console( struct screen_buffer *screen_buffer, const WCHAR switch (buffer[i]) { case '\b': - if (screen_buffer->cursor_x) screen_buffer->cursor_x--; + if (screen_buffer->cursor_x && screen_buffer->cursor_x < screen_buffer->width) + screen_buffer->cursor_x--; continue; case '\t': - j = min( screen_buffer->width - screen_buffer->cursor_x, 8 - (screen_buffer->cursor_x % 8) ); + j = screen_buffer->cursor_x < screen_buffer->width ? screen_buffer->cursor_x : 0; + j = min( screen_buffer->width - j, 8 - (j % 8) ); while (j--) write_char( screen_buffer, ' ', &update_rect, NULL ); continue; case '\n': @@ -1946,11 +1949,8 @@ static NTSTATUS write_console( struct screen_buffer *screen_buffer, const WCHAR write_char( screen_buffer, buffer[i], &update_rect, NULL ); } - if (screen_buffer->cursor_x == screen_buffer->width) - { - if (screen_buffer->mode & ENABLE_WRAP_AT_EOL_OUTPUT) screen_buffer->cursor_x--; - else screen_buffer->cursor_x = update_rect.left; - } + if (screen_buffer->cursor_x == screen_buffer->width && !(screen_buffer->mode & ENABLE_WRAP_AT_EOL_OUTPUT)) + screen_buffer->cursor_x = update_rect.left; scroll_to_cursor( screen_buffer ); update_output( screen_buffer, &update_rect ); diff --git a/programs/conhost/conhost.h b/programs/conhost/conhost.h index 6f39853..9e855c3 100644 --- a/programs/conhost/conhost.h +++ b/programs/conhost/conhost.h @@ -140,6 +140,11 @@ NTSTATUS write_console_input( struct console *console, const INPUT_RECORD *recor void notify_screen_buffer_size( struct screen_buffer *screen_buffer ); NTSTATUS change_screen_buffer_size( struct screen_buffer *screen_buffer, int new_width, int new_height ); +static inline unsigned int get_bounded_cursor_x( struct screen_buffer *screen_buffer ) +{ + return min(screen_buffer->cursor_x, screen_buffer->width - 1); +} + static inline void empty_update_rect( struct screen_buffer *screen_buffer, RECT *rect ) { SetRect( rect, screen_buffer->width, screen_buffer->height, 0, 0 ); diff --git a/programs/conhost/tests/tty.c b/programs/conhost/tests/tty.c index 7cc37fa..db6c646 100644 --- a/programs/conhost/tests/tty.c +++ b/programs/conhost/tests/tty.c @@ -553,6 +553,8 @@ static void expect_char_key_(unsigned int line, WCHAR ch) static void test_write_console(void) { + BOOL expect_newline; + child_string_request(REQ_WRITE_CONSOLE, L"abc"); skip_hide_cursor(); expect_output_sequence("abc"); @@ -761,6 +763,71 @@ static void test_write_console(void) expect_empty_output(); child_set_output_mode(ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT); + + child_set_cursor(28, 11); + skip_hide_cursor(); + expect_output_sequence("\x1b[12;29H"); /* set cursor */ + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); + + child_string_request(REQ_WRITE_CONSOLE, L"a"); + skip_hide_cursor(); + expect_output_sequence("a"); + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); + + child_string_request(REQ_WRITE_CONSOLE, L"b"); + skip_hide_cursor(); + expect_output_sequence("b"); + expect_newline = TRUE; + if (!skip_sequence("\b")) + { + expect_newline = FALSE; + expect_output_sequence("\r\n"); + } + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); + + child_string_request(REQ_WRITE_CONSOLE, L"c"); + skip_hide_cursor(); + if (expect_newline) expect_output_sequence("\r\n"); + expect_output_sequence("c"); + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); + + child_set_cursor(28, 11); + skip_hide_cursor(); + expect_output_sequence("\x1b[12;29H"); /* set cursor */ + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); + + child_string_request(REQ_WRITE_CONSOLE, L"x"); + skip_hide_cursor(); + expect_output_sequence("x"); + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); + + child_string_request(REQ_WRITE_CONSOLE, L"y"); + skip_hide_cursor(); + expect_output_sequence("y"); + expect_newline = TRUE; + if (!skip_sequence("\b")) + { + expect_newline = FALSE; + expect_output_sequence("\r\n"); + } + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); + + child_string_request(REQ_WRITE_CONSOLE, L"\b"); + expect_empty_output(); + + child_string_request(REQ_WRITE_CONSOLE, L"z"); + skip_hide_cursor(); + if (expect_newline) expect_output_sequence("\r\n"); + expect_output_sequence("z"); + skip_sequence("\x1b[?25h"); /* show cursor */ + expect_empty_output(); } static void test_tty_output(void) diff --git a/programs/conhost/window.c b/programs/conhost/window.c index a665cbb..456bcdd 100644 --- a/programs/conhost/window.c +++ b/programs/conhost/window.c @@ -430,8 +430,8 @@ static void update_window_cursor( struct console *console ) { if (console->win != GetFocus() || !console->active->cursor_visible) return; - SetCaretPos( (console->active->cursor_x - console->active->win.left) * console->active->font.width, - (console->active->cursor_y - console->active->win.top) * console->active->font.height ); + SetCaretPos( (get_bounded_cursor_x(console->active) - console->active->win.left) * console->active->font.width, + (console->active->cursor_y - console->active->win.top) * console->active->font.height ); ShowCaret( console->win ); } @@ -607,10 +607,10 @@ static void update_window( struct console *console ) } } - if (update_all || console->active->cursor_x != console->window->cursor_pos.X || + if (update_all || get_bounded_cursor_x(console->active) != console->window->cursor_pos.X || console->active->cursor_y != console->window->cursor_pos.Y) { - console->window->cursor_pos.X = console->active->cursor_x; + console->window->cursor_pos.X = get_bounded_cursor_x(console->active); console->window->cursor_pos.Y = console->active->cursor_y; update_window_cursor( console ); } -- 2.30.0
Hi Gabriel, On 3/30/21 6:33 PM, Gabriel Ivăncescu wrote:
Remember the fact we completed the entire line in cursor_x, but report it as usual in other cases as the last character on the line. When writing a new character, a newline is then written so it wraps properly.
This doesn't seem to match what we should report to console client. See the attached test (which applies on top of your patch). Thanks, Jacek
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=87969 Your paranoid android. === build (build log) === error: patch failed: programs/conhost/tests/tty.c:551 Task: Patch failed to apply === debiant2 (build log) === error: patch failed: programs/conhost/tests/tty.c:551 Task: Patch failed to apply === debiant2 (build log) === error: patch failed: programs/conhost/tests/tty.c:551 Task: Patch failed to apply
On 30/03/2021 22:21, Jacek Caban wrote:
Hi Gabriel,
On 3/30/21 6:33 PM, Gabriel Ivăncescu wrote:
Remember the fact we completed the entire line in cursor_x, but report it as usual in other cases as the last character on the line. When writing a new character, a newline is then written so it wraps properly.
This doesn't seem to match what we should report to console client. See the attached test (which applies on top of your patch).
Thanks, Jacek
Ok so, I admit I don't really understand the existing tests—or wine's behavior here. It seems that wine's conhost delays outputting a newline until a character is written. Then it writes the newline before the character. This is the current behavior. On the other hand, from the tests I seen, Windows writes a newline as soon as a character is written that reaches the screen buffer width and needs to wrap around. For example, if we have 2 characters until the end of the line, and we write "abc": wine: a, b, \r\nc windows: a, b\r\n, c If we write "ab": wine: a, b, \b windows: a, b\r\n In second case, wine doesn't output a newline at all. Instead, because of the "cursor_x--" thing, it outputs a \b instead. And of course since we don't keep track of this, the next write will overwrite the last character on the line (i.e. the "b") which is the problem here. What I don't understand, though, is why the tests accommodate for Wine's behavior? I assumed it was intentional, that is why my patch keeps this current behavior and just fixes this corner case where writes finish on line end (like the second example above with "ab"). In the current *existing* tests, you can see stuff like: if (!skip_sequence("\b")) expect_output_sequence("\r\n"); This makes it pass on Windows, since it always outputs a newline immediately and never a backspace. In fact, removing the condition at all will *still* pass on Windows. So this condition is there for Wine? Is there a reason for it? And what should I keep in mind here to solve this "properly"? If you remove the condition, of course wine will fail the tests, but Windows won't. I really don't understand it. Thanks, Gabriel
On 3/31/21 1:42 PM, Gabriel Ivăncescu wrote:
This makes it pass on Windows, since it always outputs a newline immediately and never a backspace. In fact, removing the condition at all will *still* pass on Windows. So this condition is there for Wine? Is there a reason for it? And what should I keep in mind here to solve this "properly"?
If you remove the condition, of course wine will fail the tests, but Windows won't. I really don't understand it.
Windows behavior isn't consistent between versions here. When I wrote those tests it was a relatively fresh feature and its output changed between Windows updates. Tests were written in a way that would accept all of them. Hopefully things have matured now and it's good that behavior of recent versions is better. We should follow that and probably even mark those '\b' code paths as broken. Jacek
On 31/03/2021 15:27, Jacek Caban wrote:
On 3/31/21 1:42 PM, Gabriel Ivăncescu wrote:
This makes it pass on Windows, since it always outputs a newline immediately and never a backspace. In fact, removing the condition at all will *still* pass on Windows. So this condition is there for Wine? Is there a reason for it? And what should I keep in mind here to solve this "properly"?
If you remove the condition, of course wine will fail the tests, but Windows won't. I really don't understand it.
Windows behavior isn't consistent between versions here. When I wrote those tests it was a relatively fresh feature and its output changed between Windows updates. Tests were written in a way that would accept all of them. Hopefully things have matured now and it's good that behavior of recent versions is better. We should follow that and probably even mark those '\b' code paths as broken.
Jacek
Ok so it looks like it does add a \b sometimes, but not during the tests I added because I added them after setting the output mode. I'm guessing the default output mode has some "delayed newline" flag and that differs between versions. I'll have to look into it.
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban -
Marvin