New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] allow external uis to render the popupmenu #4432

Merged
merged 4 commits into from Aug 29, 2016

Conversation

Projects
None yet
@bfredl
Member

bfredl commented Mar 9, 2016

This enables the internal drawing of the completion popupmenu to be switched off and the data instead sent to an external gui for it to display as it likes.

Not that I care that much about the pum specifically, but it is a good example of an ui element whose implementation is reasonally well isolated from the rest of screen.c and thus serves as a good prototype for externalizing ui elements in general.

Some open questions:

  • 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).
  • 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? (done in #4817 )
  • 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. solution: only use external popupmenu if all connected clients support it.

I made stupid test for the python-gui, it doesn't even draw anything but it echoes the events to stdout so one could see that they are correct.

The exposed ui events are:

popupmenu_show(items, selected, row, col)
popupmenu_select(selected)
popupmenu_hide()

items will be a list of tuples [text, kind, extra, info]. selected will either be a 0-based index or -1 if no element is selected.
To activate this mode, instead of using ui_attach(row, cols, true) use nvim_ui_attach(rows, cols, {'rgb': true, 'popupmenu_external': true}).
Alternatively nvim_ui_set_option('popupmenu_external', 'true') can be set after the ui already is attached.

@marvim marvim added the WIP label Mar 9, 2016

@tarruda

This comment has been minimized.

Member

tarruda commented Mar 9, 2016

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.

@bfredl

This comment has been minimized.

Member

bfredl commented Mar 9, 2016

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.

@wsdjeg

This comment has been minimized.

Contributor

wsdjeg commented Mar 10, 2016

will you implement this?
#396

@tarruda

This comment has been minimized.

Member

tarruda commented Mar 10, 2016

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)

@bfredl

This comment has been minimized.

Member

bfredl commented Mar 10, 2016

@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)

@wsdjeg

This comment has been minimized.

Contributor

wsdjeg commented Mar 10, 2016

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.

@vhakulinen

This comment has been minimized.

vhakulinen commented Apr 13, 2016

Is this going to happen or what?

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 13, 2016

@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

This comment has been minimized.

Member

tarruda commented Apr 13, 2016

@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.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 13, 2016

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 :)

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 13, 2016

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

@tarruda

This comment has been minimized.

Member

tarruda commented Apr 13, 2016

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.

@bfredl bfredl force-pushed the bfredl:pum_ui branch 2 times, most recently from 8cdb54c to 7773509 Apr 14, 2016

@bfredl bfredl force-pushed the bfredl:pum_ui branch from 7773509 to f788048 May 29, 2016

} else {
ADD(args, INTEGER_OBJ(selected));
ui_event("pum_select", args);
ui_event("popupmeny_select_item", args);

This comment has been minimized.

@timeyyy

timeyyy Jun 6, 2016

Contributor

@bfredl typo

@timeyyy

This comment has been minimized.

Contributor

timeyyy commented Jun 6, 2016

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

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 7, 2016

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.

@bfredl bfredl force-pushed the bfredl:pum_ui branch from f788048 to 17cc699 Jun 8, 2016

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 8, 2016

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.

@timeyyy

This comment has been minimized.

Contributor

timeyyy commented Jun 8, 2016

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.

@equalsraf

This comment has been minimized.

Contributor

equalsraf commented Jun 8, 2016

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.

@timeyyy

This comment has been minimized.

Contributor

timeyyy commented Jun 8, 2016

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.

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 8, 2016

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)

<
["set_title", title]
["set_icon", icon]
set the window title, and icon (minimized) window title, respectively.

This comment has been minimized.

@oni-link

oni-link Aug 22, 2016

Contributor

set -> Set

An item in the currently displayed popupmenu is selected. `selected`
is either a zero-based index into the array of items from the last
`popupmenu_show` event, or -1 if no item
is selected.

This comment has been minimized.

@oni-link

oni-link Aug 22, 2016

Contributor

This line fits into the previous line.

@bfredl bfredl force-pushed the bfredl:pum_ui branch from ede3585 to a529406 Aug 24, 2016

@bfredl

This comment has been minimized.

Member

bfredl commented Aug 24, 2016

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.

@bfredl bfredl force-pushed the bfredl:pum_ui branch from a529406 to 34edc66 Aug 25, 2016

@bfredl bfredl force-pushed the bfredl:pum_ui branch from 34edc66 to 8d6e3f2 Aug 29, 2016

@bfredl bfredl merged commit 0b5a7e4 into neovim:master Aug 29, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.06%) to 70.311%
Details

@jszakmeister jszakmeister removed the RFC label Aug 29, 2016

@justinmk

This comment has been minimized.

Member

justinmk commented Sep 25, 2016

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.

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 27, 2016

NVIM v0.1.6
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 28, 2016

NVIM v0.1.6
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!

@timeyyy timeyyy referenced this pull request Feb 2, 2017

Open

[RFC] External ui #5686

@justinmk justinmk added the ui-ext label Jul 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment