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

terminal: Account for number column in terminal (#5310) #7440

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@tecywiz121
Contributor

tecywiz121 commented Oct 25, 2017

This is my attempt at fixing Issue #5310: Terminal split and relativenumber cuts off the end of lines

This is my first neovim PR, so be gentle, but I would appreciate feedback.

In particular I'm not sure if I should be using w_nrwidth since it's a cached value. There's two other bugs I'd like to fix, but I'm not sure how to approach them:

  • Turning number on/off doesn't cause the terminal to be resized
  • The terminal is created with 80 columns, regardless of the size of the window.
int width = wp->w_width;
if (wp->w_p_nu || wp->w_p_rnu) {
width -= wp->w_nrwidth + 1;
}

This comment has been minimized.

@justinmk

justinmk Oct 25, 2017

Member

Is this how it looks in other code that deals with w_nrwidth?

This comment has been minimized.

@tecywiz121

tecywiz121 Oct 25, 2017

Contributor

This is an example of how w_nrwidth is calculated:

i = (wp->w_p_nu || wp->w_p_rnu) ? number_width(wp) : 0;

I don't actually see any uses of it besides determining if a redraw needs to happen.

This comment has been minimized.

@justinmk

justinmk Oct 25, 2017

Member

Would win_col_off() work instead? E.g.

    terminal_resize(buf->terminal,
                    (uint16_t)(MAX(0, curwin->w_width - win_col_off(curwin))),
                    (uint16_t)curwin->w_height)

I think this should be done at each call site, instead of adding a new terminal_resize_into function. Because many of those call sites are going to be removed later, for other reasons.

This comment has been minimized.

@tecywiz121

tecywiz121 Oct 25, 2017

Contributor

That's actually how I did it originally. I'll switch back to doing it that way.

@@ -4923,7 +4923,7 @@ void scroll_to_fraction(win_T *wp, int prev_height)
invalidate_botline_win(wp);
if (wp->w_buffer->terminal) {
terminal_resize(wp->w_buffer->terminal, 0, wp->w_height);
terminal_resize_into(wp->w_buffer->terminal, wp);

This comment has been minimized.

@justinmk

justinmk Oct 25, 2017

Member

0 has a special meaning here, so the logic changed now. Same for win_new_width.

@tecywiz121 tecywiz121 force-pushed the tecywiz121:term-width branch from 2ac1dc2 to 22c78bf Oct 25, 2017

@tecywiz121

This comment has been minimized.

Contributor

tecywiz121 commented Oct 25, 2017

Alright, I think I've got your comments @justinmk handled.

There are three other cases where the terminal isn't resized correctly that I've found:

  • Immediately after :terminal
  • When the number of digits in the line number changes
  • Toggling number, or relativenumber

Any pointers where to add the terminal_resize calls? I was thinking around here.

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 25, 2017

Immediately after :terminal

The assignments from curwin->w_width in f_termopen need to be adjusted.

When the number of digits in the line number changes
Toggling number, or relativenumber

I guess screen.c will need to be changed where you mentioned.

@tecywiz121 tecywiz121 force-pushed the tecywiz121:term-width branch from 22c78bf to 7119746 Oct 26, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 27, 2017

Just needs a test, then we can try it.

@tecywiz121 tecywiz121 force-pushed the tecywiz121:term-width branch from 18eade6 to f4b4049 Oct 27, 2017

@tecywiz121

This comment has been minimized.

Contributor

tecywiz121 commented Oct 27, 2017

So I added a functional test for it. Really neat framework! I didn't find any documentation, but I think I trial-and-errored my way into a test that passes with my patch, and fails without.

@tecywiz121 tecywiz121 force-pushed the tecywiz121:term-width branch from f4b4049 to 9fb61f0 Oct 28, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 28, 2017

Merged. Thanks @tecywiz121 !

@justinmk justinmk closed this Oct 28, 2017

justinmk added a commit that referenced this pull request Oct 28, 2017

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in neovim#4411
5b32bce Windows: `:terminal` neovim#7007
7b0ceb3 UI/API: externalize cmdline neovim#7173
b67f58b UI/API: externalize wildmenu neovim#7454
b23aa1c UI: 'winhighlight' neovim#6597
17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364
244a1f9 API: execute lua directly from the remote api neovim#6704
45626de API: `get_keymap()` neovim#6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082
dc68538 menu_get() function neovim#6322
9db42d4 :cquit : take an error code argument neovim#7336
9cc185d job-control: serverstart(): support ipv6 neovim#6680
1b7a9bf job-control: sockopen() neovim#6594
6efe84a clipboard: fallback to tmux clipboard neovim#6894
6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841
16cce1a debug: $NVIM_LOG_FILE neovim#6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize neovim#6816
cb912a3 :terminal : handle F1-F12, other keys neovim#7241
619838f inccommand: improve performance neovim#6949
04b3c32 inccommand: Fix matches for zero-width neovim#7487
60b1e8a inccommand: multiline, other fixes neovim#7315
f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967
1551f71 inccommand: fix 'gdefault' lockup neovim#7262
6338199 API: bufhl: support creating new groups neovim#7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' neovim#7440
5bec946 UI: preserve wildmenu during jobs/events neovim#7110
c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259
51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221
133f8bc shada: preserve unnamed register on restart neovim#4700
1b70a1d shada: avoid assertion on corrupt shada file neovim#6958
9f534f3 mksession: Restore tab-local working directory neovim#6859
de1084f fix buf_write() crash neovim#7140
7f76986 syntax: register 'Normal' highlight group neovim#6973
6e7a8c3 RPC: close channel if stream was closed neovim#7081
85f3084 clipboard: disallow recursion; show hint only once neovim#7203
8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193
01487d4 'titleold' neovim#7358
01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349
0f2873c Windows: multibyte startup arguments neovim#7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode neovim#6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364
2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544
023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915
6720fe2 help: `K` tries Vim help instead of manpage neovim#3104
7068370 help, man.vim: change "outline" map to `gO` neovim#7405

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 13, 2017

:terminal : fix crash on resize
closes neovim#7538
Fix wrong window references from neovim#7440

Remove some eager resizing. Still mostly doesn't address neovim#4997.

justinmk added a commit that referenced this pull request Nov 13, 2017

:terminal : fix crash on resize (#7547)
closes #7538
Fix wrong window references from #7440

Remove some eager resizing. Still mostly doesn't address #4997.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment