Skip to content

[RFC] ui: implement ext_messages to externalize all messages in msg area #7466

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

Merged
merged 1 commit into from
Feb 10, 2019

Conversation

bfredl
Copy link
Member

@bfredl bfredl commented Oct 31, 2017

extracted from #5686. Mostly just a heads-up I'm looking into this, might need some redesign before it is ready to be tested/used.

@bfredl bfredl added the ui-extensibility UI extensibility, events, protocol, externalized UI label Oct 31, 2017
@marvim marvim added the WIP label Oct 31, 2017
@bryphe
Copy link

bryphe commented Oct 31, 2017

This would be great for Oni! I'd like to explore some new ways of rendering messages in the UI.

@bfredl bfredl force-pushed the ext_messages branch 3 times, most recently from 5f2ca51 to c8ef189 Compare November 3, 2017 13:45
@bfredl
Copy link
Member Author

bfredl commented Nov 3, 2017

So, this should basically work for early testing. Multiple msg_chunk calls build up a msg line, msg_end tells the line is finished. msg_start_kind(...) tells the kind for some kinds of messages, but clients should be prepared msg_chunk:s come without a msg_start_kind(). msg_showcmd([attrs, text]) works independently of all other events.

Currently these is no concept of "multi-line messages", the API should probably try to recover it where it is possible, for instance single multiline nvim_out/err_write call.

I have only tested ext_messages also with ext_cmdline. I do not plan to test ext_messages without ext_cmdline; we should throw error in this case, or just make the former imply the later.

@bfredl
Copy link
Member Author

bfredl commented Apr 16, 2018

Rebased. Also changed:

@bfredl
Copy link
Member Author

bfredl commented Aug 10, 2018

Some more progress of this (partly because I think I understand a bit more how message.c works, and partly because this will be useful for #8455 and #8707).
Now the event format is

msg_show(kind, [["text", attr], ["more text", other_attr]], keep_batch)

which is probably closer to what UI:s want (rather than to how the msg_ semi-abstraction is constructed internally). The intention behind the "keep_batch" parameter is to signal if the message should continue to build up a batch of messages,(:echo (+ will show two error messages, but they are always displayed together), or if the previous message should be considered "done" and replaced by a new message.

It is not clear however how it should work when you type : att a "press return" prompt, where you now can keep building a batch of scrolled messages, even if they are technically unrelated. We should provide clients with the necessary information to replicate this behaviour if they so want.

Without this feature, 'cmdheight' will control how many message lines nvim will display without invoking a "press RETURN" prompt. Currently this branch works as if cmdheight=∞, but we probably want to give clients more control. Either:

  • implement nvim-side heuristic when a message is considered "large". We can count the number of individual messages and (hard) line-breaks and compare with a client supplied number (which still could be if it so wants)
  • when nvim is unsure whether a prompt should be displayed or not, do a synchronous call to the client. Either the client could wait for the user itself, or just return whether the current message batch is "large" and thus nvim should wait for return.

The latter is somewhat of a break of the current external UI design, but there a more situations where synchronous UI calls would be useful, such as client interpreted movement commands in #8707 and potentially first class support of dialogs.

@justinmk
Copy link
Member

The intention behind the "keep_batch" parameter is to signal if the message should continue to build up a batch of messages,(:echo (+ will show two error messages, but they are always displayed together), or if the previous message should be considered "done" and replaced by a new message.

How about calling this more instead of keep_batch? That's what we use for nvim_buf_lines_event.

Without this feature, 'cmdheight' will control how many message lines nvim will display without invoking a "press RETURN" prompt. Currently this branch works as if cmdheight=∞, but we probably want to give clients more control. Either: ....

I'm hoping for a third approach: remove the blocking nature of "press RETURN" entirely. I've been experimenting with this in a local branch, and it works nicely.

  • The most basic approach is that instead of "press RETURN", the wall-of-text goes away as soon as the user hits any key (the pager is opt-in via g> instead of opt-out).
  • Another idea: let users specify a maximum height for the wall-of-text, and they can clear it with CTRL-L. And again, they can opt-into the pager via g> .

Messages should be passive and opt-in, instead of constant in-your-face panic.

@bfredl
Copy link
Member Author

bfredl commented Aug 10, 2018

How about calling this more instead of keep_batch? That's what we use for nvim_buf_lines_event.

Maybe. This is still at the design phase, rather than the name bikeshedding phase :P Also the semantics are currently the reverse of buf_lines.

I've been experimenting with this in a local branch, and it works nicely.

It would be nice to see this branch.

I'm hoping for a third approach: remove the blocking nature of "press RETURN" entirely. [...] Messages should be passive and opt-in, instead of constant in-your-face panic.

I think my post was pretty clear in giving clients control, which implies giving users control (by configuring their UI, or using the UI that fits their use case). Clients being able to reimplement cmdheight=1 is not a consequence of this behavior being objective good (though personally I would like to experiment with behavior similar to something likecmdheight=2...3, where the blocking nature also can depend on the severity of the message), rather just of clients/users actually having full control to implement the behavior they want. The cmdheight=∞ case as I already mentioned would be the passive, avoid blocking as much as possible, strategy.

@justinmk
Copy link
Member

I think my post was pretty clear in giving clients control, which implies giving users control (by configuring their UI, or using the UI that fits their use case).

Yes but the "somewhat of a break of the current external UI design" would be good to avoid, I guess. And working well by default is preferable.

@bfredl
Copy link
Member Author

bfredl commented Aug 11, 2018

And working well by default is preferable.

I don't think I have or would argue against this :) My point is that ext_messages shouldn't be needlessly tangled together with a larger "press RETURN" refactor (which it sounds your branch hints at), which has a different purpose and we surely want to work with or without external UI...

The purpose of ext_messages is just to say how messages should displayed when the global grid no longer exists, or at least cannot just overdraw windows which are on separate grid. The concern would rather be to make sure that ext_messages is forward compatible with such a refactor, and I fail to see anything I have suggested or implemented would impede that (I would be happy to actually see your code, if it proves otherwise...)

Yes but the "somewhat of a break of the current external UI design" would be good to avoid, I guess.

As I already mentioned this will be very useful for other stuff. It would be the most straightforward way to implement :wincmd j (which must be synchronous, as it can be invoked as part of a script) when the UI has control over layout, rather than nvim. It would be huge and inconvenient limitation for future external UI work if nvim is forbidden to ever ask the UI a question...

@justinmk
Copy link
Member

:wincmd j ... It would be huge and inconvenient limitation for future external UI work if nvim is forbidden to ever ask the UI a question...

Hmm. If UI defines the layout, this is circular. What happens for winrestcmd() and getwininfo()?

@bfredl
Copy link
Member Author

bfredl commented Aug 11, 2018

If UI defines the layout, this is circular.

How is it circular? #8707 prototypes this today: nvim asks the UI for what window is below a specific handle, and it replies with the relevant window handle. Then nvim enters that window, exactly as if it has queried an internal datastructure as it does without ext_windows.

@bfredl
Copy link
Member Author

bfredl commented Aug 11, 2018

I see nothing in getwininfo() that is affected by this, but perhaps some vim patch will add neighbour info? winrestcmd() seems to only contain size info, it only restores after a resize (or open/close a temp window, so you get back again to where you were), not a complete change of layout?

@bfredl bfredl force-pushed the ext_messages branch 2 times, most recently from 5be4d98 to 431fcde Compare August 19, 2018 13:47
@ghost
Copy link

ghost commented Jan 7, 2019

Thanks for the latest commits and the helpful docs.

Sorry, I didn't realize that showmode was supposed to clear the recording message. I can confirm that it's working as intended now.

I can also verify that :messages sends the msg_history_show event with previous messages.

@bfredl bfredl force-pushed the ext_messages branch 2 times, most recently from ad0618d to 0c53ae5 Compare January 23, 2019 19:50
@bfredl
Copy link
Member Author

bfredl commented Jan 23, 2019

Multiline messages can now be concretely tested with lua:

:lua error("lol\nmultiline\nmessage")

@bfredl bfredl force-pushed the ext_messages branch 3 times, most recently from 1960318 to 15f515e Compare February 9, 2019 15:02
@bfredl
Copy link
Member Author

bfredl commented Feb 9, 2019

I think we should merge this, so it doesn't sit around forever. This PR now implements the MVP of letting the UI control the display of the message instead of grid. More fine grained control of the flow will be a follow-up (possibley based on feedback from UI implementers using the MVP).

@bfredl bfredl force-pushed the ext_messages branch 6 times, most recently from 5202c45 to 77eb680 Compare February 9, 2019 17:46
@@ -1124,6 +1163,24 @@ void set_keep_msg(char_u *s, int attr)
keep_msg_attr = attr;
}

void msg_set_ext_kind(const char *msg_kind)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msg_ext_set_kind

More generally, some of these flags seem potentially useful for the legacy message system. It's not clear to me if they should be considered "external only".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, their names are not public or anything, they could be changed later. (Also it is an useful ambiguity that ext also can mean "extended". If lua will render internal popupmenu from "ext" events, ext_popupmenu then means "extended popupmenu")

Co-Author: Dongdong Zhou <dzhou121@gmail.com>
@bfredl bfredl merged commit b3ce001 into neovim:master Feb 10, 2019
@justinmk justinmk removed the RFC label Feb 10, 2019
@@ -392,7 +398,7 @@ function Screen:expect(expected, attr_ids, attr_ignore)
.. ') differs from configured height(' .. #actual_rows .. ') of Screen.'
end
for i = 1, #actual_rows do
if expected_rows[i] ~= actual_rows[i] then
if expected_rows[i] ~= actual_rows[i] and expected_rows[i] ~= "{IGNORE}|" then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bfredl should this IGNORE feature be mentioned in the Screen:expect docstring?

Copy link
Contributor

@KillTheMule KillTheMule Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconded, I wasn't aware of it, and it will be pretty useful to me 👍 Although, I have to admit, I'd probably not have seen the docstring as well..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to add it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also some ext keys are missing, but that is not the same kind of deal: print_snapshot() will add them, so one doesn't need to know about them to use them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui-extensibility UI extensibility, events, protocol, externalized UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants