[RFC] api: Make nvim_set_option() update `:verbose set ...` #8329

b-r-o-c-k · 2018-04-30T02:21:23Z

Make :verbose set ... show when an option was last modified by an API client or :lua{do,file,} command. It seems the only way of identifying an API client is by its channel ID. I wonder if it would be a good idea to add a function for API clients to specify a name for themselves with, such as nvim_set_client_name().

bfredl · 2018-04-30T04:33:52Z

I wonder if it would be a good idea to add a function for API clients to specify a name for themselves with, such as nvim_set_client_name().

This would be #6743, I'll just need to fix the tests on windows.

justinmk · 2018-05-01T11:25:45Z

src/nvim/option_defs.h

@@ -821,4 +821,10 @@ enum {

 #define SB_MAX 100000  // Maximum 'scrollback' value.

+/// Stores an identifier of a script or channel that last set an option.
+typedef struct {
+  int script_id;           /// Script ID or one of SID_* special values.


Any reason not to use scid_T here?

justinmk · 2018-05-01T11:29:50Z

src/nvim/option.c

@@ -196,7 +196,7 @@ typedef struct vimoption {
  idopt_T indir;                /* global option: PV_NONE;
                                 * local option: indirect option index */
  char_u      *def_val[2];      /* default values for variable (vi and vim) */
-  scid_T scriptID;              /* script in which the option was last set */
+  LastSet last_set;             /* script in which the option was last set */


I would add a new field vimoption_T.chan_id rather than defining a LastSet type.

LastSet is also used in buffer_defs.h for winopt_T and file_buffer. Should I add the field uint64_t chan_ids[] to those structs as well?

Right again, your way is better. Cancel my suggestion.

justinmk · 2018-05-01T11:35:00Z

src/nvim/api/private/helpers.c

  }
+
+  const scid_T save_current_SID = current_SID;
+  current_SID = channel_id == LUA_INTERNAL_CALL ? SID_LUA : SID_API_CLIENT;


I guess is_internal_call() would not make sense here.

In the future I think we should somehow track the Lua function name or minimal stacktrace.

justinmk · 2018-05-01T11:37:30Z

src/nvim/api/private/helpers.c

+
+  const scid_T save_current_SID = current_SID;
+  current_SID = channel_id == LUA_INTERNAL_CALL ? SID_LUA : SID_API_CLIENT;
+  current_channel_id = channel_id;


Is it too much work to pass the channel id through the function calls, instead of a global?

No, I was just trying to avoid making lots of changes. If you're alright with making changes to all of the following functions and their callers, then I can do that instead of adding a global:

set_option_to set_option_value_for set_option_value_err set_option_value set_string_option did_set_string_option set_option_scriptID_idx

@b-r-o-c-k I retract.

~~A future improvement would be to organize these "context" globals in a single struct which can be save/restored with a single assignment.~~ That also doesn't make sense, because a script context is orthogonal to a channel context.

Probably we should set current_channel_id from channel.c itself (in a future PR), instead of only in the context of setting options.

justinmk · 2018-05-01T11:38:27Z

Can you add a couple tests to test/functional/api/vim_spec.lua in the describe('nvim_get_option, nvim_set_option', section? You can run just that test locally, like this:

TEST_FILE=test/functional/api/vim_spec.lua make functionaltest

See also https://github.com/neovim/neovim/blob/master/test/README.md

jamessan · 2018-05-01T13:16:58Z

src/nvim/api/buffer.c

@@ -581,7 +581,8 @@ Object nvim_buf_get_option(Buffer buffer, String name, Error *err)
 /// @param name       Option name
 /// @param value      Option value
 /// @param[out] err   Error details, if any
-void nvim_buf_set_option(Buffer buffer, String name, Object value, Error *err)
+void nvim_buf_set_option(uint64_t channel_id, Buffer buffer,


Why does the prototype need to be changed? I don't see anything calling nvim_buf_set_option with the new argument.

Isn't there a way we can determine/propagate the id automatically? If not, then there will need to be some changes to avoid breaking old API clients.

Here's an initial color for the bikeshed. Feel free to suggest alternatives:

void nvim_buf_set_option_for_channel(uint64_t channel_id, ...) FUNC_API_SINCE(4) { ... } void nvim_buf_set_option(Buffer buffer, ...) FUNC_API_SINCE(1) { nvim_buf_set_option_for_channel(0, ...); }

uint64_t channel_id is automatically resolved by the RPC layer, it doesn't change the signature for clients.

Ah, ok. Thanks.

uint64_t channel_id is automatically resolved by the RPC layer, it doesn't change the signature for clients.

Is there somewhere this sort of information is documented other than in the details of the generator scripts?

Make `:verbose set ...` show when an option was last modified by an API client or Lua script/chunk. In the case of an API client, the channel ID is displayed.

justinmk · 2018-05-10T10:50:29Z

src/nvim/eval.c

-  if (scriptID != 0) {
-    char_u *p = home_replace_save(NULL, get_scriptname(scriptID));
+  LastSet last_set;
+  last_set.script_id = scriptID;


It's good practice to fully initialize the struct (if it's an input):

LastSet last_set = (LastSet){ .script_id = scriptID, .channel_id = 0, };

justinmk · 2018-05-10T10:56:49Z

src/nvim/globals.h


 /* ID of script being sourced or was sourced to define the current function. */
 EXTERN scid_T current_SID INIT(= 0);
+/* ID of the current channel making a client API call */


run make lint locally and it will mention lines +/- 1 lines near any lines that were changed. Usually, lint changes are done in a separate commit titled simply lint.

justinmk · 2018-05-10T10:57:29Z

src/nvim/globals.h

@@ -397,9 +397,13 @@ EXTERN int garbage_collect_at_exit INIT(= FALSE);
 #define SID_ENV         -4      /* for sourcing environment variable */
 #define SID_ERROR       -5      /* option was reset because of an error */
 #define SID_NONE        -6      /* don't set scriptID */
+#define SID_LUA         -7      /* for Lua scripts/chunks */
+#define SID_API_CLIENT  -8      /* for API clients */


make lint cascades to nearby lines (+/- 1), so it will ask for the lines up through line 393 to be changed to // comments.

justinmk · 2018-05-10T11:19:49Z

@b-r-o-c-k To summarize, only thing remaining here is a lint commit. No logic/structural changes needed.

justinmk · 2018-05-11T08:17:18Z

Thanks! I fixed a couple remaining lint issues in the merge-commit.

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

marvim added the RFC label Apr 30, 2018

b-r-o-c-k force-pushed the last-set branch 3 times, most recently from b8f12ad to 5a5ff2a Compare April 30, 2018 16:35

justinmk reviewed May 1, 2018

View reviewed changes

jamessan reviewed May 1, 2018

View reviewed changes

b-r-o-c-k force-pushed the last-set branch from 5a5ff2a to f895ceb Compare May 4, 2018 02:01

api: Make nvim_set_option() update :verbose set ...

7170de1

Make `:verbose set ...` show when an option was last modified by an API client or Lua script/chunk. In the case of an API client, the channel ID is displayed.

b-r-o-c-k force-pushed the last-set branch from f895ceb to 7170de1 Compare May 4, 2018 02:05

justinmk reviewed May 10, 2018

View reviewed changes

lint

e31d8ed

b-r-o-c-k force-pushed the last-set branch from 9db15fd to e31d8ed Compare May 11, 2018 03:32

justinmk merged commit e31d8ed into neovim:master May 11, 2018

justinmk added a commit that referenced this pull request May 11, 2018

Merge #8329 'API: Make nvim_set_option() update :verbose set …'

273d2cd

b-r-o-c-k deleted the last-set branch May 11, 2018 12:55

justinmk mentioned this pull request Sep 8, 2019

"paste" option being set unintentional #10966

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] api: Make nvim_set_option() update `:verbose set ...` #8329

[RFC] api: Make nvim_set_option() update `:verbose set ...` #8329

b-r-o-c-k commented Apr 30, 2018

bfredl commented Apr 30, 2018

justinmk May 1, 2018

justinmk May 1, 2018

b-r-o-c-k May 1, 2018 •

edited

Loading

justinmk May 10, 2018

justinmk May 1, 2018

justinmk May 1, 2018

b-r-o-c-k May 1, 2018

justinmk May 10, 2018 •

edited

Loading

justinmk commented May 1, 2018 •

edited

Loading

jamessan May 1, 2018

bfredl May 1, 2018

jamessan May 1, 2018

jamessan May 1, 2018

justinmk May 10, 2018 •

edited

Loading

justinmk May 10, 2018

justinmk May 10, 2018 •

edited

Loading

justinmk commented May 10, 2018

justinmk commented May 11, 2018

[RFC] api: Make nvim_set_option() update :verbose set ... #8329

[RFC] api: Make nvim_set_option() update :verbose set ... #8329

Conversation

b-r-o-c-k commented Apr 30, 2018

bfredl commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-r-o-c-k May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk May 10, 2018 • edited Loading

Choose a reason for hiding this comment

justinmk commented May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk May 10, 2018 • edited Loading

Choose a reason for hiding this comment

justinmk commented May 10, 2018

justinmk commented May 11, 2018

[RFC] api: Make nvim_set_option() update `:verbose set ...` #8329

[RFC] api: Make nvim_set_option() update `:verbose set ...` #8329

b-r-o-c-k May 1, 2018 •

edited

Loading

justinmk May 10, 2018 •

edited

Loading

justinmk commented May 1, 2018 •

edited

Loading

justinmk May 10, 2018 •

edited

Loading

justinmk May 10, 2018 •

edited

Loading