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

tui: rework deferred-termcodes implementation #7720

Merged
merged 3 commits into from Dec 13, 2017

Conversation

Projects
None yet
2 participants
@justinmk
Member

justinmk commented Dec 11, 2017

Try another approach to defer the termcodes. Seems less janky, but still
not perfect.
The timer approach (ew) ed92ece eliminates the startup jank.

ref #7664
ref #7649
ref #7664
ref 27f9b1c


QB failure with the non-timer approach (6b51c72 ), seems to cause a SIGABRT (134):

17:08:50,472 INFO  - not ok 2405 - tui with non-tty file descriptors can handle pipes as stdout and stderr
17:08:50,472 INFO  - # test/functional/terminal/tui_spec.lua @ 191
17:08:50,472 INFO  - # Failure message: ./test/functional/ui/screen.lua:302: Row 1 did not match.
17:08:50,472 INFO  - # Expected:
17:08:50,472 INFO  - # |*:w testF |
17:08:50,472 INFO  - # |:q |
17:08:50,472 INFO  - # |abc |
17:08:50,472 INFO  - # | |
17:08:50,472 INFO  - # |[Process exited 0]{1: } |
17:08:50,472 INFO  - # | |
17:08:50,472 INFO  - # |{3:-- TERMINAL --} |
17:08:50,472 INFO  - # Actual:
17:08:50,472 INFO  - # |*/root/neovim/pull-requests-automated/build/Release|
17:08:50,472 INFO  - # |/bin/nvim -u NONE -i NONE --cmd 'set noswapfile no|
17:08:50,472 INFO  - # |showcmd noruler' --cmd 'normal iabc' > /dev/null 2|
17:08:50,472 INFO  - # |>&1 |
17:08:50,472 INFO  - # | |
17:08:50,472 INFO  - # |[Process exited 134]{1: } |
17:08:50,472 INFO  - # |{3:-- TERMINAL --} |
17:08:50,472 INFO  - #
17:08:50,472 INFO  - # To print the expect() call that would assert the current screen state, use
17:08:50,472 INFO  - # screen:snapshot_util(). In case of non-deterministic failures, use
17:08:50,472 INFO  - # screen:redraw_debug() to show all intermediate screen states.
17:08:50,472 INFO  - # stack traceback:
17:08:50,472 INFO  - #     ./test/functional/ui/screen.lua:302: in function 'wait'
17:08:50,472 INFO  - #     ./test/functional/ui/screen.lua:216: in function 'expect'
17:08:50,472 INFO  - #     test/functional/terminal/tui_spec.lua:195: in function <test/functional/terminal/tui_spec.lua:191>

justinmk added some commits Dec 12, 2017

pty_process_unix: _exit() on execvp() failure
Mostly cargo-culting based on a reading of the manpages, interwebs, and
the Vim source.
tui: rework deferred-termcodes implementation
Try another approach to defer the termcodes. Seems less janky, but still
not perfect.

ref #7664
ref #7649
ref #7664
ref 27f9b1c
tui: defer termcodes using a timer
With this implementation there is no "jank" during startup.

Using the main_loop in any fashion is janky. Using only the TUI loop
emits the termcodes too soon, or requires bad hacks like counting
tui_flush invocations (9 seems to work).

ref #7664
ref #7649
ref #7664
ref 27f9b1c

@justinmk justinmk force-pushed the justinmk:tmux-bullshit branch from 3136ab7 to ed92ece Dec 13, 2017

@justinmk justinmk merged commit 6ff13d7 into neovim:master Dec 13, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
QuickBuild Build pr-7720 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@justinmk justinmk deleted the justinmk:tmux-bullshit branch Dec 13, 2017

@mox-mox

This comment has been minimized.

mox-mox commented Dec 14, 2017

I tried it. This version takes considerable time until the callbacks are triggered (feels like half a second or maybe a second).

Enough time in fact to switch windows. Then, vim triggers some events and tells my window manager that vim is open and that it should switch to vim mode. Suddenly, my browser window is focussed with my window manager in vim mode and I'm confused.

Would it be possible to defer sending the request only if term=tmux?
Still, thank you for all that time and effort to try to find a solution that fits everyone.

justinmk added a commit to justinmk/neovim that referenced this pull request Dec 16, 2017

tui: rework deferred-termcodes ... again
- Revert timer-based approach.
- Instead, call loop_poll_events() with a timeout in an "active" loop,
  to infer that "TUI startup activity has mostly finished", but also to
  enforce a mininum time (100 ms) before emitting "enable focus
  reporting" termcode. (If TUI startup takes longer than that minimum
  time, it's probably a slow environment anyways.)
- Tickle `main_loop` by sending a dummy event.  Without this, the
  initial "focus-gained" response from the terminal may not get
  processed until the user hits a key.

ref neovim#7720
ref neovim#7664
ref neovim#7649
ref neovim#7664
ref 27f9b1c
@justinmk

This comment has been minimized.

Member

justinmk commented Dec 16, 2017

@mox-mox #7729 should work much better.

justinmk added a commit to justinmk/neovim that referenced this pull request Dec 16, 2017

tui: rework deferred-termcodes ... again
- Revert timer-based approach.
- Instead, call loop_poll_events() with a timeout in an "active" loop,
  to infer that "TUI startup activity has mostly finished", but also to
  enforce a mininum time (100 ms) before emitting "enable focus
  reporting" termcode. (If TUI startup takes longer than that minimum
  time, it's probably a slow environment anyways.)
- Tickle `main_loop` by sending a dummy event.  Without this, the
  initial "focus-gained" response from the terminal may not get
  processed until the user hits a key.

ref neovim#7720
ref neovim#7664
ref neovim#7649
ref neovim#7664
ref 27f9b1c
@mox-mox

This comment has been minimized.

mox-mox commented Dec 16, 2017

Yes, this is better and solves the problem for most use cases.
I guess, I'll just have to remember not to switch windows immediately after starting nvim.

@justinmk

This comment has been minimized.

Member

justinmk commented Dec 16, 2017

Why? Did you try it? If 100ms still causes the problem then the problem is impossible the avoid...

@mox-mox

This comment has been minimized.

mox-mox commented Dec 16, 2017

Of course I tested it.
I think 100ms is ok. It is quite difficult to do something so fast after launching vim.

Longer explanation:
I saw two problems with the previous commits:

  • Focus events not requested at all
  • Focus events requested too late

When testing earlier today, obviously, the first problem was averted, vim will eventually send the request.

I was however able to switch windows before the notifications got to vim, and so got into an inconsistent state because vim now thought it had the focus while it did not. This is why I wrote I'd have to remember not to switch windows immediately after starting vim.

But then, while writing this reply, I noticed that I could also avert the problem with a change of the perl code that makes urxvt send the notifications to nvim. I would therefore say that your commit fixed the second problem as well (I only noticed after I ironed out a bug in my perl code).

justinmk added a commit that referenced this pull request Jun 11, 2018

NVIM v0.3.0
FEATURES:
3cc7ebf #7234 built-in VimL expression parser
6a7c904 #4419 implement <Cmd> key to invoke command in any mode
b836328 #7679 'startup: treat stdin as text instead of commands'
58b210e :digraphs : highlight with hl-SpecialKey #2690
7a13611 #8276 'startup: Let `-s -` read from stdin'
1e71978 events: VimSuspend, VimResume #8280
1e7d5e8 #6272 'stdpath()'
f96d99a #8247 server: introduce --listen
e8c39f7 #8226 insert-mode: interpret unmapped META as ESC
98e7112 msg: do not scroll entire screen (#8088)
f72630b #8055 let negative 'writedelay' show all redraws
5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330
a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422)
207b7ca #6844 channels: support buffered output and bytes sockets/stdio

API:
f85cbea #7917 API: buffer updates
418abfc #6743 API: list information about all channels/jobs.
36b2e3f #8375 API: nvim_get_commands
273d2cd #8329 API: Make nvim_set_option() update `:verbose set …`
8d40b36 #8371 API: more reliable/descriptive VimL errors
ebb1acb #8353 API: nvim_call_dict_function
9f994bb #8004 API: nvim_list_uis
3405704 #7520 API/UI: forward option updates to UIs
911b1e4 #7821 API: improve nvim_command_output

WINDOWS OS:
9cefd83 #8084, #8516 build/win: support MSVC
ee4e1fd win: Fix reading content from stdin (#8267)

TUI:
ffb8904 #8309 TUI: add support for mouse release events in urxvt
8d5a46e #8081 TUI: implement "standout" attribute
6071637 TUI: support TERM=konsole-256color
67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3)
3d0ee17 TUI/rxvt: enable focus-reporting
d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior

FIXES:
ed6a113 #8273 'job-control: avoid kill-timer race'
4e02f1a #8107 'jobs: separate process-group'
451c48a terminal: flush vterm output buffer on pty output #8486
5d6732f :checkhealth fixes #8335
53f11dc #8218 'Fix errors reported by PVS'
d05712f inccommand: pause :terminal redraws (#8307)
51af911 inccommand: do not execute trailing commands #8256
84359a4 terminal: resize to the max dimensions (#8249)
d49c1dd #8228 Make vim_fgets() return the same values as in Vim
60e96a4 screen: winhl=Normal:Background should not override syntax (#8093)
0c59ac1 #5908 'shada: Also save numbered marks'
ba87a2c cscope: ignore EINTR while reading the prompt (#8079)
b1412dc #7971 ':terminal Enter/Leave should not increment jumplist'
3a5721e TUI: libtermkey: force CSI driver for mouse input #7948
6ff13d7 #7720 TUI: faster startup
1c6e956 #7862 TUI: fix resize-related segfaults
a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output
303e1df #7624 TUI: disable BCE almost always
249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum
6f41ce0 #7704 macOS: Set $LANG based on the system locale
a043899 #7633 'Retry fgets on EINTR'

CHANGES:
ad60927 #8304 default to 'nofsync'
f3f1970 #8035 defaults: 'fillchars'
a6052c7 #7984 defaults: sidescroll=1
b69fa86 #7888 defaults: enable cscopeverbose
7c4bb23 defaults: do :filetype stuff unless explicitly "off"
2aa308c #5658 'Apply :lmap in macros'
8ce6393 terminal: Leave 'relativenumber' alone (#8360)
e46534b #4486 refactor: Remove maxmem, maxmemtot options
131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343
c57d315 #8031 jobwait(): return -2 on interrupt also with timeout
6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940
300d365 #7919 Make 'langnoremap' apply directly after a map
ada1956 #7880 'lua/executor: Remove lightuserdata'

INTERNAL:
de0a954 #7806 internal statistics for list impl
dee78a4 #7708 rewrite internal list impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment