API/UI: ui_event_extmark

[yatli]

No description provided.

[justinmk]

ui_call_win_extmarks(..) in win_update seems noisy. Also seems a bit special case. Some alternative ideas:

• clients/subscribers can "attach" a grid, analogous to nvim_buf_attach()
• or make it a global event like nvim_error_event.
  • this will also be noisy, so we will need to introduce a subscription/opt-in mechanism for all clients. (Precedent: nvim_ui_attach())

[bfredl]

@justinmk not doing it in win_update would completely defeat the purpose of this PR though. The UI wants to know at ui_flush() that the extmark positions is in sync with the grid positions, so it can present a correct screen state to the user directly, including extmark tagged elements (and not have extmarks glitch around whenever a secondary event stream is flushed).

Also this is the first WIP draft from someone who is relatively new to the C code (and a pretty good one, for that). Let's take some time to clean it up and simplify some things (like the "lookless" stuff should just be reduced into ordinary decorations), before passing it off as too special and noisy and attempting a different implementation from scratch, which are bound to have other complexities, like re-calculating all positions which simply are just there for us for free inside the win_update code paths.

[yatli]

@justinmk initially I proposed exactly that ext_extmark to bfredl :p
The reason for choosing the current path is that, the UI may not be interested in all extmarks so it can choose to opt-in fine-grained on some namespaces. e.g. one namespace for pictures, one for collaborative editing guest cursors, and so on, but not codelens, in-line git blame, lsp error msgs etc. which are handled by other plugins.

[justinmk]

initially I proposed exactly that ext_extmark to bfredl :p
The reason for choosing the current path is that, the UI may not be interested in all extmarks so it can choose to opt-in fine-grained on some namespaces. e.g. one namespace for pictures, one for collaborative editing guest cursors,

Makes sense. Keeping it mind that nothing I say is blocking this PR, I wonder if ...

• we should not introduce "watch" as a concept since we already have "attach". IOW, rename "watch" to "attach" here?
  • or use "subscribe" or "listen" instead of "watch".
  • or rename nvim_buf_attach to nvim_buf_watch? (Or explain what is the difference?)
• are we actually watching a namespace, not extmarks? Thus nvim_attach_namespace ?
• should nvim_ui_watch_extmark be channel-agnostic (not only for UIs)?

Thus the signature might be:

/// Subscribes a channel or Lua callbacks to events in namespace {ns_id}.
///
/// @param ns_id Namespace to subscribe to.
/// @param ui If true, data is published in the UI redraw.win_extmark, redraw.win_highlight events.
/// @param opt option keys:
///            - events: list of events in namespace {ns_id} to listen to, any
///              of: "extmark", "highlight"
///            - on_extmark: Lua callback invoked on namespaced extmark updates.
///            - on_highlight: Lua callback invoked on namespaced highlight updates.
/// @return false if ns_id is invalid, else true
void nvim_attach_namespace(uint64_t channel_id, Integer ns_id, Dictionary opt) 

[yatli]

In my mind, "watch" is for a specific type of message, and "attach" is like a whole-sale solution -- attach to buffer means to receive everything related to the buffer.
We can say "nvim_attach_namespace" but it's a different scope than what I have done here.
(or, just use lua/vimL on CursorHold/TextChanged etc. to list the extmarks and problem solved)

Also, this is ui-specific (hence nvim_ui_watch_extmarks) because I'm all interested in the calculated on-screen grid positions of marks, not just the buffer positions -- this information cannot be obtained anywhere else.
Letting non-ui plugins to receive such messages probably goes across the abstraction layers (does the non-ui plugins ever handle anything "grid"?).

Edit: "watch" doesn't carry the idea that we want grid cell positions either... Thoughts?

[yatli]

@bfredl a few thoughts for dc0e7fa:

I understand that non-text marks should be filtered as early as possible, so I moved it out from draw_virt_text to decor_add.
Since DecorRange actually copies out the Decoration, it's easy to do the filtering, and conditionally set a new field ui_watched to indicate that the ui client is interested in this mark.

"lookless" stuff has been merged with ordinary decors. Debugged this a few rounds, the "lookless marks" do not have altpos (set to -1). The decor adding predicates are updated accordingly.

TODO: can get even earlier? -- put ui_watched in ExtMarkItem and compute this on mark creation/update, and watch updates.

[yatli]

@bfredl 9875086 fixes corner cases mentioned above.
Specifically, if we are redrawing the whole window, we are not sure about the previous extmarks state and a redraw-all is not a scroll, so the client cannot infer correctly -- win_extmarks_clear should be sent.

[yatli]

one more edge case fixed -- when the viewport scrolls up but there's no text updates (all empty lines), invalidate extmarks.

[yatli]

but there are still more. Higher order ones 🙀
Like, have two windows of the same buffer, insert <cr> to push a mark out of both viewports.
The focused one gets clear marks event, the other one doesn't, until going back to normal mode 😅
@bfredl could you help?

[yatli]

my conclusion is that it's not appropriate to send win_extmarks_clear in win_update because it's incremental and does not have the full information of the window.

[bfredl]

that's fine. as extmarks are anchored to cell grids, they will be cleared when the line is redrawn, and should also scroll with the line (i e it should be treated as an extra attribute to the cell).

[yatli]

A frontend can implement something like this:

yatli/fvim@6619106

so there are two sets of tracked extmarks -- one is for those associated with the on-screen cells, and the other is for scrolled-out ones which can be tracked with win_viewport.

This allows image display clipped by window top.

[yatli]

@bfredl rebased on virtual lines.

[bfredl]

@yatli would you mind write a simple test or two? I'm happy to give guidance as needed.

[yatli]

Wilco. I was waiting for the shape of this PR to settle down before moving to tests, and it looks like the time is now.

[yatli]

@bfredl how do I receive events from a screen(ui)?
helpers.expect_msg_seq?

I see there's also screen:expect_unchanged(..., request_cb)
Also session:run(...)

Not sure which one to use.

Edit:
Hmm screen:expect has its own notification_cb that handles redraw. Not a good idea to intercept.

@yatli look here in case you forget:

test/functional/ui/screen.lua:L477 function Screen:_wait(check, flags)
test/functional/ui/screen.lua:L516 local function notification_cb(method, args)
test/functional/ui/screen.lua:L617 function Screen:_redraw(updates)

[bfredl]

@yatli indeed. instead of trying to "intercept" screen.lua, we have to implement support there, perhaps not too unlike how a "real" UI would. so we should write a function like Screen:_handle_win_extmarks (actually should be singular: win_extmark ) and save state in a new self variable. then we can assert the value after event processing is done, like for other extended attributes.

[yatli]

@bfredl 224bbc2

nothing goes into self._grid_win_extmarks... ?
The handler is not reached.

[bfredl]

@yatli try screen:redraw_debug() before the expect. Then we can see the exact stream of events (if the event is missing or malformatted, or something)

[yatli]

...
{ "win_viewport", { 2, {
      id = 1000,
      <metatable> = <1>{
        __index = <table 1>,
        new = <function 1>
      }
    }, 0, 3, 1, 14, 2 } }
{ "grid_line", { 1, 0, 0, { { "n", 0 }, { "o" }, { "n" }, { " " }, { "u" }, { "i" }, { "-" }, { "w" }, { "a" }, { "t" }, { "c" }, { "h" }, { "e" }, { "d" }, { " " }, { "l" }, { "i" }, { "n" }, { "e" } } }, { 1, 1, 0, { { "u", 0 }, { "i" }, { "-" }, { "w" }, { "a" }, { "t" }, { "c" }, { "h" }, { "e" }, { "d" }, { " " }, { "l" }, { "i" }, { "n" }, { "e" } } }, { 1, 2, 0, { { "~", 7 }, { " ", 7, 19 } } }, { 1, 3, 0, { { " ", 1, 20 } } } }
{ "win_viewport", { 2, {
      id = 1000,
      <metatable> = <1>{
        __index = <table 1>,
        new = <function 1>
      }
    }, 0, 3, 1, 14, 2 } }
{ "grid_cursor_goto", { 1, 1, 14 } }
{ "flush", {} }

screen:expect{grid=[[
  non ui-watched line |
  ui-watched lin^e     |
  {1:~                   }|
                      |
]], attr_ids={
  [1] = {foreground = Screen.colors.Blue1, bold = true};
}}

{ "mode_change", { "normal", 0 } }
{ "mouse_off", {} }
{ "flush", {} }

screen:expect{grid=[[
  non ui-watched line |
  ui-watched lin^e     |
  {1:~                   }|
                      |
]], attr_ids={
  [1] = {foreground = Screen.colors.Blue1, bold = true};
}}

{ "win_extmark", { 2, {
      id = 1000,
      <metatable> = <1>{
        __index = <table 1>,
        new = <function 1>
      }
    }, 1, 1, 0, 20 } }
{ "win_viewport", { 2, {
      id = 1000,
      <metatable> = <1>{
        __index = <table 1>,
        new = <function 1>
      }
    }, 0, 3, 1, 14, 2 } }
{ "flush", {} }

screen:expect{grid=[[
  non ui-watched line |
  ui-watched lin^e     |
  {1:~                   }|
                      |
]], attr_ids={
  [1] = {foreground = Screen.colors.Blue1, bold = true};
}}

The event is present.
I tried to add error('hey') in _handle_win_extmark but that didn't trigger an error.

[yatli]

oka removed table.remove and added redraw_debug or sleep and the msg loop is pumped correctly.

[yatli]

@bfredl tests ready

[yatli]

Done the changes.
The errors (of not having sleep and unchanged) are gone now.

Is that what you mean by "checked multiple times"? That the error should not break the execution flow.

[bfredl]

yes. it will check against the conditions each time there is a "flush" event. so it must be prepared to check state multiple times. than screen:except can work "async" all by itself.

[bfredl]

Thanks, merged!

We might want more docs/tests for how to properly interact with grid_scroll et al, though that could be a follow up. This is good enough for UI:s to start using it.