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

external UI: tabline #6583

Merged
merged 4 commits into from Apr 26, 2017

Conversation

Projects
None yet
3 participants
@justinmk
Member

justinmk commented Apr 25, 2017

Continues #6170

Now that :help api-contract formalizes how we can grow the API, we can accelerate UI widget "externalization". We don't need to perfectly implement the UI events because :help api-contract allows for adding new items to existing events.

#6170 is simple enough that it's worth including in 0.2.

Todo (future):

  • tabline click events (StlClickRecord) aren't handled by this change. See #6170 (comment)

cc @bfredl

@justinmk justinmk force-pushed the justinmk:ui-tabline branch from d41f6ee to 761682d Apr 25, 2017

@@ -264,10 +264,13 @@ a dictionary with these (optional) keys:
colors.
Set to false to use terminal color codes (at
most 256 different colors).
`popupmenu_external` Instead of drawing the completion popupmenu on

This comment has been minimized.

@justinmk

justinmk Apr 25, 2017

Member

Un-document popupmenu_external but continue to support it. We can try removing it on master after 0.2 and see if it breaks anything.

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

But why?

This comment has been minimized.

@justinmk

justinmk Apr 25, 2017

Member

Do you mean why remove it? I am not sure if anyone is actually using it, yet. OTOH 0.1.7 will be in debian stable, so yeah it's not a good idea to remove it.

@justinmk justinmk force-pushed the justinmk:ui-tabline branch from 761682d to fc99d82 Apr 25, 2017

if (value.type != kObjectTypeBoolean) {
api_set_error(error, kErrorTypeValidation,
"popupmenu_external must be a Boolean");
return;
}
ui->pum_external = value.data.boolean;
ui->ui_ext[kUIPopupmenu] = value.data.boolean;
} else if (strequal(name.data, "ui_ext")) {

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

Why reimplement ui options inside ui options? Inner platform effect?

This comment has been minimized.

@justinmk

justinmk Apr 25, 2017

Member

Would you prefer a bunch of ext_foo fields? I'm fine with that, I suppose, the pesudo-namespacing seemed like a signal that they shouldn't be separate options. Also the documentation for each one will be very redundant.

This comment has been minimized.

@justinmk

justinmk Apr 25, 2017

Member

Flattened the options. I'm generally an advocate of avoiding needless hierarchy :)

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

Because ui options was introduced for smart ui (that is, external widgets) in the first place, so it seems weird to introduce a sub-option for smart ui options... It seems we will end up with two options, rgb, and ui_ext containing the rest of the options....

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

(note I'm talking of the external interface, I'm not against ui->ui_ext)

win_T *cwp = (tp == curtab) ? curwin : tp->tp_curwin;
get_trans_bufname(cwp->w_buffer);
Array tab = ARRAY_DICT_INIT;
ADD(tab, INTEGER_OBJ(tp->handle));

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

TABPAGE_OBJ

This comment has been minimized.

@justinmk

justinmk Apr 25, 2017

Member

If I do that, the test result looks like this:

[tab] = {
[id] = '' }

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

That is purely a limit of neovim-lua objects not printing themselves properly. This only affects debug output, not if the test passes or not. Compare with nvim.Tabpage.new(1) etc.

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

or compare against objects from nvim('list_tabpages'), this is what the API tests do.

This comment has been minimized.

@justinmk

justinmk Apr 25, 2017

Member

Thanks. Seems awkward. How does it help clients?

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

It is awkward due to limitations in neovim-lua. In the python-client (and similarly in other plugin hosts) you get a useful object you can call methods on, assign to nvim.current.tabpage etc. The lua client should at least be fixed to decode the id so that it prints like {[id]=1}.

This comment has been minimized.

@bfredl

This comment has been minimized.

@justinmk

justinmk Apr 25, 2017

Member

@bfredl Thanks! That worked nicely.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 25, 2017

Is nvim_set_current_tabpage roughly equivalent to clicking on the tabline? If we provide no other means, external ui:s are going to use it.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 25, 2017

It is a problem though that ui options aren't introspectible. It will be an error if a newer gui tries to enable a widget in an older nvim. Maybe the metadata should contain the supported ui options?

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 25, 2017

nvim_set_current_tabpage and ex_tabnext both resolve to goto_tabpage_tp so I think we're ok there. However there's still the mouse-click issue mentioned in #6583 (comment).

It will be an error if a newer gui tries to enable a widget in an older nvim.

Yeah, I thought about silently ignoring unknown options but I think it's better for UIs to explicitly check api_info.version.

Maybe the metadata should contain the supported ui options

That's also a good idea, though in practice most clients probably are fine with checking api_info.version.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 25, 2017

That will work for releases, but not for master version. Yes, users of master gui/plugins should use master nvim, but it nice to be able to easily avoid one annoying error.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 25, 2017

Supporting 'tabline' in general is going to be tricky. But as this PR uses a dict, it can be done backwards-compatible later on.

FOR_ALL_TABS(tp) {
win_T *cwp = (tp == curtab) ? curwin : tp->tp_curwin;
get_trans_bufname(cwp->w_buffer);
Array tab = ARRAY_DICT_INIT;

This comment has been minimized.

@bfredl

bfredl Apr 25, 2017

Member

this array seems redundant, can't we have a tabpage field in the dict?

@justinmk justinmk force-pushed the justinmk:ui-tabline branch 2 times, most recently from f73b70c to 907a8b8 Apr 26, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 26, 2017

Updated, much better thanks to suggestions.

@justinmk justinmk force-pushed the justinmk:ui-tabline branch 2 times, most recently from 5f88224 to 7bd63be Apr 26, 2017

if (ui_is_external(kUITabline)) {
ui_ext_tabline_update();
return;
}

This comment has been minimized.

@justinmk

justinmk Apr 26, 2017

Member

@dzhou121 I moved this below redraw_tabline = false to avoid unnecessary draw_tabline() calls. Did you have it swapped on purpose?

This comment has been minimized.

@dzhou121

dzhou121 Apr 26, 2017

Contributor

No, it wasn't on purpose. It should be below redraw_tabline = false

justinmk added some commits Apr 25, 2017

api/ui: externalize tabline
- Work with a bool[] array parallel to the UIWidget enum.
- Rename some functions.
- Documentation.

@justinmk justinmk force-pushed the justinmk:ui-tabline branch from 7bd63be to 6944aba Apr 26, 2017

@justinmk justinmk merged commit 0b59f98 into neovim:master Apr 26, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
QuickBuild Build pr-6583 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage decreased (-0.04%) to 76.253%
Details

@justinmk justinmk deleted the justinmk:ui-tabline branch Apr 26, 2017

Array tabs = ARRAY_DICT_INIT;
FOR_ALL_TABS(tp) {
Dictionary tab_info = ARRAY_DICT_INIT;
PUT(tab_info, "tab", TABPAGE_OBJ(tp->handle));

This comment has been minimized.

@dzhou121

dzhou121 Apr 27, 2017

Contributor

@justinmk I just tested this on my remote ui. But TABPAGE_OBJ seems to break the msgpack, i.e., I can't decode the msgpack messages properly. And it's fine when I change it back to INTEGER_OBJ.

This comment has been minimized.

@bfredl

bfredl Apr 27, 2017

Member

You need an msgpack library capable of extension types. The EXT value can either be sent as-is to any method expecting a tabpage, or the internal data can be decoded as a msgpack INT.

This comment has been minimized.

@dzhou121

dzhou121 Apr 27, 2017

Contributor

I'm using github.com/neovim/go-client, does it support it?

This comment has been minimized.

@bfredl

bfredl Apr 27, 2017

Member

Hmm, looks like it should...

This comment has been minimized.

@justinmk

justinmk Apr 27, 2017

Member

@dzhou121 could you report at the go client repo?

This comment has been minimized.

@dzhou121

dzhou121 Apr 27, 2017

Contributor

Thanks for the info. Have reported it at the go client repo.

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas, packages.

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

@justinmk justinmk referenced this pull request May 1, 2017

Merged

NVIM v0.2.0 #6634

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

@justinmk justinmk added the ui-ext label Jul 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment