Are these 2 events needed ? since we already have ["mode_change", mode]

I would argue they are not synonymous. If we get mode_change to support cmdline, it should indicate cmdline regardless of cmdline_external. However these events are emitted only when cmdline_external, and the ui thus should draw the cmdline, though they should be called show and hide in consistency with popupmenu.

Can't this be deduced from ["cmdline", content, pos] as well ? or given as a parameter of ["cmdline", content, pos,submode={search/reverse-search/...}]. In fact all the events could be merged into that single one.

Merging with show into a single ["cmdline_show", content, pos, firstc, prompt] makes sense but I still think there should be a separate cmdline_pos, to easier avoid unnecessary redraws and it would also make the api consistent with popupmenu.

Maybe this logic should be moved to getcmdline_prompt. It would seem cleaner to have all display logic in ex_getln.c and not in eval.c (and remove the need for get_cmdline_external() )

instead of modifying the text this could be a separate event ["cmdline_char", char, shift], This would allow the gui to change the display by showing it in a different color etc.

@bfredl It looks like interaction with #6364 is going to be pretty straightforward: move color_cmdline call to the start of the function, stuff the results here. Only need to agree now how these results are transferred.

The only thing which bothers me is coloring of arabic shaping: I have no tests for this (if I am not mistaking, new Vim tests were not ported) and not sure what I need to do in first place.

At least to start with I think we can assume one single word (sequence of codepoints being shaped together) of arabic text will have just one color. So at the end of the if arabic_char() block we could check if the byte index is equal to or greater than the end of the current highlight chunk and then "flush" the arshape_buf so far with msg_outtrans_len_attr

Still, what about making content be a list of pairs [highlight group, string] (always one item content=[["Normal", text]] in this PR because cmdline highlighting PR is not finished yet)?

This is not safe, cmdfirstc is an int. (it works accidentally on a LE platform as long as firstc is ASCII)

firstc is only : / ? etc which is defined by nvim. So there's no so many of them.

Still, it will break on BE. We should avoid tricks like this. If we assue ASCII use buf[0] = (char)ccline.cmdfirstc; buf[1] = NUL.

One could avoid buf with (String) { .size = 1, .data = xmemdupz((char []) { (char)ccline.cmdfirstc }, 1) }.

At that point, wouldn't introducing char_to_string() be better?

this should probably just be inline

Why?

it is short, and not longer than the other branch in the calling function, we should not "hide away" ext ui code specifically, it is as important as the other branch.

Wouldn't it be helpful for all of the UI widget events to live in ui_ext_<event_name> functions?

Why? "cmdline_char" (or ui_event("cmdline_char" to be even preciser) is already greppable.

the pattern "certain classes of code must be in a separate function, even if it is very short and integral with the surrounding code" I have never heard of.

The API functions and foo_event handlers are somewhat analogous. "Externalized widgets" are special and should be called out. Regularity is really helpful. And given that these functions are part of a class as you noted, formalizing the name under a psuedo-namespace natural.

Most logic isn't trivially assignable to a class or pattern. Why not take the few opportunities that we can?

Consistency is good. But "consistency" is not an argument for doing X consistently over doing Y consistently. And we consistently try to not extract short functions that only will be called once and are heavily coupled to their surroundings.

And marshallers are not equivalent to extracting functions. Extracting functions manually adds yet another layer, located in a different place, one must analyse where values can be passed trough or module-globals maybe are used. Marshallers are sematically equal to calling ui_event directly.

The converse is probably not.

And this is not true. ui_event (only used for external stuff, not for TUI) is easy to grep for.

I don't disagree, and I strongly agree about avoiding needless indirection/abstraction. I see this case as an exception, it's a way of labeling an important special class of functionality. I'm not concerned about grepping.

Maybe my point is better illustrated by an example:

/// Undisplay the popup menu (later).
void pum_undisplay(void)
{
  pum_is_visible = false;
  pum_array = NULL;

  if (pum_external) {
    Array args = ARRAY_DICT_INIT;
    ui_event("popupmenu_hide", args);
  } else {
    redraw_all_later(SOME_VALID);
    redraw_tabline = true;
    status_redraw_all();
  }
}

The relevant abstraction here is already pum_undisplay(). If we add yet another layer, the actual ui_event (or maybe later ui_popupmenu_hide() auto generated wrapper), will be doubly divorced from the true source of the ui change, the calls to pum_undisplay(). In both this and the cmdline_putchar case that will be a very short function one is forced to analyze separately (from the true abstraction point) to make sure it don't have extra conditions etc. On the other side, ui_ext_tabline_show and cmdline_show makes sense as there actually are some logic going on. Also, it would make sense to extract the external logic in pum_display to one function, even if it might result in two different events. To put it bluntly, I don't see how a bad pattern becomes a good pattern just because one uses it consistently.

not needed, cstr_to_string is NULL safe.

won't a cmdline_show be sent right afterwards anyway? (so this event can be removed).

A separate cmdline_enter event can be helpful in this scenario:

:<C-R>=1+2<CR><CR>

The events would be cmdline_enter cmdline_enter cmdline_leave cmdline_leave

If we only have cmdline_show, the events will be:

cmdline_show cmdline_show cmdline_leave

You wouldn't know when you entered the "sub" cmdline.

I don't think the ui is really supposed to keep track of the mode stack-machine, it should just be told what to draw right now. When entering the sub cmdline it will see cmdline_show with the new relevant attributes. When leaving, it will see cmdline_hide immediately followed by cmdline_show for the outer command line. In the end it will always see consistent state.

I think we can leave the UI to decide whether they want to implement this complexity. What's the drawbacks to enable this new event?

Separation of concerns. We do want richer mode change events, but that's relevant not only for ui, and it doesn't belong in ext_cmdline. These events should just tell the ui the current cmdline state to draw.

I think it's the limitation of tui that we only draw the current state. But UI is cable to draw all the states. (they can ignore this anyway) I personally would love to see both of the commands at the same time. Isn't it the purpose of this PR to make the cmdline more presentable?

True, but that can be done in a state-less way by adding a level field to show and hide. If an ui attaches in the middle of cmdline, we do not need to "simulate" enter events for it to get a consistent world view. That will also incidentally work better with cmdline window. Then the first-level cmdline (if it ever was shown) will be hidden (as it is now drawn in a window), and if the user does : again, only the second-level cmdline will be shown.

(though it should probably be fixed in the TUI also, just print a new msgline, but that's another PR)

cmdline_hide

cchar_to_string((char)c) would be better, maybe (add to api/private/helpers.c)

/// Allocates a String consisting of a single char. Does not support multibyte
/// characters. The resulting string is also NUL-terminated, to facilitate
/// interoperating with code using C strings.
///
/// @param char the char to convert
/// @return the resulting String, if the input char was NUL, an
///         empty String is returned
String cchar_to_string(char c)
{
    char buf[] = { c, NUL };
    return (String) {
        .data = xmemdupz(buf, 1),
        .size = (c != NUL) ? 1 : 0
    };
}

2 space indent

2 space

2 space

can cmdline_pos and cmdline_char be combined into a general cmdline_update event (which may send other updates as well, in a map-like form)

@justinmk What was the point of this suggestion? UI events are already prefectly extensible by adding new events, and new fields to extisting events. The only effect this has is that ext ui contributors would have to write a lot of dict packing code that the metadata mechanism could have autogenerated for them... The point of #6618 was exactly to move away from ADD("x", ...); ADD("y", ...); ui_event() style code, so I wonder why we want to cancel the effect of that PR by recreating ui_event() as an inner platform...

Also, from the ui implementers view, the metadata cmdline_pos(Integer pos, Integer level) is more or less self-documenting, while cmdline_update(Dictionary) is positively useless.