feat: user command "preview" (like inccommand) #18194

famiu · 2022-04-20T18:43:12Z

This PR aims to add support for inccommand incremental preview functionality to user commands. It defines an incremental preview "protocol" that a user command can follow to add support for incremental live preview of that command when inccommand is enabled. This PR adds an API-only preview flag to user commands which allows a command to be incrementally previewed.

Closes #7370

runtime/doc/map.txt

src/nvim/api/private/helpers.c

src/nvim/api/vim.c

runtime/doc/map.txt

runtime/doc/api.txt

runtime/doc/map.txt

justinmk · 2022-04-23T15:49:44Z

#18194 (comment)

Don't know if we want to extend the vimscript/ex-mode implementation this has been avoided in the past (#17426) favoring implementation on the API level.

💯 do not add more Vimscript syntax without a very strong reason.

I think we should make an exception in this case personally since it only has any effect when inccommand, a Neovim-specific feature is enabled, so it shouldn't conflict with Vim. But I'm free to make changes if required

I don't understand the reasoning there. Why is this worth increasing the surface area of code we need to maintain, documentation that users need to read and understand, syntax elements that need to be highlighted, errors that need to be surfaced, ...? Using :command intead of the API to define complex functions is just pointless.

src/nvim/ex_cmds_defs.h

test/functional/api/vim_spec.lua

justinmk · 2022-04-23T16:02:27Z

Does this close #7370 or at least partially address it? Please reference relevant issues.

famiu · 2022-04-23T16:30:37Z

Does this close #7370 or at least partially address it? Please reference relevant issues.

This does resolve most of #7370

famiu · 2022-04-23T16:33:13Z

#18194 (comment)

Don't know if we want to extend the vimscript/ex-mode implementation this has been avoided in the past (#17426) favoring implementation on the API level.

100 do not add more Vimscript syntax without a very strong reason.

Alright, I'll make it API-only in that case

I think we should make an exception in this case personally since it only has any effect when inccommand, a Neovim-specific feature is enabled, so it shouldn't conflict with Vim. But I'm free to make changes if required

I don't understand the reasoning there. Why is this worth increasing the surface area of code we need to maintain, documentation that users need to read and understand, syntax elements that need to be highlighted, errors that need to be surfaced, ...? Using :command intead of the API to define complex functions is just pointless.

I have no issue making it API-only. But I'm just wondering how it should be documented in that case. Putting it with other command attributes in the documentation would be weird since it'd be Lua only, and it's too much information to put in the documentation of nvim_create_user_command. Any suggestions regarding that?

clason · 2022-04-23T16:37:31Z

I have no issue making it API-only. But I'm just wondering how it should be documented in that case. Putting it with other command attributes in the documentation would be weird since it'd be Lua only, and it's too much information to put in the documentation of nvim_create_user_command. Any suggestions regarding that?

There's precedent for that: the arguments passed to (only) Lua autocommand callbacks.

A more structured way of indicating different API exposure (FUNC_API_*) in the docs would be nice, though.

justinmk · 2022-04-23T16:41:38Z

Putting it with other command attributes in the documentation would be weird since it'd be Lua only,

True, but that can be improved later. For example the list can become agnostic, and then referenced from :help :command and :help nvim_.... For now just slap a "(Nvim API only)" or something like that.

Vimscript is the "eval" part of Vim (including :function, :command, etc). The "editor DSL" is Ex and normal-mode commands, which is great for ad-hoc editing work. Vimscript is frozen.

runtime/doc/builtin.txt

runtime/doc/map.txt

famiu · 2022-05-28T10:19:54Z

I meant just with cursor on line 1, which should mean the same thing. The amount of implicit state in ex commands still makes me nauseous...

I think that's moreso the fault of parse_cmdline only parsing the command and not setting defaults values for stuff like line2

runtime/doc/map.txt

justinmk

getting pretty close! A bit worried about the no_send_tick / changedtick buffer flag

src/nvim/buffer_defs.h

justinmk · 2022-05-30T23:51:33Z

test/functional/ui/inccommand_user_spec.lua

+local command = helpers.command
+
+-- Implementation of a :Replace command that's used for testing user inccommand
+local SetupReplaceCmd = [[


Let's stick to one convention. all Lua code in the codebase uses snake_case.

This function is huge. Is it necessary for the test? Any way to reduce the code?

I guess we could try having a completely different type of preview function entirely. I made one that mimics :substitute because that's currently the standard for how command preview should behave.

test/functional/ui/inccommand_user_spec.lua

justinmk · 2022-05-30T23:55:14Z

src/nvim/buffer_updates.c

@@ -253,7 +253,7 @@ void buf_updates_send_changes(buf_T *buf, linenr_T firstline, int64_t num_added,
  for (size_t i = 0; i < kv_size(buf->update_callbacks); i++) {
    BufUpdateCallbacks cb = kv_A(buf->update_callbacks, i);
    bool keep = true;
-    if (cb.on_lines != LUA_NOREF && (cb.preview || !(State & MODE_CMDPREVIEW))) {
+    if (cb.on_lines != LUA_NOREF && (cb.preview || !cmdpreview)) {


why did this change from a formal "editor state", to a global variable (an implicit pseudo-state)?

Quite a few functions reset the value of State, which caused inccommand preview for some commands to not work properly, changing it to a global state fixes that issue.

src/nvim/ex_cmds.lua

src/nvim/ex_cmds_defs.h

src/nvim/lua/executor.c

src/nvim/ex_getln.c

justinmk · 2022-05-31T00:24:43Z

src/nvim/ex_docmd.c

+    return false;
+  } else if (!IS_USER_CMDIDX(ea.cmdidx)) {
+    // find_ex_command sets the flags for user commands automatically
+    ea.argt = cmdnames[(int)ea.cmdidx].cmd_argt;


did find_ex_command have these side-effects before this PR? is that avoidable?

It did. And I think it's fine the way it is since find_ex_command also has some other side-effects, so if we wanted to remove side-effects we'd have to remove all of them, which'd be quite a hassle.

src/nvim/ex_docmd.c

src/nvim/ex_getln.c

runtime/doc/map.txt

justinmk · 2022-05-31T11:07:16Z

runtime/doc/map.txt

+	      line1 - 1,
+	      line2,
+	      0,
+	      new_lines


The example will look a lot less intimidating if we just don't wrap the function args like this. Don't worry too much about exceeding 80 chars for code examples, if it helps with readability.

runtime/doc/map.txt

justinmk · 2022-05-31T11:15:21Z

src/nvim/buffer_updates.c

@@ -197,6 +197,9 @@ void buf_updates_send_changes(buf_T *buf, linenr_T firstline, int64_t num_added,
    return;
  }

+  // Don't send b:changedtick during 'inccommand' preview if "buf" is the current buffer.
+  bool send_tick = !(cmdpreview && buf == curbuf);


The buf == curbuf condition makes me wonder if the cmdpreview protocol should forbid switching buffers? But it is common for plugins to temporarily switch buffers, and we don't have a way of preventing it.

I think it's fine. If a plugin does something stupid, it's the plugin's fault and Neovim doesn't have to deal with that

test/functional/ui/inccommand_user_spec.lua

Adds a Lua-only `preview` flag to user commands which allows the command to be incrementally previewed like `:substitute` when 'inccommand' is set.

justinmk · 2022-05-31T19:15:40Z

src/nvim/ex_getln.c

+///    5. If the return value of the preview callback is not 0, update the screen while the effects
+///       of the preview are still in place.
+///    6. Revert all changes made by the preview callback.
+static void cmdpreview_show(CommandLineState *s)


A key feature of 'inccommand' :substitute is that it skips the preview if the preview takes too long:

neovim/src/nvim/ex_cmds.c

Lines 4430 to 4432 in 46536f5

if (got_quit || profile_passed_limit(timeout)) { // Too slow, disable.

set_string_option_direct("icm", -1, (char_u *)"", OPT_FREE,

SID_NONE);

That should also be built into the handling of user-defined cmd preview.

Another important optimization was to stop looking for matches if additional ones would not be shown to the user (either the preview buffer is full and icm=split, or they're out of the viewport of the current buffer with icm=nosplit). This PR is too complicated for me, so I could not discern if that was implemented, but I figured I'd mention it (ofc it's most helpfull in large files).

A key feature of 'inccommand' :substitute is that it skips the preview if the preview takes too long:

neovim/src/nvim/ex_cmds.c

Lines 4430 to 4432 in 46536f5

if (got_quit || profile_passed_limit(timeout)) { // Too slow, disable.

set_string_option_direct("icm", -1, (char_u *)"", OPT_FREE,

SID_NONE);

That should also be built into the handling of user-defined cmd preview.

How should we implement that though? Or should we leave that to the preview callback?

How should we implement that though? Or should we leave that to the preview callback?

Should be done implicitly, builtin.

Maybe f_wait (:help wait()) could be used, or at least provides a starting point:

neovim/src/nvim/eval/funcs.c

Line 3869 in c632f64

static void f_wait(typval_T *argvars, typval_T *rettv, FunPtr fptr)

I think we need this: Lua: interrupt / cancel Lua code #6800

related stale PR: [WIP] Allow interrupting lua code #6809

Another important optimization was to stop looking for matches ... I could not discern if that was implemented,

@KillTheMule I believe that is covered by tests so I don't think there was a regression.

CHANGES: - Add a mutli-cursor mapping TODO: - Live preview via neovim/neovim#18194 - Add a highlight group (potentially)

dundargoc reviewed Apr 20, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

gpanders reviewed Apr 20, 2022

View reviewed changes

src/nvim/api/private/helpers.c Outdated Show resolved Hide resolved

src/nvim/api/vim.c Outdated Show resolved Hide resolved

runtime/doc/map.txt Outdated Show resolved Hide resolved

muniter reviewed Apr 20, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

runtime/doc/map.txt Outdated Show resolved Hide resolved

zeertzjq reviewed Apr 21, 2022

View reviewed changes

runtime/doc/api.txt Outdated Show resolved Hide resolved

famiu requested review from dundargoc and gpanders April 21, 2022 04:56

dundargoc reviewed Apr 21, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

dundargoc reviewed Apr 21, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk mentioned this pull request Apr 23, 2022

API: nvim_get_mode(): return command-line type #16891

Open

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

src/nvim/ex_cmds_defs.h Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

test/functional/api/vim_spec.lua Outdated Show resolved Hide resolved

justinmk changed the title ~~feat: add preview functionality to user commands~~ feat: user command "preview" support (like inccommand) Apr 23, 2022

justinmk changed the title ~~feat: user command "preview" support (like inccommand)~~ feat: user command "preview" (like inccommand) Apr 23, 2022

famiu force-pushed the feat/usercmd_preview branch from 26ae0a3 to edd9ef5 Compare April 23, 2022 17:52

famiu requested a review from justinmk April 23, 2022 20:05

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/builtin.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed Apr 23, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

famiu force-pushed the feat/usercmd_preview branch 3 times, most recently from 361b708 to 3c14cb1 Compare May 29, 2022 19:27

justinmk reviewed May 30, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed May 30, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed May 30, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed May 30, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed May 31, 2022

View reviewed changes

src/nvim/ex_getln.c Outdated Show resolved Hide resolved

justinmk reviewed May 31, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed May 31, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed May 31, 2022

View reviewed changes

runtime/doc/map.txt Outdated Show resolved Hide resolved

justinmk reviewed May 31, 2022

View reviewed changes

test/functional/ui/inccommand_user_spec.lua Outdated Show resolved Hide resolved

justinmk approved these changes May 31, 2022

View reviewed changes

feat: add preview functionality to user commands

46536f5

Adds a Lua-only `preview` flag to user commands which allows the command to be incrementally previewed like `:substitute` when 'inccommand' is set.

famiu force-pushed the feat/usercmd_preview branch from 55194fa to 46536f5 Compare May 31, 2022 14:55

bfredl merged commit 7380ebf into neovim:master May 31, 2022

famiu deleted the feat/usercmd_preview branch May 31, 2022 16:04

This was referenced May 31, 2022

inccommand: plugin support: cmd_can_preview for user commands #11985

Closed

[WIP] Inccommand for global #11958

Closed

justinmk reviewed May 31, 2022

View reviewed changes

b0o mentioned this pull request May 31, 2022

Add inccommand preview for Neovim tpope/vim-abolish#107

Open

adelarsq mentioned this pull request Jun 1, 2022

Add inccommand support adelarsq/neovcs.vim#2

Open

This was referenced Jun 26, 2022

inccommand: fix for wrapping of large string in preview mode #8283

Closed

Memory leak when inccommand is set to split/nosplit #14549

Closed

inccommand=nosplit: segfault/hang when adding ":%s//" to the end of a extract-constant macro #12028

Closed

asyncedd added a commit to asyncedd/dots.nvim that referenced this pull request May 20, 2023

feat(leap): ✨ Add multi-cursor support

ce43e3c

CHANGES: - Add a mutli-cursor mapping TODO: - Live preview via neovim/neovim#18194 - Add a highlight group (potentially)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: user command "preview" (like inccommand) #18194

feat: user command "preview" (like inccommand) #18194

famiu commented Apr 20, 2022 •

edited

justinmk commented Apr 23, 2022 •

edited

justinmk commented Apr 23, 2022

famiu commented Apr 23, 2022

famiu commented Apr 23, 2022

clason commented Apr 23, 2022

justinmk commented Apr 23, 2022 •

edited

famiu commented May 28, 2022

justinmk left a comment •

edited

justinmk May 30, 2022

famiu May 31, 2022

justinmk May 30, 2022 •

edited

famiu May 31, 2022

justinmk May 31, 2022

famiu May 31, 2022

justinmk May 31, 2022

justinmk May 31, 2022

famiu May 31, 2022

justinmk May 31, 2022

KillTheMule May 31, 2022

famiu May 31, 2022

justinmk May 31, 2022 •

edited

	if (got_quit \|\| profile_passed_limit(timeout)) { // Too slow, disable.
	set_string_option_direct("icm", -1, (char_u *)"", OPT_FREE,
	SID_NONE);

feat: user command "preview" (like inccommand) #18194

feat: user command "preview" (like inccommand) #18194

Conversation

famiu commented Apr 20, 2022 • edited

justinmk commented Apr 23, 2022 • edited

justinmk commented Apr 23, 2022

famiu commented Apr 23, 2022

famiu commented Apr 23, 2022

clason commented Apr 23, 2022

justinmk commented Apr 23, 2022 • edited

famiu commented May 28, 2022

justinmk left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk May 30, 2022 • edited

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

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 31, 2022 • edited

Choose a reason for hiding this comment

famiu commented Apr 20, 2022 •

edited

justinmk commented Apr 23, 2022 •

edited

justinmk commented Apr 23, 2022 •

edited

justinmk left a comment •

edited

justinmk May 30, 2022 •

edited

justinmk May 31, 2022 •

edited