[RFC] terminal: resize to the max viewed size (#8096). #8249

mrbiggfoot · 2018-04-08T21:35:01Z

When a window is split, the same terminal instance is owned by both windows. The terminal size may be set to match the smaller window size, which results in bugs like #8096. This fix makes sure the terminal dimensions match the largest window.

justinmk · 2018-04-13T01:09:55Z

src/nvim/terminal.c

@@ -359,6 +359,15 @@ void terminal_resize(Terminal *term, uint16_t width, uint16_t height)
    return;
  }

+  FOR_ALL_TAB_WINDOWS(tp, wp) {


@mrbiggfoot this will loop through all tabs, is that intended? To loop only the current tab:

FOR_ALL_WINDOWS_IN_TAB(wp, curtab)

But the same terminal instance may be open in windows on multiple tabs, right? So yes, this is intended.

justinmk · 2018-04-13T01:12:43Z

The change here isn't really correct as-is. Besides the failed builds, it also ignores explicit jobresize().

mrbiggfoot · 2018-04-13T01:51:19Z

@justinmk I tried to comprehend the nature of the build failures, but it did not look like it was related to my change. Also, almost all PRs were failing builds when I looked. Can you please point the specific test that my change breaks?

Also, I don't think jobresize() should be affected by this change since jobresize is a manual call. And I'm a firm believer that shooting yourself in the foot is a constitutional right.

justinmk · 2018-04-13T02:03:18Z

Also, almost all PRs were failing builds when I looked

There are some unreliable tests, but no PR gets merged if it fails every build.

The build log shows the error (it's not failing on master, nor any other PR, it's a legitimate failure):

[1m�[31m[  ERROR   ]�[0m�[0m �[1m2�[0m errors, listed below:
�[1m�[31m[  ERROR   ]�[0m�[0m �[36m...ld/neovim/neovim/test/functional/terminal/mouse_spec.lua�[0m @ �[36m98�[0m: �[1mterminal mouse when the terminal has focus with a split window and other buffer before_each�[0m
test/functional/ui/screen.lua:307: Row 4 did not match.
Expected:
  |line28                   â”‚line28                  |
  |line29                   â”‚line29                  |
  |line30                   â”‚line30                  |
  |*rows: 5, cols: 24        â”‚rows: 5, cols: 24       |
  |{2:^ }                        â”‚{2: }                       |
  |==========                ==========              |
  |:vsp                                              |
Actual:
  |line28                   â”‚line28                  |
  |line29                   â”‚line29                  |
  |line30                   â”‚line30                  |
  |*rows: 5, cols: 25        â”‚rows: 5, cols: 25       |
  |{2:^ }                        â”‚{2: }                       |
  |==========                ==========              |
  |:vsp                                              |

To print the expect() call that would assert the current screen state, use
screen:snapshot_util(). In case of non-deterministic failures, use
screen:redraw_debug() to show all intermediate screen states.  

stack traceback:
	test/functional/ui/screen.lua:307: in function 'wait'
	test/functional/ui/screen.lua:221: in function 'expect'
	...ld/neovim/neovim/test/functional/terminal/mouse_spec.lua:100: in function <...ld/neovim/neovim/test/functional/terminal/mouse_spec.lua:98>

�[1m�[31m[  ERROR   ]�[0m�[0m �[36m...ld/neovim/neovim/test/functional/terminal/mouse_spec.lua�[0m @ �[36m98�[0m: �[1mterminal mouse when the terminal has focus with a split window and other buffer before_each�[0m
test/functional/ui/screen.lua:307: Row 4 did not match.
Expected:
  |line28                   â”‚line28                  |
  |line29                   â”‚line29                  |
  |line30                   â”‚line30                  |
  |*rows: 5, cols: 24        â”‚rows: 5, cols: 24       |
  |{2:^ }                        â”‚{2: }                       |
  |==========                ==========              |
  |:vsp                                              |
Actual:
  |line28                   â”‚line28                  |
  |line29                   â”‚line29                  |
  |line30                   â”‚line30                  |
  |*rows: 5, cols: 25        â”‚rows: 5, cols: 25       |
  |{2:^ }                        â”‚{2: }                       |
  |==========                ==========              |
  |:vsp                                              |

Take a look at cc8f477 for hints on what needs to be done. It was partially reverted a few months ago, but I think we should restore it, but only for height. We need to avoid shrinking terminal width, it truncates text in a non-interactive program (libvterm limitation).

justinmk · 2018-04-13T02:23:12Z

@mrbiggfoot actually, let's just fix the failing test and try this out as-is. I think the test will pass if you change both cases of cols: 24 to cols: 25.

There are more "subtle" behavior improvements needed for :terminal, but any (experimental) improvement is better than nothing for now.

mrbiggfoot · 2018-04-13T05:34:17Z

@justinmk I had to change 24 to 25 in a bunch of places to make the test pass. I admit here, loud and clear, that I did a monkey job with the unit test and I don't understand why that change was needed.

mrbiggfoot · 2018-04-13T08:29:10Z

...s/build/neovim/neovim/test/functional/ui/output_spec.lua:55: [Unreliable on Travis macOS.]^M
ESC[1mESC[31m[  ERROR   ]ESC[0mESC[0m ESC[1m1ESC[0m error, listed below:^M
ESC[1mESC[31m[  ERROR   ]ESC[0mESC[0m ESC[36mtest/functional/helpers.luaESC[0m @ ESC[36m800ESC[0m: ESC[1mafter_eachESC[0m^M
test/helpers.lua:251: crash detected (see above)^M
^M
stack traceback:^M
        test/helpers.lua:251: in function 'check_cores'^M
        test/functional/helpers.lua:805: in function <test/functional/helpers.lua:800>^M

[ RUN      ] 'spell' joins long lines #7937: ERR
test/functional/ui/screen.lua:307: Row 1 did not match.
Expected:
  |*{1:^Lorem} {1:ipsum} dolor sit {1:amet}, {1:consectetur} {1:adipiscing} {1:elit}, {1:sed} do {1:eiusmod} {1:tempor} {1:i}|
  |{1:ncididunt} {1:ut} {1:labore} {1:et} {1:dolore} {1:magna} {1:aliqua}. {1:Ut} {1:enim} ad minim {1:veniam}, {1:quis} {1:nostru}|
  |{1:d} {1:exercitation} {1:ullamco} {1:laboris} {1:nisi} {1:ut} {1:aliquip} ex ea {1:commodo} {1:consequat}. {1:Duis} {1:aut}|
  |{1:e} {1:irure} dolor in {1:reprehenderit} in {1:voluptate} {1:velit} {1:esse} {1:cillum} {1:dolore} {1:eu} {1:fugiat} {1:n}|
  |{1:ulla} {1:pariatur}. {1:Excepteur} {1:sint} {1:occaecat} {1:cupidatat} non {1:proident}, {1:sunt} in culpa {1:qui}|
  | {1:officia} {1:deserunt} {1:mollit} {1:anim} id est {1:laborum}.                                   |
  |{0:~                                                                               }|
  |                                                                                |
Actual:
  |*                                                                                |
  |{0:~                                                                               }|
  |{0:~                                                                               }|
  |{0:~                                                                               }|
  |{0:~                                                                               }|
  |{0:~                                                                               }|
  |{0:~                                                                               }|
  |^                                                                                |
To print the expect() call that would assert the current screen state, use
screen:snapshot_util(). In case of non-deterministic failures, use
screen:redraw_debug() to show all intermediate screen states.  
stack traceback:
	test/functional/ui/screen.lua:307: in function 'wait'
	test/functional/ui/screen.lua:221: in function 'expect'
	...is/build/neovim/neovim/test/functional/ui/spell_spec.lua:38: in function <...is/build/neovim/neovim/test/functional/ui/spell_spec.lua:27>

Does it look related @justinmk ?

justinmk · 2018-04-13T11:27:56Z

no, see #8102

mrbiggfoot · 2018-04-13T20:42:13Z

FYI - here's how similar issue was fixed in vim:
vim/vim@98da6ec

justinmk · 2018-04-14T09:12:23Z

@mrbiggfoot so... can you try the patch in nvim and see if it fixes your issue?

mrbiggfoot · 2018-04-14T10:30:57Z

Already tried. It does not.

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

terminal: resize to the max viewed size (neovim#8096).

4f458af

mrbiggfoot force-pushed the issue8096 branch from fc4c5c5 to 4f458af Compare April 8, 2018 21:53

mrbiggfoot mentioned this pull request Apr 8, 2018

Incorrect fixed-height window behavior #8096

Closed

marvim added the RFC label Apr 8, 2018

justinmk reviewed Apr 13, 2018

View reviewed changes

Fix mouse_spec.lua test.

922c7f4

justinmk added this to the 0.3 milestone Apr 13, 2018

javian mentioned this pull request Apr 15, 2018

logging: invalid msgpack-rpc string #8102

Open

justinmk merged commit 84359a4 into neovim:master Apr 16, 2018

justinmk removed the RFC label Apr 16, 2018

mrbiggfoot deleted the issue8096 branch April 17, 2018 00:09

mrbiggfoot mentioned this pull request Apr 29, 2018

:terminal : set topline based on window height #8325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[RFC] terminal: resize to the max viewed size (#8096). #8249

[RFC] terminal: resize to the max viewed size (#8096). #8249

Uh oh!

mrbiggfoot commented Apr 8, 2018

Uh oh!

justinmk Apr 13, 2018

Uh oh!

mrbiggfoot Apr 13, 2018

Uh oh!

justinmk commented Apr 13, 2018

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

justinmk commented Apr 13, 2018

Uh oh!

justinmk commented Apr 13, 2018 •

edited

Loading

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

justinmk commented Apr 13, 2018

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

justinmk commented Apr 14, 2018

Uh oh!

mrbiggfoot commented Apr 14, 2018

Uh oh!

Uh oh!

Uh oh!

[RFC] terminal: resize to the max viewed size (#8096). #8249

[RFC] terminal: resize to the max viewed size (#8096). #8249

Uh oh!

Conversation

mrbiggfoot commented Apr 8, 2018

Uh oh!

justinmk Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

mrbiggfoot Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

justinmk commented Apr 13, 2018

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

justinmk commented Apr 13, 2018

Uh oh!

justinmk commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

justinmk commented Apr 13, 2018

Uh oh!

mrbiggfoot commented Apr 13, 2018

Uh oh!

justinmk commented Apr 14, 2018

Uh oh!

mrbiggfoot commented Apr 14, 2018

Uh oh!

Uh oh!

justinmk commented Apr 13, 2018 •

edited

Loading