[RFC] Reimplement builtin terminal UI with termkey/unibilium #1820

tarruda · 2015-01-15T21:17:49Z

This PR will throw a lot of old code into the garbage can.

justinmk · 2015-01-15T21:24:25Z

:)

justinmk · 2015-01-15T21:26:23Z

.gitignore

@@ -4,6 +4,7 @@
 /.deps/
 /tmp/

+*.i


Shouldn't the build put *.i files in the build/ subdirectories (in which case, ignoring them here wouldn't be necessary)?

bfredl · 2015-01-15T21:34:10Z

Nice line count of this PR. 👍 Segfaults for the moment with my nvimrc tho, but with nvim -u NONE it works just fine :)

bfredl · 2015-01-15T21:46:40Z

Not sure if helpful, but all craches ends with

Program received signal SIGABRT, Aborted.
0x00007fa72c06ba97 in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007fa72c06ba97 in raise () from /usr/lib/libc.so.6
#1  0x00007fa72c06ce6a in abort () from /usr/lib/libc.so.6
#2  0x00007fa72c0ab2b3 in __libc_message () from /usr/lib/libc.so.6
#3  0x00007fa72c0b072e in malloc_printerr () from /usr/lib/libc.so.6
#4  0x00007fa72c0b0eeb in _int_free () from /usr/lib/libc.so.6
#5  0x00000000005f48b5 in destroy_screen (ui=0x170aff0) at /home/bjorn/Programming/neovim/src/nvim/tui/tui.c:315
#6  0x00000000005f448c in try_resize (ev=...) at /home/bjorn/Programming/neovim/src/nvim/tui/tui.c:241
#7  0x00000000005e328d in process_events_from (queue=0x16fac90) at /home/bjorn/Programming/neovim/src/nvim/os/event.c:174
#8  0x00000000005e3189 in event_poll (ms=0) at /home/bjorn/Programming/neovim/src/nvim/os/event.c:135
#9  0x00000000005e59d2 in input_poll (ms=0) at /home/bjorn/Programming/neovim/src/nvim/os/input.c:25

It crashes on resize and at random times it decides to resize itself (e.g. when doing cmdline completion) with the aforementioned result.

BlackEdder · 2015-01-16T09:20:20Z

This ui doesn't seem to work correctly with terminology (https://www.enlightenment.org/p.php?p=about/terminology) and from my limited testing neither did pynvim. The problem seem to have to do with getting the correct terminal width. The first letter of every sentence starts at the end of the line and then wraps around. Might be a tickit issue I guess (@leonerd)?

jszakmeister · 2015-01-16T09:26:24Z

src/nvim/CMakeLists.txt

@@ -189,8 +192,14 @@ list(APPEND NVIM_LINK_LIBRARIES
    ${LIBUV_LIBRARIES}
    ${MSGPACK_LIBRARIES}
    ${LUAJIT_LIBRARIES}
+    "-Wl,--whole-archive"


This is failing to build on Mac OS X:

ld: unknown option: --whole-archive

I should add that the error above is on Yosemite where it is using LLVM's linker. Mavericks is probably a different story, but FreeBSD may fail as well. There is a -all_load, but the corresponding -noall_load is obsolete. Turns out -all_load affects all static archives. :-( There is also a -force_load LIBRARY, but detecting the presence of the option at build time is kind of a pain since it requires an archive.

Honestly I don't understand why this flag is necessary for libtickit & dependencies, I've used it before but only for building shared libraries that include other static libraries. Do you know a another way of avoiding missing reference errors during link stage?

Just reverse the order to:

${LIBTICKIT_LIBRARIES} ${LIBTERMKEY_LIBRARIES} ${LIBUNIBILIUM_LIBRARIES}

And it is fine.

Just reverse the order to:

👍 great, thanks!

I'm trying to understand why the change in order was necessary. Is it because unibilium/termkey are skipped when it determines none of its symbols are directly used by the nvim binary? If so then it actually makes sense.

It's because linking is very linear. In this case, libtickit depends on both libtermkey and libunibilium. If you link libtickit last, then those functions won't be resolved again (because they were present in previous libraries). However, if you change the order so that libtickit gets linked first, then the linker knows about the unresolved functions in libtickit can fix them up correctly when libtermkey and libunibilium are linked in next.

In general, you want the objects with the most dependencies to be linked in first, and the objects with no dependencies linked in last. And you want to make sure to link the dependencies in after the object that requires them. What you are really doing is a topological sort. :-)

tarruda · 2015-01-16T10:20:26Z

Nice line count of this PR. 👍 Segfaults for the moment with my nvimrc tho, but with nvim -u NONE it works just fine :)

I'm just getting started, I think this PR will remove more than 7k lines :)

It crashes on resize and at random times it decides to resize itself (e.g. when doing cmdline completion) with the aforementioned result.

Could be what @oni-link spotted, I will investigate.

tarruda · 2015-01-17T00:00:27Z

This ui doesn't seem to work correctly with terminology (https://www.enlightenment.org/p.php?p=about/terminology) and from my limited testing neither did pynvim. The problem seem to have to do with getting the correct terminal width. The first letter of every sentence starts at the end of the line and then wraps around. Might be a tickit issue I guess (@leonerd)?

👍 Will investigate.

Some legacy functional tests are failing, but the UI should be as stable as the python version.

fwalch · 2015-01-17T13:53:51Z

Is it possible to get the libtickit/libtermkey changes into upstream before this is merged into master? I ask because e.g. Arch Linux will need to package these libraries, and first introducing a special libtickit-unstable-neovim and then replacing it with libtickit-unstable at some point will be a bit more work.

tarruda · 2015-01-17T13:56:22Z

Is it possible to get the libtickit/libtermkey changes into upstream before this is merged into master? I ask because e.g. Arch Linux will need to package these libraries, and first introducing a special libtickit-unstable-neovim and then replacing it with libtickit-unstable at some point will be a bit more work.

👍. Don't worry as this will only be merged when all tests are fixed and the changes are sent back to @leonerd.

Did you get a chance to test this branch? How stable is it for you?

fwalch · 2015-01-17T14:07:32Z

Did you get a chance to test this branch? How stable is it for you?

I had to check nvim --version to make sure I'm really using this branch, because I didn't notice any differences. Didn't have any problems so far :-)

tarruda · 2015-01-17T14:13:17Z

I had to check nvim --version to make sure I'm really using this branch, because I didn't notice any differences. Didn't have any problems so far :-)

Nice, if you don't mind please keep using it. This PR introduces some serious architectural changes and the more people are testing it the better.

I'm interested in knowing this branch deals with troubling key mappings. One of the compelling reasons I choose libtickit/libtermkey is because it is capable of understanding an encoding scheme that uniquely identifies each key(though I'm not sure if some special terminal configuration is required for that)

oni-link · 2015-01-17T14:28:03Z

src/nvim/tui/tui.c

+  // setup libtickit
+  data->tt = tickit_term_new();
+  tickit_term_set_input_fd(data->tt, data->in_fd);
+  tickit_term_set_output_fd(data->tt, data->in_fd);


data->out_fd instead of data->in_fd?

oni-link · 2015-01-17T16:40:23Z

src/nvim/tui/tui.c

+  char buf[64];
+
+  if (type == TICKIT_KEYEV_KEY) {
+    if (!strcmp("BackSpace", str)) {


Should be Backspace instead of BackSpace.

Tranquility · 2015-01-17T21:24:26Z

I realized a small problem with the shell output. Unlike the pynvim ui it is extremely fast which is awesome but the the background color is wrong. Only the text has background color of the colorscheme but the part without text is black.

aktau · 2015-01-18T18:25:50Z

I'm not entirely through the code yet, but where did input encoding go? (the removed lines in os/input.c).

tarruda · 2015-01-19T02:13:39Z

I'm not entirely through the code yet, but where did input encoding go? (the removed lines in os/input.c).

I've removed it, the input_enqueue function should always receive utf-8 strings as input. I'm gonna investigate if that input conversion code is really necessary, and if so it will be done in the tickit UI layer.

tarruda · 2015-02-17T01:29:11Z

Ok, this is as ready as its gonna get, merging after the tests.

@elmart as soon as you identify a pattern for reproducing the YCM slowdown, I will investigate and hopefully come up with a fix soon

justinmk · 2015-02-17T01:44:21Z

Great job!

The `wait` function will only return after all input has been processed by nvim. It is useful to time assertions correctly.

Without waiting for the 'gg' command to be processed, its possible that the following assertion will fail.

The `system` function is never executed with these tests because the ctrl+c is queued with the input string that calls it(The `process_interrupts` function will destroy all previous input when a ctrl+c is found).

Some screen tests such as system/ctrl+c(viml_system_spec.lua) can take some time to respond(default kill timeout is 2 seconds for an interrupted job) and fail when running under a slow environment such as travis.

This is required to correctly handle certain keys such as <delete>

Ignore all keys that aren't prefixed with KS_EXTRA.

The input buffer is only used for data that really came from another process and is only visible to os/input.c. Remove the input_buffer_{save,restore} functions, they are not necessary(Also can result in problems if data comes while the typeahead is saved).

- Remove abstract_ui global, now it is always active - Remove some terminal handling code - Remove unused functions - Remove HAVE_TGETENT/TERMINFO/TERMIOS/IOCTL #ifdefs - Remove tgetent/terminfo from version.c - Remove curses/terminfo dependencies - Only start/stop termcap when starting/exiting the program - msg_use_printf will return true if there are no attached UIs( messages will be written to stdout) - Remove `ex_winpos`(implement `:winpos` with `ex_ni`)

Now all terminal-handling code was moved to src/nvim/tui, which implements a new terminal UI based on libtermkey and unibilium

Now the attrentry_T structure will store all attributes in separate fields for cterm and rgb UIs.

- The syntax `gui=` is invalid when setting properties of highlight group. - Wait for the initial "-- More --" prompt before continuing. Required to avoid a race condition

bigeagle · 2015-02-17T09:04:00Z

Hi, using the merged version, there're still a problem that the terminal's background color cannot be automatically "resetted". To repeat this bug, nvim -u NONE and try to show some error message, e.g. :q on an unsaved file and there would be an error message with red bg color, now press : and the red background is still there.
I'm using xterm-256color.

justinmk · 2015-02-17T16:40:27Z

Please report in a new issue.

elmart · 2015-02-18T10:49:30Z

as soon as you identify a pattern for reproducing the YCM slowdown, I will investigate and hopefully come up with a fix soon

Ok.

Really great work, @tarruda. Thumbs up! 👍

justinmk reviewed Jan 15, 2015
View reviewed changes

marvim added the WIP label Jan 15, 2015

jszakmeister reviewed Jan 16, 2015
View reviewed changes

tarruda force-pushed the tickit-ui branch 2 times, most recently from fa82072 to a1daf73 Compare January 16, 2015 17:52

tarruda mentioned this pull request Jan 16, 2015

Resizing causes flicker problems with GNOME terminal #1827

Closed

tarruda force-pushed the tickit-ui branch 2 times, most recently from 2fc8cf1 to 0c64a89 Compare January 16, 2015 23:56

tarruda mentioned this pull request Jan 17, 2015

[RFC] snapshot_util (Screen Tests for the Lazy) and enable stricter highlight tests #1835

Closed

tarruda force-pushed the tickit-ui branch from 0c64a89 to 79fcf9e Compare January 17, 2015 13:57

oni-link reviewed Jan 17, 2015
View reviewed changes

tarruda force-pushed the tickit-ui branch from 79fcf9e to c20b5a2 Compare January 17, 2015 15:28

oni-link reviewed Jan 17, 2015
View reviewed changes

tarruda mentioned this pull request Jan 17, 2015

Pynvim segfaults #1837

Closed

fwalch mentioned this pull request Jan 18, 2015

clicking on line n moves cursor to line n+1 #1839

Closed

tarruda force-pushed the tickit-ui branch from c20b5a2 to 824c9f5 Compare January 19, 2015 02:08

tarruda mentioned this pull request Jan 19, 2015

Travis neovim builds timing out google/vroom#83

Open

tarruda force-pushed the tickit-ui branch from a5be2d4 to be33a99 Compare February 17, 2015 01:21

tarruda added 14 commits February 16, 2015 23:17

test: Add synchronization helper for functional tests

e6208df

The `wait` function will only return after all input has been processed by nvim. It is useful to time assertions correctly.

test: Fix race condition in window_spec.lua

e974b00

Without waiting for the 'gg' command to be processed, its possible that the following assertion will fail.

test: Remove unnecessary tests from viml_system_spec.lua

291e15c

The `system` function is never executed with these tests because the ctrl+c is queued with the input string that calls it(The `process_interrupts` function will destroy all previous input when a ctrl+c is found).

test: Increase default_screen_timeout when running on travis

98dca8a

Some screen tests such as system/ctrl+c(viml_system_spec.lua) can take some time to respond(default kill timeout is 2 seconds for an interrupted job) and fail when running under a slow environment such as travis.

macro: Add MAX macro and rename min to MIN

8f31ebc

input: Prefer keycodes in input_enqueue key translation

2b6ffa6

This is required to correctly handle certain keys such as <delete>

input: Fix handle_mouse_event function

e17f92e

Ignore all keys that aren't prefixed with KS_EXTRA.

ui: Remove redundant cursor_goto calls

6383ea6

ui: Rewrite the builtin terminal UI

9a2dd7c

Now all terminal-handling code was moved to src/nvim/tui, which implements a new terminal UI based on libtermkey and unibilium

syntax: Refactor to store all term and gui attributes independently

d8f3458

Now the attrentry_T structure will store all attributes in separate fields for cterm and rgb UIs.

cleanup: Remove unused functions and builtin termcaps

419da83

test: Fix 051_highlight_spec.lua

40b7990

- The syntax `gui=` is invalid when setting properties of highlight group. - Wait for the initial "-- More --" prompt before continuing. Required to avoid a race condition

tarruda force-pushed the tickit-ui branch from be33a99 to 40b7990 Compare February 17, 2015 02:17

tarruda changed the title ~~[RFC] Reimplement builtin terminal UI using libtermkey/unibilium~~ [RFC] Reimplement builtin terminal UI with termkey/unibilium Feb 17, 2015

tarruda merged commit 40b7990 into neovim:master Feb 17, 2015

jszakmeister removed the RFC label Feb 17, 2015

tarruda added a commit that referenced this pull request Feb 17, 2015

Merge PR #1820 'Reimplement builtin terminal UI with termkey/unibilium'

0429857

ghost mentioned this pull request Feb 17, 2015

Commit a99d33ad breaks bindings with meta(alt) key in xterm #1881

Closed

lithammer mentioned this pull request Feb 17, 2015

New terminal UI broke bracketed paste mode #2000

Closed

neovim locked and limited conversation to collaborators Feb 17, 2015

mjlbach added this to Done in LSP integration Mar 14, 2021

mjlbach removed this from Done in LSP integration Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Reimplement builtin terminal UI with termkey/unibilium #1820

[RFC] Reimplement builtin terminal UI with termkey/unibilium #1820

tarruda commented Jan 15, 2015

justinmk commented Jan 15, 2015

justinmk Jan 15, 2015

tarruda Jan 16, 2015

bfredl commented Jan 15, 2015

bfredl commented Jan 15, 2015

BlackEdder commented Jan 16, 2015

jszakmeister Jan 16, 2015

jszakmeister Jan 16, 2015

tarruda Jan 17, 2015

jszakmeister Jan 17, 2015

tarruda Jan 19, 2015

tarruda Jan 19, 2015

jszakmeister Jan 21, 2015

tarruda commented Jan 16, 2015

tarruda commented Jan 17, 2015

fwalch commented Jan 17, 2015

tarruda commented Jan 17, 2015

fwalch commented Jan 17, 2015

tarruda commented Jan 17, 2015

oni-link Jan 17, 2015

tarruda Jan 17, 2015

oni-link Jan 17, 2015

Tranquility commented Jan 17, 2015

aktau commented Jan 18, 2015

tarruda commented Jan 19, 2015

tarruda commented Feb 17, 2015

justinmk commented Feb 17, 2015

bigeagle commented Feb 17, 2015

justinmk commented Feb 17, 2015

elmart commented Feb 18, 2015

@@ @@ -4,6 +4,7 @@ @@
               /.deps/
               /tmp/
+              *.i

[RFC] Reimplement builtin terminal UI with termkey/unibilium #1820

[RFC] Reimplement builtin terminal UI with termkey/unibilium #1820

Conversation

tarruda commented Jan 15, 2015

justinmk commented Jan 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfredl commented Jan 15, 2015

bfredl commented Jan 15, 2015

BlackEdder commented Jan 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarruda commented Jan 16, 2015

tarruda commented Jan 17, 2015

fwalch commented Jan 17, 2015

tarruda commented Jan 17, 2015

fwalch commented Jan 17, 2015

tarruda commented Jan 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tranquility commented Jan 17, 2015

aktau commented Jan 18, 2015

tarruda commented Jan 19, 2015

tarruda commented Feb 17, 2015

justinmk commented Feb 17, 2015

bigeagle commented Feb 17, 2015

justinmk commented Feb 17, 2015

elmart commented Feb 18, 2015