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

[RFC] Make terminals work with buffer updates #8616

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@KillTheMule
Contributor

KillTheMule commented Jun 21, 2018

Closese #8575.

I don't trust the test. Is there something like test-shell that gives me environment-independent output in the terminal, but is a tad more interactive? Like, an echo server or so? Any ideas welcome, this feels a tad too easy :D

@@ -1241,7 +1241,7 @@ static void refresh_screen(Terminal *term, buf_T *buf)
int change_end = change_start + changed;
changed_lines(change_start, 0, change_end, added,
// Don't send nvim_buf_lines_event for :terminal buffer.

This comment has been minimized.

@mhinz

mhinz Jun 21, 2018

Member

Just a reminder that this comment is obsolete after this change. :>

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jun 21, 2018

Then again, we're kinda shipping an interactive program indeed, so using that seems obvious. Let's see what CI makes of it :)

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jun 21, 2018

Ah that reminds me, @phodge any reason you disabled buffer updates a priori? Is there something I'm not seeing?

@KillTheMule KillTheMule force-pushed the KillTheMule:termupdates branch 2 times, most recently from ab033a3 to 6668d19 Jun 21, 2018

@phodge

This comment has been minimized.

Contributor

phodge commented Jun 22, 2018

@KillTheMule: I disabled it because it seemed like the unit tests required to prove the feature works would be complex and time consuming to make and I didn't think anyone would want buffer updates for a :terminal window anyway. It wouldn't surprise me if changing that false to true is all you need to do to get terminal buffer updates working.

@KillTheMule KillTheMule force-pushed the KillTheMule:termupdates branch 2 times, most recently from a8f7cd1 to f5317ad Jun 22, 2018

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jun 22, 2018

Ok, the test is officially driving me nuts. It takes a whooping 30s on my machine, but everytime I try to measure something, the offending line turns out to be something different. I tried putting in early returns, that made this line seem to be 1/3 of the culprit. Then again, I've put in all these measurements via os.clock() (which seem to be somewhat off, since those return a duration of 0.30, but for now I assume there's a factor of 100 missing somewhere), and that made all those sendkeys lines the culprits. There's a delay in there, but changing that delay changes... nothing!

I'm stumped. Help!

(e) Observation: This test takes 30s on appveyor as well as on travis and quickbuild. That's ... weird.

@KillTheMule KillTheMule force-pushed the KillTheMule:termupdates branch from f5317ad to d6f4327 Jun 22, 2018

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jun 23, 2018

@bfredl figured it out. I was calling next_msg() until it returned nil, but it only does so after reaching its timeout.
What we're now doing the following:

  1. Do some things with nvim in the terminal
  2. At some point, go through all the next_msges and update our mirrored buffer state. After each next_msg, compare the mirrored state to an expected state, and if it matches, we're happy and stop calling next_msg.
  3. Go back to 1.

We're not comparing the state of the embedded nvim's message area (that's to racy in my experience), and we're ignoring the changedtick's, which seem dependent on the redraw behavior of the embbeded nvim, which does not seem predictable between different OSes. But I think making sure that 'keeping a buffer mirror' makes you reach the expected state sometime is what can be expected here.

Question: I can shave of 100ms off the test by not quitting the embedded neovim instance properly. Should I do that?

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jun 23, 2018

Ok, CI seems happy, I'm marking this as RFC and pushing some little improvements.

@KillTheMule KillTheMule force-pushed the KillTheMule:termupdates branch from 0f6b1ec to 5a8e550 Jun 23, 2018

@KillTheMule KillTheMule changed the title from Make terminals work with buffer updates to [RFC] Make terminals work with buffer updates Jun 23, 2018

@marvim marvim added the RFC label Jun 23, 2018

it('terminal with a nested nvim instance', function()
local buffer_lines = {}
local expected_lines = {}
command("terminal " .. nvim_dir ..'/nvim -n -c "set shortmess+=A"')

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

-u NONE -i NONE might be a good idea as well, else a developer's local config may get in the way.

return lines_subset(f, s) and lines_subset(s, f)
end
local function did_match_somwhere(buffer_lines, expected_lines)

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

typo: somewhere

expected_lines[1] = 'Blarg'
expected_lines[22] = 'tmp_terminal_nvim [+] ' ..
'1,6 All'

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

With lua it's a good idea to wrap multiline expressions in parens. That also allows the .. operator to start the next line.

Maybe it's worth using (' '):rep(41) to repeat the spaces.

expected_lines[22] = ('tmp_terminal_nvim [+]'..(' '):rep(41)
                     ..'1,6            All')
end
it('terminal with a nested nvim instance', function()
local buffer_lines = {}

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

AFAICT this could be created in did_match_somewhere, no need to pass (and mutate) it as an argument.

This comment has been minimized.

@KillTheMule

KillTheMule Jun 24, 2018

Contributor

It's kind of the point to test this exact behavior, namely "externally" (here only by a function-external variable) the buffer state and making sure it would stay correct during consecutive updates. Note that not all updates consist of 23 lines, so to compare our mirrored buffer state we need a notion of "how did it look before the update event".

Am I making sense?

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

Do you mean expected_lines?

buffer_lines is set inside did_match_somewhere. Only expected_lines needs to be passed as an argument.

This comment has been minimized.

@KillTheMule

KillTheMule Jun 24, 2018

Contributor

No I meant buffer_lines. It's only updated in did_match_somewhere, not always fully set. What would happen if the nvim_buf_lines_event only contains 2 lines? We couldn't compare expected_lines then, because those contain the full expected buffer. We could of course shrink down expected_lines for that comparison, but than we'd be back a step in that we're looking for a nvim_buf_lines_event containing some specific event data, which I had problem with (probably due to screen refreshes that weren't fully deterministic between OSes).

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

oh, so later calls to did_match_somewhere depend on earlier calls. might want to note that in a comment above did_match_somewhere.

end
msg = next_msg()
end
return false

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

should assert(false) here, with a brief semantic message.

assert(false, 'did not match expected lines')

I don't see any place where the return value of did_match_somewhere is actually checked.

This comment has been minimized.

@KillTheMule

KillTheMule Jun 24, 2018

Contributor

Right, I was planing on using the result for an assert, but somehow got over it.

This comment has been minimized.

@KillTheMule

KillTheMule Jun 24, 2018

Contributor

It's undesirable though that one can see where the assert failed, but not how the expected/passed in lines looked like. Is there a way to do that?

This comment has been minimized.

@justinmk

justinmk Jun 24, 2018

Member

something like

assert(false, 'did not match expected lines: \n'..table.concat(expected, '\n'))
@justinmk

This comment has been minimized.

Member

justinmk commented Jun 24, 2018

I can shave of 100ms off the test by not quitting the embedded neovim instance properly.

Yes, the test harness takes care of that.

@@ -4,6 +4,7 @@ local buffer, command, eval, nvim, next_msg = helpers.buffer,
helpers.command, helpers.eval, helpers.nvim, helpers.next_msg
local expect_err = helpers.expect_err
local write_file = helpers.write_file
local nvim_dir = helpers.nvim_dir

This comment has been minimized.

@justinmk

justinmk Jul 1, 2018

Member

could use nvim_prog instead.

return lines_subset(f, s) and lines_subset(s, f)
end
local function did_match_somewhere(buffer_lines, expected_lines)

This comment has been minimized.

@justinmk

justinmk Jul 1, 2018

Member

I renamed this to assert_match_somewhere and swapped the parameter order (typically the "expected" value is the first parameter).

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 1, 2018

Merged. Thanks @KillTheMule !

@justinmk justinmk closed this Jul 1, 2018

@justinmk justinmk removed the RFC label Jul 1, 2018

justinmk added a commit that referenced this pull request Jul 1, 2018

justinmk added a commit that referenced this pull request Jul 18, 2018

NVIM v0.3.1
FEATURES:
07499a8 #8709 man.vim: C highlighting for EXAMPLES section
07f82ad #8699 TUI: urxvt: also send xterm focus-reporting seqs
40911e4 #8616 API: emit nvim_buf_lines_event from :terminal
c46997a #8546 fillchars: Add "eob" flag

FIXES:
74d19f6 #8576 startup: avoid blank stdin buffer if other files were opened
4874214 #8737 Only waitpid() for processes that we care about
cd6e7e8 #8743 Check all child processes for exit in SIGCHLD handler
c230ef2 #8746 channel.c: Prevent channel_destroy_early() from freeing uninitialized rpc stuff
0ed8b12 #8681 transstr_buf: fix length comparison
d241f27 #8708 TUI: Fix standout mode
9afed40 #8698 man.vim: fix for mandoc
e889640 #8682 provider/node: npm --loglevel silent
1cbc830 #8613 API: nvim_win_set_cursor: set curswant
bf6048e #8628 checkhealth: Python: fix VIRTUAL_ENV check
3cc3506 #8528 checkhealth: node.js: also search yarn

CHANGES:
b751449 #8619 defaults: shortmess+=F
1248178 #8578 highlight: high-priority CursorLine if fg is set.
01570f1 #8726 terminal: handle &confirm and :confirm on unloading
56065bb #8721 screen: truncate showmode messages
bf2460e #7551 buffer: fix copying :setlocal options
c1c14fa #8520 Ex mode: always "improved" (gQ)
050f397 #7992 options: remove 'maxcombine` option (always 6)

INTERNAL:
463da84 #7992 screen: use UTF-8 representation

UtkarshMe added a commit to UtkarshMe/neovim that referenced this pull request Jul 28, 2018

UtkarshMe added a commit to UtkarshMe/neovim that referenced this pull request Jul 28, 2018

NVIM v0.3.1
FEATURES:
07499a8 neovim#8709 man.vim: C highlighting for EXAMPLES section
07f82ad neovim#8699 TUI: urxvt: also send xterm focus-reporting seqs
40911e4 neovim#8616 API: emit nvim_buf_lines_event from :terminal
c46997a neovim#8546 fillchars: Add "eob" flag

FIXES:
74d19f6 neovim#8576 startup: avoid blank stdin buffer if other files were opened
4874214 neovim#8737 Only waitpid() for processes that we care about
cd6e7e8 neovim#8743 Check all child processes for exit in SIGCHLD handler
c230ef2 neovim#8746 channel.c: Prevent channel_destroy_early() from freeing uninitialized rpc stuff
0ed8b12 neovim#8681 transstr_buf: fix length comparison
d241f27 neovim#8708 TUI: Fix standout mode
9afed40 neovim#8698 man.vim: fix for mandoc
e889640 neovim#8682 provider/node: npm --loglevel silent
1cbc830 neovim#8613 API: nvim_win_set_cursor: set curswant
bf6048e neovim#8628 checkhealth: Python: fix VIRTUAL_ENV check
3cc3506 neovim#8528 checkhealth: node.js: also search yarn

CHANGES:
b751449 neovim#8619 defaults: shortmess+=F
1248178 neovim#8578 highlight: high-priority CursorLine if fg is set.
01570f1 neovim#8726 terminal: handle &confirm and :confirm on unloading
56065bb neovim#8721 screen: truncate showmode messages
bf2460e neovim#7551 buffer: fix copying :setlocal options
c1c14fa neovim#8520 Ex mode: always "improved" (gQ)
050f397 neovim#7992 options: remove 'maxcombine` option (always 6)

INTERNAL:
463da84 neovim#7992 screen: use UTF-8 representation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment