It would seem unreasonable to define a member of ui_t for every ui event that would arise from an externalizied ui element. For the moment I just added (*event)(name, args) for a generic event that external gui:s care about but not the tui, but it seems like a hack over the present model. (Ref disscussion in #4322 about the distinction of ui/non-ui events).

👍 I plan to get rid of the UI structure, soon even the builtin TUI will be notified via msgpack-rpc.

Similar question could be raised about ui requests like remote_ui_try_resize. Why don't we have api/remote_ui.c and just autogenerate the msgpack validation as for all other api requests?

👍

If multiple uis (including the tui) are connected they might differ in which ui elements they implement themselves or should be drawn on the grid. The "proper" solution would be to have multiple "layers" of the grid that ui:s could show or not show but that looks too complicated at this point (it should probably wait until progress is made on the smart ui protocol). A reasonable heuristic for this case could just be: the ui that sends the keypress opening the pum determines the display mode.

I haven't thought much about how to handle "smart widgets" like a popup menu or the command line, but I imagine there's no need to have layers in the core grid structure, we just need to keep track of what is being displayed and(if applicable) where, leaving other details for the UI to define. For example, here's how I imagine the popup menu to be handled at protocol level:

// first notify UIs that they should display a popup menu at position (x,y) with the list of visible items:
[2, 'ui_display_popup_menu', {position: [x, y], visible_items: [...]}]
// each time the user presses tab, something like this is sent
[2, 'ui_popup_menu_item_changed', {selected_index: x}]
// when it needs to scroll:
[2, 'ui_popup_menu_scroll', {new_visible_items: [...], selected_index: x}]
// when the user selects a completion:
[2, 'ui_hide_popup_menu']

Note that there's no need to keep track of the widget dimensions, so it is possible for UIs to implement the widget with a different font size for example. The UI is also free to allow the user to customize such widgets(see nyaovim), this is possible because the widget is "detached" from the monospace font grid.

Why msgpack (for tui) and not just Object ? Surely copying a Object tree is simpler and cheaper that first serializing and then deserializing msgpack ? (of course unless you plan to externalize tui to a separate process...)

With "layer" just meant the actual drawing that the core still needs to do when the tui (or an external ui not aware of a particular widget) is active, that we need to toggle on/off. But I don't see why the protocol should keep track of and send "visible" items. That would be more work (look how simple the present implementation is) and would just limit what an external ui is able to do. There is no need to restrict ui:s based on how the existing implementation presents stuff.

will you implement this?
#396

of course unless you plan to externalize tui to a separate process...)\

That's the idea, to remove the builtin UI infrastructure and only maintain a system for external UIs(Though the TUI is still going to be distributed with nvim)

@wsdjeg It will certainly move in that direction but that is a discussion thread with many ideas. Do you have any specific suggestion in mind?

For instance for more fancy widgets in the TUI (regarding @choco:s designs), an alternative could be to let a lua callback be called every time the the pum needs to be drawn (including redraws) and let it draw on the screen using the low-level screen_putchar etc functions (to be clear: a plugin will only be allowed to draw directly on the screen buffers in this kind of callback functions)

I would like to have a hotkey to open the info of current item in the popupmenu,the info can be opened below the popupmenu or on the right of the menu.I think it is a feature just like proview windows,but it is better.

Is this going to happen or what?

@tarruda are you working on the TUI refactor soon, our should we move ahead on this before it? I think the most important refactor for this is to refactor the requests in msgpack_rpc/remote_ui.c to proper API functions in api/ui.c; we can live with ui_event in the meanwhile. Ping also @timeyyy who IIRC expressed interest on working on this.

@tarruda are you working on the TUI refactor soon, our should we move ahead on this before it?

This doesn't clash with what I'm doing, so feel free to move ahead. It will be good to validate/improve the concept.

I noticed that we don't currently have many screen tests for the current popup menu implementation(I only found a few in test/functional/viml/completion_spec.lua), so it would be a good idea to add more coverage to ensure this maintains backwards compatibility.

True, there has been some bugs regarding popupmenu + keys/events in the past. Sounds like a good start.

screen tests for the current popup menu in test/functional/viml/completion_spec.lua

I added those :)

Looks we can get a long way to that by adding a screen:expect after every existing plain expect/getline in completion_spec.lua ...

Looks we can get a long way to that by adding a screen:expect after every existing plain expect/getline in completion_spec.lua ...

👍

Using screen:expect is the way to go when testing user-visible features, as it has a good chance to represent what the user sees.

Now that the new version of the lua client(w libmpack as backend) is merged, I will work on the native nvim client, which I'm going to live in the main repository. Keeping in the main repo will improve maintainability as it will share the ugrid.c module with the core, and even the lua client will build on top of the native client. Screen:expect and native UIs will obtain its state from ugrid.c, making screen:expect even closer to what UIs will display.

I got a nice little prototype going.

Might need some more tests.. haven't been able to reproduce generally but i experience a bug in a particular file where tabbing through the externalized pum (just printing), generates a call to popupmenu_show when it shouldn’t, The pum diverges from what neovim displays in this case.

Should we be generating events for colouring e.g Background/Foreground/Highlight?
In this case the colouring is quite simple but something like the status line might require a more generalized heuristic.

for the function names I think popupmenu_x is ok no need for popupmenu_x_item

Yes it is [WIP] because of incomplete tests. Also ui_popupmenu_select_item is not tested at all (if it doesn't work it can be relegated to a later PR as it is not essential, just seems nice for an ui to allow the user to control the menu with mouse for instance)

a call to popupmenu_show when it shouldn’t

Is this a transient, i.e. immediately followed by another show or hide, or is the final visual state different ? Any specifics that triggers it like a certain complete source?

Should we be generating events for coloring e.g Background/Foreground/Highlight?

Not sure, the ui could potentially do more fanciful coloring than the buildin, like different colors for different item kinds. For now let's say the ui could read the Pmenu etc values if it likes, or use its own coloring. (I would imagine a gtk/qt ui could just use the native menu widget with system colors).

for the function names I think popupmenu_x is ok no need for popupmenu_x_item

Sounds good, I will update the names.

Updated (still not done), new names are

request:
ui_set_popupmenu_external(true/false)

events:
popupmenu_show(items, selected)
popupmenu_select(selected)
popupmenu_hide()

I removed ui_popupmenu_select_item because the implementation is entirely independent; let's add it in a followup PR.

popupmenu_select SEEMS to be working.

Is this a transient, i.e. immediately followed by another show or hide, or is the final visual state different ? Any specifics that triggers it like a certain complete source?

A single call to popupmenu_show occurs, the visual state is then different from what vim is showing. (As far as i understand popupmenu_show should replace all existing items in the pum with the new items?, this is what i have implemented)
The behaviour is repeatable on the same file and same keyword. It seems to happen more readily if there are >5 items

For now let's say the ui could read the Pmenu etc values if it likes,

If the colours used by vim for the pum can be accessed i think that is enough. Could you give an example :h Pmenu was a bit vauge.

Having a look at this we seem to be doing two things at once in this PR

1. Allow the UI to render the popupmenu using a custom widget
2. Provide an API for the UI to manipulate the popupmenu (popupmenu_select)

Whats bothering me is the need for the UI to "register" (ui_set_popupmenu_external) to enable this feature, because one UI could register and mess up the user experience for a second UI that is attached (or attaches later) where the popupmenu would then be invisible (at least thats my impression, pum_wants_external is global, not per UI).

But it seems to me that point 1. should be possible without the need for ui_set_popupmenu_external but I'm not sure about 2. yet. If anything because vim/vim had a very similar design for the gui APIs.

As is, when the external popupmenu is disabled Neovim sends the usual redraw events to paint the popup. When the external popupmenu is enabled Neovim skips sending the regular redraw events and instead sends this menu contents.

I wonder if we can't use the new events to tell the UI that I'm about to draw the menu but if you want instead you can draw it yourself and here is the data (something like this).

The downside of doing it like this is the extra data sent in events i.e. the pum always goes out. Another way to go about it would be to make pum_wants_external a per UI setting.

Another way to go about it would be to make pum_wants_external a per UI setting.

I like this. Use the normal draw method as a default, and each gui requests what it wants to handle.

I wonder if we can't use the new events to tell the UI that I'm about to draw the menu

I suppose that is roughly what I meant with "multiple layers of the grid that ui:s could show or not show" I looked briefly into it when starting this, if it could be as simple as hinting "now I'm drawing this/that, don't draw if you don't like that" but it isn't because of the buffering into ScreenLines, so this would need quite some refactor.

at least thats my impression, pum_wants_external is global, not per UI

It happens to be global in the present internal implementation, but it is already per UI in the protocol (which is why the UI needs to "register"), so to forward enable doing something better. As I said, a simple possible heuristic is: the ui that sends the keypress opening the pum determines the display mode.

Provide an API for the UI to manipulate the popupmenu (popupmenu_select)

I removed this for now as it is implementationwise completely separate and just as well could be a separate PR (and the changed logic in edit.c is much more complex than popupmenu.c and thus much more testing is needed)

Thanks for the comments @oni-link. Updated.

The docs are certainly not complete yet, but I think we should merge this unless there are more comments on the code. Making readable and coherent docs for something like this will be simpler once RPC method docs are integrated in :help.

Just a heads up for @rhysd @qvacua @rogual @carlosdcastillo who may be interested in this new API feature. See #4432 (comment) for a proof-of-concept.