Skip to content

Conversation

bfredl
Copy link
Member

@bfredl bfredl commented May 14, 2017

To implement stuff like #6742 in a forward-compatible fashion, @justinmk suggested an api function where embedders and connected clients might identify themselves and their capabilities. Before implementing it would be good to discuss what it should contain. Some suggestions

name
version: a dicitionary with "major", "minor", "commit", "prerelease" etc keys (all optional)
role: informal description of the clients role, with some suggested values: host, plugin (for a single-plugin process), gui
methods: builtin methods (before loading plugins into the host), not sure how detailed, but at least argument count so methods can be extended.

There probably should be an autocommand or something so interested parties (like the rplugin infrastructure) could listen for identified clients. And a nvim_list_clients() function for introspection/debugging.

@justinmk
Copy link
Member

justinmk commented May 14, 2017

version: a dicitionary with "major", "minor", "commit", "prerelease" etc keys (all optional)

If the client sends this dict then "major" should be required (if "minor" were provided without "major" it could be a mistake).

role: informal description of the clients role, with some suggested values: host, plugin (for a single-plugin process), gui

Perhaps type, and make it formal: only certain values are allowed. When UIs connect we automatically set this to ui.

Informal description could live in a description field.

There probably should be an autocommand or something so interested parties (like the rplugin infrastructure) could listen for identified clients.

@teto did some work towards this. We discussed a generic "channel created" event.

Which reminds me, the thing holding us back from adding new events like this (instead of autocommands) is that we don't have guidelines for nvim events vs plugin events. Currently it's ad-hoc and any plugin could broadcast an event to all channels, which could cause bugs that are difficult to track. I have been thinking we may want to reserve a prefix for nvim core events, or devise some other way of having a reserved channel that only nvim can publish to (but all clients can subscribe to).

@bfredl
Copy link
Member Author

bfredl commented May 14, 2017

Bulitin events should start with nvim_. The trick is how we change redraw to nvim_redraw in a backwards-compatible fashion.

A complication is that internal events can be generated with different levels of async-ness:

  1. vimL can run freely (most autocommands)
  2. vimL can run with restrictions (textlock, sandbox etc)
  3. vimL must not be run, but lua should be allowed with restrictions ( a lua callback for nvim_redraw can draw "external ui widgets" on the TUI)
  4. time critical, vimL or lua must not be run right now, only send notifications, and maybe schedule vimL/lua to the next main loop iteration.

@bfredl
Copy link
Member Author

bfredl commented May 25, 2017

Perhaps type, and make it formal: only certain values are allowed. When UIs connect we automatically set this to ui.
Informal description could live in a description field.

Perhaps there should be an extensible dict for "informal" stuff (github weblink, license, fancy SVG logo, etc) ?

@bfredl bfredl changed the title idea: nvim_set_channel_info [WIP] nvim_set_channel_info May 25, 2017
@bfredl
Copy link
Member Author

bfredl commented May 25, 2017

Updated with some actual code. You can listen for ChannelInfo event or use the nvim_get_channels() function. Currently only for RPC, but is forward-compatible with also enumerating jobs and byte channels (when we get them) in the future.

@bfredl
Copy link
Member Author

bfredl commented Jun 3, 2017

@justinmk Implemention-wise this does what I need from it (except for some validation). The info is incomplete, but naturally extendable later on (extend to non-RPC channels, info about the process, the socket address etc). Any comments about the design or should I just finish it off with tests?

@bfredl
Copy link
Member Author

bfredl commented Jun 3, 2017

Another concern: In case of nvim-to-nvim connection later on the two instance should be able to do the rendez-vous automatically. Not implemented here, but we should make sure the design is forward compatible (and it must also support The Amazing Cat scenario).

@bfredl
Copy link
Member Author

bfredl commented Sep 7, 2017

Note to self: for pty channels show the /dev/pts/XX

@@ -226,12 +195,13 @@ void channel_create_event(Channel *chan, char *ext_source)
source = (char *)IObuff;
}

ILOG("new channel %" PRIu64 " (%s%s): %s", chan->id, stream_desc,
mode_desc, source);
ILOG("new channel %" PRIu64 " (%s%s): %s", chan->id, source);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many %s in the format string.

(void)ext_source;
#endif

channel_info_changed(chan);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used before declared?

@bfredl bfredl force-pushed the channel_info branch 3 times, most recently from 9eb57cc to de98bbc Compare December 13, 2017 17:41
@bfredl bfredl force-pushed the channel_info branch 5 times, most recently from 9c19e48 to 7420000 Compare February 12, 2018 17:29
@bfredl
Copy link
Member Author

bfredl commented May 7, 2018

@b-r-o-c-k I wonder if the indeterminism is related to CPU/IO load somehow. But the changed code (firing of the autocmd) happens after the return code is determined by process_spawn, so I don't understand why it should make a difference in the first place.

@bfredl
Copy link
Member Author

bfredl commented May 12, 2018

I tested reverting all code changes with latest push and this simple test still fails on appveyor (no autocmds or anything), sometimes on the first and sometimes on the second line:

eq(3, eval("jobstart(['cat'], {})"))
eq(4, eval("jobstart(['cat'], {})"))

So some racyness in process creation with jobstart('cat') is unrelated to the change of this PR. I would suggest to mark the cat test as pending on appveyor. (which is only one of several tests for the actual PR functionality, which really doesn't depend on connection type ). We will add tests for real world subprocess usage with the python client later.

@bfredl bfredl force-pushed the channel_info branch 2 times, most recently from 3fb2582 to 49adbe8 Compare May 12, 2018 14:57
@bfredl
Copy link
Member Author

bfredl commented May 13, 2018

Failure looks unrelated:

  [  ERROR   ] 1 error, listed below:
  [  ERROR   ] test\unit\viml\expressions\parser_tests.lua @ 6329: api nvim_parse_expression works with &opt
  C:/projects/neovim/test/functional\api\vim_spec.lua:1186: Error while processing test ('(1+&)', m):
CUSTOMBUILD : table error :  [C:\projects\neovim\build\functionaltest.vcxproj]
  C:/projects/neovim/test/functional\api\vim_spec.lua:1170: Expected objects to be the same.
  Passed in:
  (table) {
    [1] = 'NvimNestingParenthesis:0:0:('
    [2] = 'NvimNumber:0:1:1'
    [3] = 'NvimBinaryPlus:0:2:+'
    [4] = 'NvimInvalidOptionSigil:0:3:&'
   *[5] = 'NvimInvalidOptionScope:0:4:)'
    [6] = 'NvimInvalidOptionScopeDelimiter:0:5:'
    [7] = 'NvimInvalidOptionName:0:6:'
    [8] = 'NvimNestingParenthesis:0:4:)' }
  Expected:
  (table) {
    [1] = 'NvimNestingParenthesis:0:0:('
    [2] = 'NvimNumber:0:1:1'
    [3] = 'NvimBinaryPlus:0:2:+'
    [4] = 'NvimInvalidOptionSigil:0:3:&'
   *[5] = 'NvimNestingParenthesis:0:4:)' }
  
  ({message = 'C:/projects/neovim/test/functional\\api\\vim_spec.lua:1170: Expected objects to be the same.\nPassed in:\n(table) {\n  [1] = \'NvimNestingParenthesis:0:0:(\'\n  [2] = \'NvimNumber:0:1:1\'\n  [3] = \'NvimBinaryPlus:0:2:+\'\n  [4] = \'NvimInvalidOptionSigil:0:3:&\'\n *[5] = \'NvimInvalidOptionScope:0:4:)\'\n  [6] = \'NvimInvalidOptionScopeDelimiter:0:5:\'\n  [7] = \'NvimInvalidOptionName:0:6:\'\n  [8] = \'NvimNestingParenthesis:0:4:)\' }\nExpected:\n(table) {\n  [1] = \'NvimNestingParenthesis:0:0:(\'\n  [2] = \'NvimNumber:0:1:1\'\n  [3] = \'NvimBinaryPlus:0:2:+\'\n  [4] = \'NvimInvalidOptionSigil:0:3:&\'\n *[5] = \'NvimNestingParenthesis:0:4:)\' }'})
  
  stack traceback:
  	C:/projects/neovim/test/functional\api\vim_spec.lua:1186: in function 'check_parsing'
  	test\unit\viml\expressions\parser_tests.lua:6475: in function <test\unit\viml\expressions\parser_tests.lua:6329>

@justinmk
Copy link
Member

  [  ERROR   ] test\unit\viml\expressions\parser_tests.lua @ 6329: api nvim_parse_expression works with &opt
  C:/projects/neovim/test/functional\api\vim_spec.lua:1186: Error while processing test ('(1+&)', m):

Haven't seen that failure before, and meths.parse_expression should be deterministic...

@justinmk justinmk added this to the 0.3.1 milestone May 14, 2018
@bfredl
Copy link
Member Author

bfredl commented May 20, 2018

@justinmk any reason not to have this in 0.3?

@justinmk
Copy link
Member

@bfredl No reason, would like to take one more look at it later today, feel free to merge otherwise.

@bfredl bfredl force-pushed the channel_info branch 2 times, most recently from 04d8fce to fe3a2cd Compare May 21, 2018 19:36
@bfredl
Copy link
Member Author

bfredl commented May 22, 2018

This travis ASAN failure is a bit weird:

0.66s$ sudo -E apt-get -yq --no-install-suggests --no-install-recommends --force-yes install autoconf automake apport build-essential clang cmake cscope g++-multilib gcc-multilib gdb language-pack-tr libc6-dev-i386 libtool locales ninja-build pkg-config unzip valgrind xclip
Reading package lists...
Building dependency tree...
Reading state information...
autoconf is already the newest version (2.69-6).
automake is already the newest version (1:1.14.1-2ubuntu1).
build-essential is already the newest version (11.6ubuntu6).
libtool is already the newest version (2.4.2-1.7ubuntu1).
pkg-config is already the newest version (0.26-1ubuntu4).
pkg-config set to manually installed.
unzip is already the newest version (6.0-9ubuntu1.5).
locales is already the newest version (2.13+git20120306-12.1).
Some packages could not be installed. This may mean that you have
requested an impossible situation or if you are using the unstable
distribution that some required packages have not yet been created
or been moved out of Incoming.
The following information may help to resolve the situation:
The following packages have unmet dependencies:
 g++-multilib : Depends: g++-4.8-multilib (>= 4.8.2-5~) but it is not going to be installed
 gcc-multilib : Depends: gcc-4.8-multilib (>= 4.8.2-5~) but it is not going to be installed
E: Unable to correct problems, you have held broken packages.

A 64-bit build shouldn't need multilib, no?

Fire autocmd when channel opens or its info changes.
Add a way for API clients can describe themselves.
@bfredl bfredl merged commit 418abfc into neovim:master May 23, 2018
justinmk added a commit that referenced this pull request Jun 11, 2018
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
vimpostor added a commit to vimpostor/vim-tpipeline that referenced this pull request Jul 9, 2022
Obviously if GUI vim is started outside of tmux, we always already
disabled this plugin, since $TMUX would be empty then.
But if someone starts GUI vim from the command line within tmux, then
$TMUX would not be empty and the plugin would become enabled which would
look very weird, because we then embed the GUI vim statusline in a
remote tmux console window.

For gVim fixing this is very easy, as we can simply check for
has('gui_running').

Unfortunately, this variable will probably not work in neovim GUIs,
since the neovim GUI only attaches after vimrc has already been sourced,
so has('gui_running') will return 0 even though the GUI will attach
later in the startup process.

We do not want to use neovim-qt specific variables like g:GuiLoaded [1].
For example gonvim uses another variable [2].

This is where things get more complicated.
It seems like the only reliable way to detect it is to use the UIEnter
autocmd [3], which luckily exists since 2017 [4].

There is also a way to identify neovim clients over RPC [5], but using
autocmds is probably much easier.

Therefore to solve the problem, we disable the plugin on UIEnter, which
we do with the new tpipeline#state#restore() function added in
a95ccb6.

Theoretically we could also reload the plugin again on UILeave, but in
reality I don't see anyone ever needing to do this.

We could also add an option to keep the plugin enabled for GUIs if
someone wants to have that feature, but let's do that in the future if
that usecase ever comes up, which it probably won't (yes I am jinxing
it).
At least we now have sane defaults for GUI vim.

Fixes #29.

[1] equalsraf/neovim-qt#219
[2] dzhou121/gonvim#17
[3] neovim/neovim#3646
[4] neovim/neovim#6917
[5] neovim/neovim#6743
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants