Skip to content

Fix stale hover reuse for unchanged regions#1533

Open
betamaxbandit wants to merge 1 commit into
eclipse-lsp4e:mainfrom
betamaxbandit:fix1514
Open

Fix stale hover reuse for unchanged regions#1533
betamaxbandit wants to merge 1 commit into
eclipse-lsp4e:mainfrom
betamaxbandit:fix1514

Conversation

@betamaxbandit
Copy link
Copy Markdown

LSPTextHover reused a completed hover request when the viewer and hover region were unchanged. As a result, a repeated hover over the same region could return stale content and avoid issuing a fresh textDocument/hover request.

Fixes #1514

@rubenporras
Copy link
Copy Markdown
Contributor

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

LSPTextHover reused a completed hover request when the viewer and hover
region were unchanged. As a result, a repeated hover over the same
region could return stale content and avoid issuing a fresh
textDocument/hover request.

A previous request is still reused while it is in progress for the same
viewer/region, but any completed requests are now treated as stale, so a
subsequent hover on the same region starts a fresh textDocument/hover
request instead of reusing the old completed result indefinitely.

Fixes eclipse-lsp4e#1514
@betamaxbandit
Copy link
Copy Markdown
Author

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed.
The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.

I've updated the commit message appropriately.

Cheers John

@betamaxbandit
Copy link
Copy Markdown
Author

Hi @rubenporras ,

I see the following failure for my PR:

continuous-integration/jenkins/pr-merge — This commit cannot be built

I'm not sure what I need to do, if anything. Is it simply waiting for a committer to approve the run?

@betamaxbandit
Copy link
Copy Markdown
Author

Hi @rubenporras ,

I see the following failure for my PR:

continuous-integration/jenkins/pr-merge — This commit cannot be built

I'm not sure what I need to do, if anything. Is it simply waiting for a committer to approve the run?

Hi @rubenporras ,
can you help me progress this please?
Cheers John

@FlorianKroiss
Copy link
Copy Markdown
Contributor

I haven't reviewed the code (yet), but I approved the workflow runs

@rubenporras
Copy link
Copy Markdown
Contributor

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed. The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.

I've updated the commit message appropriately.

Cheers John

I think this will trigger many unneeded recomputations. Could we do it so that we invalidate the last computed hover request only if the editor looses focus?

That should cover the case of external resources and be less agressive on the computation side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LSPTextHover reuses completed hover result for unchanged region and does not issue a new textDocument/hover request

3 participants