Cleanup of QuickView handleMouseMove

[marcelgerber]

This one is for #6538 @jasonsanjose.
Unit tests still passing fine, everything seems to work.

Please notify me in case there are some more improvements that can be done.

[peterflynn]

@SAplayer We can't assume that the editor with focus is the editor the mouse is over (unless we want to drop functionality that exists today).

In some sense, the tooltip-show delay is already a massive debounce on the whole tooltip UI concept. But we're doing a lot of the tooltip-related work on every mousemove instead of only after the hover delay has been hit. We could wrap a debounce around the mousemove events, but since that's basically what our hover delay timer already does, I'd rather just move most of the logic inside the timeout handler.

The main thing blocking us fro doing that is the character bounds check. Tooltips typically tolerate a little bit of mouse motion and still allow the delay timer to keep running, eventually showing the tooltip. Ideally you tolerate any motion that doesn't leave the bounds of that particular tooltip's area. We can't do that since it requires polling all the providers upfront on (almost) every mouse move to establish what those bounds are. Instead our code adds a super rough tolerance factor by just permitting mousemoves that stay over top of the same character as the previous pos. It seems very reasonable to just convert that into a tolerance that's an absolute number of pixels instead, which then lets us move all the really heavy code onto the other side of the timeout.

Thoughts?

[marcelgerber]

Just added a commit to introduce the new function getHoveredEditor(), this makes the code somewhat clearer.

The real problem with checking changes in showPreview() is that we still need most of the things to hide the tooltip immediately (when the mouse is no longer in range, when the mouse is past the end of the line, ...).
And as these must be live, they have to be checked in handleMouseMove().

@peterflynn

[peterflynn]

@SAplayer Hiding is a simpler problem though -- once the hover preview is shown, we know the text range it applies to. We can store that range's bounds once, and then on each mousemove just check if the cursor has left those bounds. (Or even if we do the full computations, that's still only in the case where a popup is already open, so it's not going to impact general usage like scrolling).

So as of now, this PR cleans up the code but doesn't actually change anything about the performance or what code gets run when. I imagine we could merge this as is just as a cleanup, but #6538 would stay open...

[marcelgerber]

Yes, this is maybe the best idea. In this case, I or anyone else can do a "real" improvement on this code in another PR.

Isn't the detection of the hovered editor performance expensive? Do we need this when a popover is already open?

EDIT: Just changed the title (was: "Fix for #6538: Debounced QuickView handleMouseMove")
Btw, can you remove the "fix in progress" tag from #6538?

[peterflynn]

@SAplayer Good point, I've removed the "fix in progress" label.

I'm not sure how expensive the overall getHoveredEditor() chunk is. You could time it pretty simply by recording performance.now() at the start of the block of code and then again at the end, and printing out the difference (which will be a number in ms).

In general though, I don't think we should be as worried about the performance while a popup is visible. The more common case -- the one we really want to make sure is fast -- is when no popup is currently visible. That's the case that will occur constantly in general usage, e.g. just moving the mouse for other reasons or scrolling the editor around.

[marcelgerber]

getHoveredEditor() is around 0-1ms, the whole handleMouseMove() is around 1-2ms - maybe my computer is just too fast 😀

[peterflynn]

1-2ms isn't nothing! If you want 60fps smooth scrolling, you only get about 16ms total including the time it'll take the native code to recompute layout, repaint, etc. So getting handleMouseMove() down to near-0 for the common case (no popup visible) still seems worthwhile.

[redmunds]

I played around with this on win7 and I'm seeing 2 mousemove events for every requestAnimationFrame. So, first step might be to only keep track of mouse position delta for each mousemove event, and then process any mouse moves only once for each animation frame. After that code is in place, it could be throttled more than that if necessary using setTimeout() or setInterval().

[marcelgerber]

I'm not seeing double events often on Windows 8, 6926b2e, but I just submitted a little fix to this. Hope this helps.

[redmunds]

@SAplayer I don't think that code adds anything useful. When I said that I'm seeing "2 mousemove events for every requestAnimationFrame", I didn't mean that they were duplicated events. What I meant was that there are twice as many mousemove events sent as frames that you can show updates to user, so it doesn't make sense to process every event.

[redmunds]

@SAplayer @peterflynn I implemented my idea in pull request #6697. Let me know what you think.

[redmunds]

I updated PR #6697 to use the getHoveredEditor() function, so I think this one can be closed.