Hover tooltip and Definitions move on scroll

@bryphe I've noted that while scrolling which is now more tolerable on mac, that the hover tooltip and the definitions follow the scroll, whereas I think the expected outcome with the tool tip is that it would close and the definition would disappear, it almost as if whatever controls those element's positioning is unaware of the scroll action, as the moment you click somewhere they both "do the right thing".

I'd be happy to take a look at this but would appreciate a heads up re where the code lives that controls how these elements respond.

Ah ya, I think this is potentially a dup of #1434 - it's definitely a visible and annoying issue. Agree that we should close when we scroll (it looks bizarre when they are just hanging out!)

I'd be happy to take a look at this but would appreciate a heads up re where the code lives that controls how these elements respond.

That would be awesome to have help! 👍

The place that controls this is the LangaugeEditorIntegration object here: https://github.com/onivim/oni/blob/479fd42a4912be847d69297f9ff81267804f160a/browser/src/Services/Language/LanguageEditorIntegration.ts#L51

This LanguageEditorIntegration owns a redux store that manages all the state regarding to language services, and bubbles this up into a couple simpler events:

onShowHover
onHideHover
onShowDefinition
onHideDefinition

There's a couple options:

Listen to a scroll event, add a SCROLL action, and hide the hover/definition through epics in the store
Listen to a scroll event, and just dispatch the 'hide' events directly on the LanguageEditorIntegration

One challenge is that I'm not sure we have a super reliable event for scrolling right now - so we might need to fix that first. We do have an onBufferScrolled event on the editor, but it only seems to dispatch when the cursor moves (which will only happen if the scroll hits the edge of the screen). There seems to be something wrong with how that is wired up - so we need that to reliably report scrolls to us, first.

Hope that helps! Let me know if you have questions around any particulars.

@bryphe apologies re the duplicate trying to avoid that so we don't drown in them 😆 .Thanks for the input that helps a lot still wasn't really sure where to go looking.

I've looked into this a bit and as far as I can tell from an initial exploration, it seems that neovim give us quite an accurate scroll event that picks up scrolling properly, but the event is handled here and from what I can tell the handling I think is made to fit a use case that doesn't quite match what the event returns.

I think the markdown plugin required some extra information on the scroll which led to adding an async function call to the scroll dispatch so that the actual event is dispatched asynchronously and after logging it seems loads of events are missed.

What the actual event returns is a number which I believe indicates the number of lines scrolled aka -3 for up 3 and 3 for down three.

My inclination was to have the scroll event match the data that neovim sends and I think if there are any use cases that require extra metadata somehow generate that outside of the scroll event just being transmitted, given the potential frequency and time sensitivity (in terms of ux) of the event.

Another potential solution is have an autocommand hook like we have for other events and have that return the context which is what the is currently expected and have the native event reported by neovim just used to respond to scrolling with not much expectation of contextual info.

Wanted to get your opinion before implementing anything here especially if it might end up needing to change the api

I've looked into this a bit and as far as I can tell from an initial exploration, it seems that neovim give us quite an accurate scroll event that picks up scrolling properly, but the event is handled here and from what I can tell the handling I think is made to fit a use case that doesn't quite match what the event returns.

Hmm, I looked here and perhaps the link is incorrect - we expose this event off of NeovimInstance:

onScroll: IEvent<EventContext>

But this isn't actually something that Neovim gives us - it's something we built on top and it's the thing that doesn't work 100%. Neovim doesn't have a ScrollChanged autocommand unfortunately, so we use a combination of strategies to detect the scroll. Ideally, this event should always dispatch when the buffer is scrolled, but that isn't the case today.

You can test it yourself by running this code in the console:

Oni.editors.activeEditor.neovim.onScroll.subscribe(() => console.log("scrolled"))

or, using:

Oni.editors.activeEditor.onBufferScrolled.subscribe((evt) => console.log("scroll: " + JSON.stringify(evt)))

You'll notice that it only fires when the cursor moves - but if the cursor is in the center of the screen, and you scroll just a couple of lines, that event won't get fired. Like, if you center the cursor with zz, and then press <C-e>, it won't fire. We'd have to fix this issue first, so that we can reliably detect scroll.

I think the markdown plugin required some extra information on the scroll which led to adding an async function call to the scroll dispatch so that the actual event is dispatched asynchronously and after logging it seems loads of events are missed.

I don't think this is the case, actually - AFAIK there is no Scroll autocommand, so we had to implement this event ourselves. Some discussion here: https://github.com/neovim/neovim/issues/4322

Another potential solution is have an autocommand hook like we have for other events and have that return the context which is what the is currently expected and have the native event reported by neovim just used to respond to scrolling with not much expectation of contextual info.

Hmm, what autocommand should we hook up? Is there one we are missing that can get us this info? I didn't see any that would help here, aside from the possibility of something like a 'Scroll' autocommand mentioned above.

Wanted to get your opinion before implementing anything here especially if it might end up needing to change the api

I don't think it should be necessary to change the API - this is the set of info we dispatch on a scroll:

                const convertedArgs: Oni.EditorBufferScrolledEventArgs = {
                    bufferTotalLines: args.bufferTotalLines,
                    windowTopLine: args.windowTopLine,
                    windowBottomLine: args.windowBottomLine,
                }

It's not clear to me why reducing / changing that would solve the problem. The core method that dispatches this event actually grabs all that for us: https://github.com/onivim/oni/blob/6bc376beb6c504c1b63115cf916b9d469a73d9a5/browser/src/neovim/NeovimInstance.ts#L669

I put together a PR that makes this scroll event more reliable by using the 'scroll' message we get from redraw (and also dispatching it on cursor move). I think that combination should be a pretty complete set to detect scroll - there are cases where a scroll can occur but we don't get the scroll draw message (like, if it ends up being a complete redraw) - so the cursor moving seems reasonable to catch those cases. It's in #2143 - let me know what you think!

Once we have the scroll event set up such that it's reliably reporting scrolls... then we can tackle the next piece of this PR - actually hiding the definition / quickinfo in response to a scroll 😀

onivim / oni

Hover tooltip and Definitions move on scroll #2138