feat(ui)!: emit prompt "messages" as cmdline events #31525

luukvbaal · 2024-12-09T09:40:22Z

Problem: Prompts are emitted as message events, where cmdline events
are more appropriate. The user input is also emitted as
message events in fast context, so cannot be displayed with
vim.ui_attach().
Solution: Ask for user input through cmdline prompts.

Depends on neovim/neovim#31525 See neovim/neovim#31522

luukvbaal · 2024-12-10T16:08:52Z

Hoping to make this incremental change while leaving what is displayed on the current message grid unaffected. Invoking a cmdline prompt affects the message grid globals however, which makes that tricky. Nearly there now but spending a lot of time trying to make it work; might have to leave this until after the message grid is hopefully removed...

luukvbaal · 2024-12-10T18:53:35Z

src/nvim/input.c

@@ -157,105 +147,60 @@ int get_keystroke(MultiQueue *events)
  return n;
 }

-/// Get a number from the user.
-/// When "mouse_used" is not NULL allow using the mouse.


This PR now also fixes mouse clicks for number prompts, which have been broken since 2019💀

commit 1ce28d7d9b2fd0e6a81174769e30dc59ded93876 Date: Sun Jun 9 16:00:09 2019 -0400 vim-patch:8.0.1756: GUI: after prompting for a number the mouse shape is wrong

The oldtests for this still passed because it bypasses the problem through nvim_input_mouse().

luukvbaal · 2024-12-11T14:17:32Z

test/functional/ui/input_spec.lua

-      "Xtest-overwrite"                                           |
-      {9:WARNING: The file has been changed since reading it!!!}      |
-      {6:Do you really want to write to it (y/n)?}u                   |
-      {6:Do you really want to write to it (y/n)?}                    |


The point of this test was to make sure no unprintable characters are put on the screen, which I suppose still holds true. The actual display difference seems insignificant.

luukvbaal · 2024-12-11T14:27:50Z

src/nvim/ui.c

@@ -721,7 +721,7 @@ void ui_call_event(char *name, bool fast, Array args)
  // Prompt messages should be shown immediately so must be safe
  if (strcmp(name, "msg_show") == 0) {
    char *kind = args.items[0].data.string.data;
-    fast = !kind || ((strncmp(kind, "confirm", 7) != 0 && strstr(kind, "_prompt") == NULL));
+    fast = !kind || ((strncmp(kind, "confirm", 7) != 0 && strcmp(kind, "return_prompt") != 0));


Can probably simplify this and remove the non-fast msg_show path once all prompt messages are followed by a cmdline_show event. Not sure it's worth the churn to move the "return_prompt" event there now (since it doesn't need to display user input). Probably best to leave that one alone and remove it altogether when we have a default UI that doesn't use it anymore. Heads up @folke.

test/functional/ui/input_spec.lua

justinmk

This looks great to me, it adds structure to flows that were spread around and implicit in various places.

It might be a bit risky but I think we should try it out.

@fredizzimo @glacambre @theol0403 do you see any obvious problems?

glacambre · 2024-12-11T17:08:01Z

I haven't looked at the code, just did a quick check and this doesn't work too well with firenvim - :%s/x/y/gc results in an empty command line popping up without additional information for the user to know what state they're in (i.e., the message saying replace with b? (y/n) is gone).
This might just be an issue with Firenvim itself though, I'll let you know once I've figured it out.

luukvbaal · 2024-12-11T17:17:49Z

I just rebased my default ext_ui branch on top of this (which I admittedly hadn't done yet, just looked at the affected ext_ui tests which seemed to show correct state). Seems to work as expected if you display the prompt message in the cmdline.

zeertzjq · 2024-12-12T00:58:18Z

I think this is dangerous. Any code can be executed in cmdline mode, so this can lead to potential segfaults, and defeat the point of #27874.

luukvbaal · 2024-12-12T01:11:47Z

Well the same reasoning from #31224 still holds. Prompt events simply must be (made) safe. Whether we emit a non-fast message, or cmdline event doesn't really matter.

The risk for prompts is smaller than messages, which are used all over the place. So I don't think it defeats the point of #27874.

zeertzjq · 2024-12-12T14:29:49Z

Whether we emit a non-fast message, or cmdline event doesn't really matter.

It does matter. With a non-fast message the executed code is limited to the callback. With a cmdline prompt there can be autocommands from unrelated plugins.

folke · 2024-12-20T14:03:45Z

fyi, I'll be on vacation till January 1, so would be great not to merge this till I'm back, since this will break some things in noice, which are easy to fix, but will need to be fixed. ty

luukvbaal · 2024-12-20T14:46:01Z

Fine by me @folke. FYI (if we consider #31525 (comment) to be resolved); after this PR you would no longer have to keep track of an internal message queue for prompt messages, scheduled messages will be displayed when Nvim enters the cmdline state (@przepompownia).

przepompownia · 2024-12-20T21:08:09Z

After several hours of working without a queue on this PR, I did not encounter any of the previously observed issues, especially with inputlist.

it seems to be no longer needed with neovim/neovim#31525

fredizzimo · 2024-12-23T10:06:58Z

I tested this with Neovide, and didn't see any obvious problems, neither in practice nor when reviewing the code. So, this LGTM.

luukvbaal · 2024-12-23T11:46:45Z

test/functional/ui/cmdline_spec.lua

-    ]],
-    }
+      {3:                         }|
+                               |*2


Small regression in this test simply because the message is now a cmdline prompt. This triggers a :set cmdheight=0 issue that happens somewhere in the incsearch path. Whenever cmdheight == 0 and we type /foo, ?foo or :%s/foo where foo exceeds the number of Columns we get a cmdline/msg_row conflict, resulting in the cleared row and extra scrolled line here. I think it's related to:

neovim/src/nvim/message.c

Lines 237 to 240 in adcd936

// TODO(bfredl): this should already be the case, but fails in some

// "batched" executions where compute_cmdrow() use stale positions or

// something.

cmdline_row = msg_grid_pos;

I could attempt to fix this but that work will be in vain assuming we remove the message grid after #27855 (and issue is not present with ext_cmdline (obviously)).

justinmk

Will wait for @folke / Jan 2.

Needs a news item, perhaps in the "UI" section of news-features. Or add a "UI" section to news-breaking.

luukvbaal · 2024-12-30T21:52:10Z

I think it fits the "EVENTS" section in news-breaking? Those items need updating anyhow for this change, and what is shown to the user is largely unchanged as a result of this change.

Problem: Prompts are emitted as messages events, where cmdline events are more appropriate. The user input is also emitted as message events in fast context, so cannot be displayed with vim.ui_attach(). Solution: Ask for user input through cmdline prompts.

folke · 2025-01-04T14:56:56Z

@luukvbaal was just looking into fixing noice, but I'm seeing some weird behavior.

I'm testing with:

vim.fn.confirm("Save changes?", "&Yes\n&No\n&Cancel")

When executing this:

I receive a msg_show confirm event immediately
No cmdline_show events yet
Only after I type an invalid option, like l, I then get cmdline_show events with the prompt.

I assume the cmdline_show event should also come in immediately and right after the msg_show?

Additionally, it also seems no longer possible to know that the cmdline_show event is part of a confirm.
Previously I used that in noice, to show a different ui, with buttons based on the prompt text.

luukvbaal · 2025-01-04T15:47:53Z

I assume the cmdline_show event should also come in immediately and right after the msg_show?

It should, and that's precisely the behavior I'm seeing. I can try to reproduce with noice later.

Additionally, it also seems no longer possible to know that the cmdline_show event is part of a confirm.
Previously I used that in noice, to show a different ui, with buttons based on the prompt text.

I think you can use the cmdline_show->prompt arg for that now.

folke · 2025-01-04T15:52:50Z

Ok, thanks for confirming the behavior. Will see if I can create a repro without Noice.

folke · 2025-01-04T15:56:41Z

And yes, noice shows the prompt, which is fine, but knowing it's a prompt triggered by a confirm would be a nice to have, but not that important.

folke · 2025-01-04T16:03:29Z

Ok, figured it out. The cmdline_show is not shown when triggered from a keymap with silent=true.
Which makes sense I guess, but probably not wanted for confirm messages?

vim.keymap.set("n", "T", function()
	vim.fn.confirm("Save changes?", "&Yes\n&No\n&Cancel")
end, { silent = true })

folke · 2025-01-04T16:15:32Z

After further testing, the behavior without ui_attach, so regular TUI also seems a bit off.

With my example, and without using a keymap and with cmdheight=5 only the save changes? is printed, not the yes/no message.

Yeah maybe, but that mapping yields the same behavior without ext_cmdline right?

~~Nope, without ext_cmdline, it shows just the yes/no with silent=true.~~
It shows just the message indeed

luukvbaal · 2025-01-04T16:16:32Z

Yeah I meant the same broken behavior. That is a regression that should be fixed.
Not sure how, didn't consider silent mappings for this PR. I guess this being a prompt we should just ignore that it is silent.

github-actions bot added spell spellcheck ui labels Dec 9, 2024

This comment was marked as off-topic.

Sign in to view

przepompownia added a commit to przepompownia/msg-show.nvim that referenced this pull request Dec 9, 2024

Queue messages from fast context

5df3cb3

Depends on neovim/neovim#31525 See neovim/neovim#31522

luukvbaal force-pushed the cmdnumber branch from 73f5307 to 3a30534 Compare December 10, 2024 15:51

luukvbaal changed the title ~~feat(ui): use commandline for number prompts~~ feat(ui): use commandline for prompt "messages" Dec 10, 2024

luukvbaal force-pushed the cmdnumber branch 5 times, most recently from 8ce796b to 2a97fed Compare December 11, 2024 14:40

luukvbaal added ui-extensibility UI extensibility, events, protocol, externalized UI cmdline-mode command line, also cmdwin ci:skip-news and removed spell spellcheck labels Dec 11, 2024

luukvbaal force-pushed the cmdnumber branch from 2a97fed to 7692fc5 Compare December 11, 2024 15:00

luukvbaal marked this pull request as ready for review December 11, 2024 15:00

luukvbaal commented Dec 11, 2024

View reviewed changes

luukvbaal requested review from bfredl and zeertzjq December 11, 2024 15:10

justinmk reviewed Dec 11, 2024

View reviewed changes

test/functional/ui/input_spec.lua Show resolved Hide resolved

justinmk approved these changes Dec 11, 2024

View reviewed changes

luukvbaal force-pushed the cmdnumber branch 2 times, most recently from 9dd604e to aea19ad Compare December 11, 2024 23:21

luukvbaal force-pushed the cmdnumber branch from 75d83f8 to 378edbb Compare December 17, 2024 14:55

przepompownia added a commit to przepompownia/msg-show.nvim that referenced this pull request Dec 21, 2024

Drop using message queue

613f1e2

it seems to be no longer needed with neovim/neovim#31525

luukvbaal force-pushed the cmdnumber branch from 378edbb to fe7be7a Compare December 23, 2024 11:36

luukvbaal commented Dec 23, 2024

View reviewed changes

luukvbaal mentioned this pull request Dec 26, 2024

Improve usability for search/replace "with confirm" #31667

Closed

justinmk approved these changes Dec 30, 2024

View reviewed changes

luukvbaal force-pushed the cmdnumber branch from fe7be7a to 5f26a89 Compare December 30, 2024 21:52

luukvbaal removed the ci:skip-news label Dec 30, 2024

justinmk changed the title ~~feat(ui): use commandline for prompt "messages"~~ feat(ui): emit prompt "messages" as cmdline events Dec 30, 2024

luukvbaal force-pushed the cmdnumber branch from 5f26a89 to 777939a Compare December 30, 2024 22:14

luukvbaal changed the title ~~feat(ui): emit prompt "messages" as cmdline events~~ feat(ui)!: emit prompt "messages" as cmdline events Dec 30, 2024

justinmk merged commit 48e2a73 into neovim:master Jan 2, 2025
29 of 31 checks passed

folke mentioned this pull request Jan 4, 2025

Noice is partially broken on Nightly folke/noice.nvim#998

Closed

6 tasks

luukvbaal mentioned this pull request Jan 5, 2025

fix(cmdline): always show cmdline when it is a prompt #31866

Merged

justinmk mentioned this pull request Jan 8, 2025

%s/a/b/gc not displaying prompt with neovim nightly glacambre/firenvim#1661

Open

PriceHiller mentioned this pull request Jan 14, 2025

bug: Noice confirmation prompts are broken on nightly folke/noice.nvim#1036

Open

4 tasks

luukvbaal mentioned this pull request Jan 17, 2025

Calling redraw clears inputlist() choices #32068

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui)!: emit prompt "messages" as cmdline events #31525

feat(ui)!: emit prompt "messages" as cmdline events #31525

luukvbaal commented Dec 9, 2024 •

edited

Loading

This comment was marked as off-topic.

luukvbaal commented Dec 10, 2024 •

edited

Loading

luukvbaal Dec 10, 2024

luukvbaal Dec 11, 2024 •

edited

Loading

luukvbaal Dec 11, 2024

justinmk left a comment

glacambre commented Dec 11, 2024 •

edited

Loading

luukvbaal commented Dec 11, 2024

zeertzjq commented Dec 12, 2024 •

edited

Loading

luukvbaal commented Dec 12, 2024

zeertzjq commented Dec 12, 2024 •

edited

Loading

folke commented Dec 20, 2024

luukvbaal commented Dec 20, 2024

przepompownia commented Dec 20, 2024

fredizzimo commented Dec 23, 2024

luukvbaal Dec 23, 2024 •

edited

Loading

This comment was marked as duplicate.

justinmk left a comment

luukvbaal commented Dec 30, 2024 •

edited

Loading

folke commented Jan 4, 2025 •

edited

Loading

luukvbaal commented Jan 4, 2025

folke commented Jan 4, 2025

folke commented Jan 4, 2025

folke commented Jan 4, 2025 •

edited

Loading

folke commented Jan 4, 2025 •

edited

Loading

luukvbaal commented Jan 4, 2025 •

edited

Loading

	// TODO(bfredl): this should already be the case, but fails in some
	// "batched" executions where compute_cmdrow() use stale positions or
	// something.
	cmdline_row = msg_grid_pos;

feat(ui)!: emit prompt "messages" as cmdline events #31525

feat(ui)!: emit prompt "messages" as cmdline events #31525

Conversation

luukvbaal commented Dec 9, 2024 • edited Loading

This comment was marked as off-topic.

luukvbaal commented Dec 10, 2024 • edited Loading

luukvbaal Dec 10, 2024

Choose a reason for hiding this comment

luukvbaal Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

luukvbaal Dec 11, 2024

Choose a reason for hiding this comment

justinmk left a comment

Choose a reason for hiding this comment

glacambre commented Dec 11, 2024 • edited Loading

luukvbaal commented Dec 11, 2024

zeertzjq commented Dec 12, 2024 • edited Loading

luukvbaal commented Dec 12, 2024

zeertzjq commented Dec 12, 2024 • edited Loading

folke commented Dec 20, 2024

luukvbaal commented Dec 20, 2024

przepompownia commented Dec 20, 2024

fredizzimo commented Dec 23, 2024

luukvbaal Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

justinmk left a comment

Choose a reason for hiding this comment

luukvbaal commented Dec 30, 2024 • edited Loading

folke commented Jan 4, 2025 • edited Loading

luukvbaal commented Jan 4, 2025

folke commented Jan 4, 2025

folke commented Jan 4, 2025

folke commented Jan 4, 2025 • edited Loading

folke commented Jan 4, 2025 • edited Loading

luukvbaal commented Jan 4, 2025 • edited Loading

luukvbaal commented Dec 9, 2024 •

edited

Loading

luukvbaal commented Dec 10, 2024 •

edited

Loading

luukvbaal Dec 11, 2024 •

edited

Loading

glacambre commented Dec 11, 2024 •

edited

Loading

zeertzjq commented Dec 12, 2024 •

edited

Loading

zeertzjq commented Dec 12, 2024 •

edited

Loading

luukvbaal Dec 23, 2024 •

edited

Loading

luukvbaal commented Dec 30, 2024 •

edited

Loading

folke commented Jan 4, 2025 •

edited

Loading

folke commented Jan 4, 2025 •

edited

Loading

folke commented Jan 4, 2025 •

edited

Loading

luukvbaal commented Jan 4, 2025 •

edited

Loading