#4332 - Fix bug with 'onScrolled' event not firing in all cases #2143
Conversation
| @@ -760,6 +765,7 @@ export class NeovimInstance extends EventEmitter implements INeovimInstance { | |||
| break | |||
| case "scroll": | |||
| this.emit("action", Actions.scroll(a[0][0])) | |||
| this._dispatchScrollEvent() | |||
There was a problem hiding this comment.
@bryphe ah this was the line I was referring to which I though represented a scroll event from neovim, that was the what I was logging out which was providing a more accurate response to scrolling didnt realise it was from the redraw event
There was a problem hiding this comment.
Haha ya, I think the "bug" is that we weren't using it for our onScroll event 😄 So we were missing out on this info, which is the most reliable way to check for scroll events when the cursor doesn't move (it misses large scroll events, like G on a long file)
| if (this._isDisposed) { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
I was also logging here so I thought this bit being put into the the event loop rather than running synchronously was where things were going missing since if by some quirk of fate if the next scroll event came straight after and the pendingScrollTimeout still existed the function would just return so it might in effect be swallowing scroll events, didn't actually verify that this was happening
There was a problem hiding this comment.
Ah ya, we actually want it to not dispatch too aggressively - the "redraw" place is an especially performance-critical path, so we want to defer dispatching the scroll event until we're done drawing. And there is no point in queuing up multiple calls if we don't need to - we just want to make sure that a scroll event gets out in response to this. It's essentially 'debouncing' here.
There was a problem hiding this comment.
👍 Makes sense thanks for explaining
Codecov Report
@@ Coverage Diff @@
## master #2143 +/- ##
==========================================
- Coverage 37.15% 37.14% -0.01%
==========================================
Files 296 296
Lines 12125 12128 +3
Branches 1596 1596
==========================================
Hits 4505 4505
- Misses 7369 7372 +3
Partials 251 251
Continue to review full report at Codecov.
|
Issue: The
onScrollevent we expose fromNeovimInstance(and whichNeovimEditor/OniEditordepend on to power theonBufferScrolledevent for theEditorinterface) doesn't work reliably - it only dispatches when the cursor moves. However, there are lots of scroll cases where the cursor doesn't move, like:zzand scrolling by a single line (<C-e>)It's easy to repro by adding an event listener in the dev tools:
For those cases above, there will be no output.
Defect: Neovim doesn't expose a
Scrollautocommand, so we rely on a few things to power that event. The cursor moved events trigger this today, but there is also a redraw event that lets us know when there is an incremental scroll - we weren't using that at the moment.Fix: Hook up the
onScrollevent to use the scroll redraw message to work reliably in cases where the cursor doesn't move.Remaining work:
onBufferScrolledhandler build-over-build 😄