Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

win,tty: Added set cursor style to CSI sequences #1884

Closed
wants to merge 2 commits into from

Conversation

erw7
Copy link
Contributor

@erw7 erw7 commented Jun 15, 2018

This is for adding a set cursor style(CSI Ps SP q, CSI SP q) to handling of libuv escape sequence. This was originally created for neovim(neovim/neovim#8462).

Fix except 3. of #1874. I confirmed 3. with another terminal(xterm, mintty). Since another terminal accepted a sequence like CSI Ps ; Ps ; Ps H, I did not fix it.

Copy link
Contributor

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally seems reasonable, but I have a few comments.

Additionally, the repeated prev_utf8_codepoint != ' ' checks drive me crazy a little, it's kind difficult now to reason about state transitions now. Would it be possible to check for space at a single location, and maybe add an extra state flag (e.g. ANSI_DECSCUSR) similar to the ANSI_EXTENSION flag you just added?

src/win/tty.c Outdated Show resolved Hide resolved
src/win/tty.c Outdated Show resolved Hide resolved
src/win/tty.c Outdated Show resolved Hide resolved
src/win/tty.c Show resolved Hide resolved
src/win/tty.c Outdated Show resolved Hide resolved
src/win/tty.c Outdated Show resolved Hide resolved
src/win/tty.c Outdated Show resolved Hide resolved
@erw7 erw7 force-pushed the feature-set-cursor-style branch from a7c279a to fff3c4b Compare June 21, 2018 20:50
@refack
Copy link
Contributor

refack commented Jun 21, 2018

@erw7 any way to unit test this?

@erw7 erw7 force-pushed the feature-set-cursor-style branch from fff3c4b to f1715a1 Compare June 21, 2018 20:56
@erw7
Copy link
Contributor Author

erw7 commented Jun 21, 2018

any way to unit test this?

I think that it is possible. But in order to do unit tests, I think that it is necessary to change uv__vterm_state to UV_UNSUPPORTED only during testing. Can I create a private function that does that setting? Or is there a better way?

@erw7
Copy link
Contributor Author

erw7 commented Jun 27, 2018

I wrote the test and noticed the following things, please tell me about uv_tty_virtual_offset.

I think it is a bug to initialize uv_tty_virtual_offset with info->dwCursorPosition.Y, because the cursor position is not always on the first line of the display window. I think uv_tty_virtual_offset should be initialized with info->srWindow.Top. Is this idea correct?

If the above is correct, I think that uv_tty_virtual_offset is always equal to info->srWindows.Top. If so, are there any problems if we use info->srWindow.Top instead of uv_tty_virtual_offset?

@erw7 erw7 force-pushed the feature-set-cursor-style branch from 726f05f to 58f4f81 Compare June 27, 2018 19:54
@piscisaureus
Copy link
Contributor

Nice work @erw7.
All these tests are awesome -- we have to get this in.

@piscisaureus
Copy link
Contributor

Note that on windows 10 these tests don't all pass, because windows processes the escape sequences and not libuv. Maybe you can somehow forcefully enable libuv's escape sequence processing when running the tty tests?

test output
1..340
ok 1 - tty
ok 2 - tty_cursor_back
ok 3 - tty_cursor_down
ok 4 - tty_cursor_forward
ok 5 - tty_cursor_horizontal_move_absolute
ok 6 - tty_cursor_move_absolute
ok 7 - tty_cursor_next_line
ok 8 - tty_cursor_previous_line
ok 9 - tty_cursor_up
ok 10 - tty_empty_write
ok 11 - tty_erase
ok 12 - tty_erase_line
not ok 13 - tty_escape_sequence_processing
# exit code 3
# Output from process `tty_escape_sequence_processing`:
# line:1 col:1 expected character 'H' but found ' '
# line:1 col:2 expected character 'e' but found ' '
# line:1 col:3 expected character 'l' but found ' '
# line:1 col:4 expected character 'l' but found ' '
# line:1 col:5 expected character 'o' but found ' '
# line:1 col:6 expected character 'H' but found ' '
# line:1 col:7 expected character 'e' but found ' '
# line:1 col:8 expected character 'l' but found ' '
# line:1 col:9 expected character 'l' but found ' '
# line:1 col:10 expected character 'o' but found ' '
# Assertion failed in c:\users\bertbelder\d\libuv\test\test-tty-escape-sequence-processing.c on line 1362: compare_screen(&tty_out, &actual, &expect)
ok 14 - tty_file
ok 15 - tty_full_reset
ok 16 - tty_hide_show_cursor
ok 17 - tty_large_write
ok 18 - tty_pty
ok 19 - tty_raw
ok 20 - tty_save_restore_cursor_position
ok 21 - tty_set_cursor_shape
not ok 22 - tty_set_style
# exit code 3
# Output from process `tty_set_style`:
# line:23 col:77 expected character 'H' but found ' '
# line:23 col:77 expected attributes '0' but found '7'
# line:23 col:78 expected character 'e' but found ' '
# line:23 col:78 expected attributes '0' but found '7'
# line:23 col:79 expected character 'l' but found ' '
# line:23 col:79 expected attributes '0' but found '7'
# line:23 col:80 expected character 'l' but found ' '
# line:23 col:80 expected attributes '0' but found '7'
# line:23 col:81 expected character 'o' but found ' '
# line:23 col:81 expected attributes '0' but found '7'
# Assertion failed in c:\users\bertbelder\d\libuv\test\test-tty-escape-sequence-processing.c on line 1093: compare_screen(&tty_out, &actual, &expect)

erw7 added a commit to erw7/libuv that referenced this pull request Jul 28, 2018
@erw7
Copy link
Contributor Author

erw7 commented Jul 29, 2018

Maybe you can somehow forcefully enable libuv's escape sequence processing when running the tty tests?

I think that tty tests use libuv escape sequence processing, because I call uv__set_vterm_state in the initialize_tty function and set UV_SUPPORTED for uv__vterm_state.

When I compiled with MSYS 2 MinGW 64-bit on WIndows 10 installed in Virtual Box, I passed the corresponding test.

@erw7 erw7 mentioned this pull request Sep 27, 2018
9 tasks
erw7 added a commit to erw7/libuv that referenced this pull request Sep 27, 2018
erw7 added a commit to erw7/libuv that referenced this pull request Oct 6, 2018
- Retrieve libuv#1884 changes
- Change uv_set_vterm_state from Private to Public
- Revert "win,tty: Use ANSI and Xterm processing of ConEmu (#1)"
justinmk added a commit to neovim/libuv that referenced this pull request Oct 6, 2018
erw7 added a commit to erw7/libuv that referenced this pull request Oct 8, 2018
- Retrieve libuv#1884 changes
- Change uv_set_vterm_state from Private to Public
- Revert "win,tty: Use ANSI and Xterm processing of ConEmu (#1)"
erw7 added a commit to erw7/libuv that referenced this pull request Oct 12, 2018
- Retrieve libuv#1884 changes
- Change uv_set_vterm_state from Private to Public
- Revert "win,tty: Use ANSI and Xterm processing of ConEmu (#1)"
erw7 added a commit to erw7/libuv that referenced this pull request Oct 28, 2018
- Retrieve libuv#1884 changes
- Change uv_set_vterm_state from Private to Public
- Revert "win,tty: Use ANSI and Xterm processing of ConEmu (#1)"
erw7 added a commit to erw7/libuv that referenced this pull request Oct 29, 2018
- Retrieve libuv#1884 changes
- Change uv_set_vterm_state from Private to Public
- Revert "win,tty: Use ANSI and Xterm processing of ConEmu (#1)"
src/win/tty.c Outdated Show resolved Hide resolved
erw7 added a commit to erw7/libuv that referenced this pull request Nov 15, 2018
- Retrieve libuv#1884 changes
- Change uv_set_vterm_state from Private to Public
- Revert "win,tty: Use ANSI and Xterm processing of ConEmu (#1)"
@saghul
Copy link
Member

saghul commented Feb 27, 2020

@erw7 feel free to land!

erw7 added a commit to erw7/libuv that referenced this pull request Feb 28, 2020
PR-URL: libuv#1884
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
erw7 added a commit to erw7/libuv that referenced this pull request Feb 28, 2020
PR-URL: libuv#1884
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
erw7 added a commit to erw7/libuv that referenced this pull request Feb 28, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
erw7 added a commit to erw7/libuv that referenced this pull request Feb 28, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
erw7 and others added 2 commits February 28, 2020 15:26
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
PR-URL: libuv#1884
Refs: libuv#1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
erw7 added a commit that referenced this pull request Feb 29, 2020
PR-URL: #1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
erw7 added a commit that referenced this pull request Feb 29, 2020
PR-URL: #1884
Refs: #1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
@erw7
Copy link
Contributor Author

erw7 commented Feb 29, 2020

Landed in 288a067, 73ca4ac.

@c42f
Copy link

c42f commented Mar 12, 2020

Awesome, thanks @erw7 this is epic! Now this is in, revisiting scroll region handling seems more approachable.

@erw7 erw7 deleted the feature-set-cursor-style branch March 13, 2020 04:10
musm pushed a commit to musm/libuv that referenced this pull request Jul 9, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 73ca4ac)
musm pushed a commit to musm/libuv that referenced this pull request Jul 9, 2020
PR-URL: libuv#1884
Refs: libuv#1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 288a067)
musm pushed a commit to musm/libuv that referenced this pull request Jul 9, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 73ca4ac)
musm pushed a commit to musm/libuv that referenced this pull request Jul 9, 2020
PR-URL: libuv#1884
Refs: libuv#1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 288a067)
musm pushed a commit to musm/libuv that referenced this pull request Jul 14, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 73ca4ac)
musm pushed a commit to musm/libuv that referenced this pull request Jul 14, 2020
PR-URL: libuv#1884
Refs: libuv#1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 288a067)
musm pushed a commit to musm/libuv that referenced this pull request Jul 14, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 73ca4ac)
musm pushed a commit to musm/libuv that referenced this pull request Jul 14, 2020
PR-URL: libuv#1884
Refs: libuv#1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 288a067)
musm pushed a commit to musm/libuv that referenced this pull request Jul 14, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 73ca4ac)
musm pushed a commit to musm/libuv that referenced this pull request Jul 14, 2020
PR-URL: libuv#1884
Refs: libuv#1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 288a067)
musm pushed a commit to JuliaLang/libuv that referenced this pull request Jul 21, 2020
PR-URL: libuv#1884
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 73ca4ac)
musm pushed a commit to JuliaLang/libuv that referenced this pull request Jul 21, 2020
PR-URL: libuv#1884
Refs: libuv#1874
Co-authored-by: Bert Belder <bertbelder@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 288a067)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants