Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Links get re-evaluated when buffer changes happen outside viewport #4814

Closed
Tyriar opened this issue Sep 19, 2023 · 6 comments · Fixed by #4820
Closed

Links get re-evaluated when buffer changes happen outside viewport #4814

Tyriar opened this issue Sep 19, 2023 · 6 comments · Fixed by #4820
Assignees
Labels
area/links type/bug Something is misbehaving
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Sep 19, 2023

Repro:

  1. Run ls to fill viewport
  2. Open vscode repo in terminal
  3. Echo a url
  4. Run ./scripts/test
  5. Scroll up while that's running and hover the url, the underline will flicker

The issue is confusion between requesting re-rendering of dirty rows relative to ybase:

// Refresh any dirty rows accumulated as part of parsing
this._onRequestRefreshRows.fire(this._dirtyRowTracker.start, this._dirtyRowTracker.end);

Then incorrectly firing onRenderedViewportChange incorrectly even when the viewport isn't re-rendered when scrolled up:

if (!this._isNextRenderRedrawOnly) {
this._onRenderedViewportChange.fire({ start, end });
}

And assuming that the event is relative to ydisp:

this._linkCacheDisposables.push(this._renderService.onRenderedViewportChange(e => {
// Sanity check, this shouldn't happen in practice as this listener would be disposed
if (!this._currentLink) {
return;
}
// When start is 0 a scroll most likely occurred, make sure links above the fold also get
// cleared.
const start = e.start === 0 ? 0 : e.start + 1 + this._bufferService.buffer.ydisp;
const end = this._bufferService.buffer.ydisp + 1 + e.end;

@Tyriar Tyriar added area/links type/bug Something is misbehaving labels Sep 19, 2023
@Tyriar Tyriar added this to the 5.4.0 milestone Sep 19, 2023
@Tyriar Tyriar self-assigned this Sep 19, 2023
@jerch
Copy link
Member

jerch commented Sep 20, 2023

@Tyriar Wow, nice find. Could this also be the reason for the selection events being fired over and over while input flows, although there is no active selection?

@tisilent
Copy link
Contributor

Links in tests.....
无标题123

@tisilent
Copy link
Contributor

20230920_091001.mp4

Flashing like this.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 20, 2023

There seems to be a bigger problem here where we often render when nothing happens just from input that was written below. Terminal.refresh is called for the viewport (relative to ydisp), but often (not always) the intent is to refresh the bottom viewport only.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 20, 2023

@tisilent that's probably some Windows-only wrapping/reflow issue.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 20, 2023

@jerch yeah probably. Do we have an issue tracking that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/links type/bug Something is misbehaving
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants