Skip to content

Commit b52d4ec

Browse files
committed
Fix stale hover reuse for unchanged regions
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 #1514
1 parent b912717 commit b52d4ec

2 files changed

Lines changed: 84 additions & 16 deletions

File tree

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/hover/HoverTest.java

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2016 Rogue Wave Software Inc. and others.
2+
* Copyright (c) 2016, 2026 Rogue Wave Software Inc. and others.
33
* This program and the accompanying materials are made
44
* available under the terms of the Eclipse Public License 2.0
55
* which is available at https://www.eclipse.org/legal/epl-2.0/
@@ -235,4 +235,55 @@ public void completed(ProgressEvent event) {
235235
shell.dispose();
236236
}
237237
}
238+
239+
@Test
240+
public void testHoverRegionRefreshesForSameOffsetAfterCompletedRequest() throws Exception {
241+
// Test for https://github.com/eclipse-lsp4e/lsp4e/issues/1514
242+
// Verifies that getHoverRegion refreshes for the same offset after a completed
243+
// request, instead of reusing the previous completed hover range indefinitely.
244+
final var firstHover = new Hover(List.of(Either.forLeft("FirstValue")),
245+
new Range(new Position(0, 0), new Position(0, 5)));
246+
final var secondHover = new Hover(List.of(Either.forLeft("SecondValue")),
247+
new Range(new Position(0, 6), new Position(0, 10)));
248+
249+
IFile file = TestUtils.createUniqueTestFile(project, "HoverRange Other Text");
250+
ITextViewer viewer = TestUtils.openTextViewer(file);
251+
252+
MockLanguageServer.INSTANCE.setHover(firstHover);
253+
assertEquals(new Region(0, 5), hover.getHoverRegion(viewer, 2));
254+
255+
MockLanguageServer.INSTANCE.setHover(secondHover);
256+
assertEquals(new Region(6, 4), hover.getHoverRegion(viewer, 2));
257+
}
258+
259+
@Test
260+
public void testHoverInfoRefreshesForSameOffsetAfterCompletedRequest() throws Exception {
261+
// Test for https://github.com/eclipse-lsp4e/lsp4e/issues/1514
262+
// Verifies that a second hover at the same offset recomputes the hover region
263+
// and refreshes the hover content after the previous request completed.
264+
final var firstHover = new Hover(List.of(Either.forLeft("FirstValue")),
265+
new Range(new Position(0, 0), new Position(0, 5)));
266+
final var secondHover = new Hover(List.of(Either.forLeft("SecondValue")),
267+
new Range(new Position(0, 6), new Position(0, 10)));
268+
269+
IFile file = TestUtils.createUniqueTestFile(project, "HoverRange Other Text");
270+
ITextViewer viewer = TestUtils.openTextViewer(file);
271+
272+
MockLanguageServer.INSTANCE.setHover(firstHover);
273+
Region firstRegion = (Region) hover.getHoverRegion(viewer, 2);
274+
assertEquals(new Region(0, 5), firstRegion);
275+
276+
String firstHtml = hover.getHoverInfoFuture(viewer, firstRegion).get(2, TimeUnit.SECONDS);
277+
assertNotNull(firstHtml);
278+
assertTrue(firstHtml.contains("FirstValue"));
279+
280+
MockLanguageServer.INSTANCE.setHover(secondHover);
281+
Region secondRegion = (Region) hover.getHoverRegion(viewer, 2);
282+
assertEquals(new Region(6, 4), secondRegion);
283+
284+
String secondHtml = hover.getHoverInfoFuture(viewer, secondRegion).get(2, TimeUnit.SECONDS);
285+
assertNotNull(secondHtml);
286+
assertTrue(secondHtml.contains("SecondValue"));
287+
assertTrue(!secondHtml.contains("FirstValue"));
288+
}
238289
}

org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/hover/LSPTextHover.java

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2016, 2023 Red Hat Inc. and others.
2+
* Copyright (c) 2016, 2026 Red Hat Inc. and others.
33
* This program and the accompanying materials are made
44
* available under the terms of the Eclipse Public License 2.0
55
* which is available at https://www.eclipse.org/legal/epl-2.0/
@@ -16,8 +16,6 @@
1616
*******************************************************************************/
1717
package org.eclipse.lsp4e.operations.hover;
1818

19-
import static org.eclipse.lsp4e.internal.NullSafetyHelper.castNonNull;
20-
2119
import java.util.List;
2220
import java.util.NoSuchElementException;
2321
import java.util.Objects;
@@ -98,10 +96,18 @@ public class LSPTextHover implements ITextHover, ITextHoverExtension, ITextHover
9896
}
9997

10098
public CompletableFuture<@Nullable String> getHoverInfoFuture(ITextViewer textViewer, IRegion hoverRegion) {
101-
if (this.request == null || !textViewer.equals(this.lastViewer) || !hoverRegion.equals(this.lastRegion)) {
99+
CompletableFuture<List<Hover>> locRequest = this.request;
100+
if (locRequest == null || locRequest.isDone() || !textViewer.equals(this.lastViewer)
101+
|| !hoverRegion.equals(this.lastRegion)) {
102102
initiateHoverRequest(textViewer, hoverRegion.getOffset());
103+
locRequest = this.request;
104+
}
105+
106+
if (locRequest == null) {
107+
return CompletableFuture.completedFuture(null);
103108
}
104-
return castNonNull(request).thenApply(hoversList -> {
109+
110+
return locRequest.thenApply(hoversList -> {
105111
String result = hoversList.stream() //
106112
.filter(Objects::nonNull) //
107113
.map(LSPTextHover::getHoverString) //
@@ -110,9 +116,8 @@ public class LSPTextHover implements ITextHover, ITextHoverExtension, ITextHover
110116
.trim();
111117
if (!result.isEmpty()) {
112118
return MarkdownUtil.renderToHtml(result);
113-
} else {
114-
return null;
115119
}
120+
return null;
116121
});
117122
}
118123

@@ -149,28 +154,40 @@ public class LSPTextHover implements ITextHover, ITextHoverExtension, ITextHover
149154
@Override
150155
public @Nullable IRegion getHoverRegion(ITextViewer textViewer, int offset) {
151156
final var lastRegion = this.lastRegion;
152-
if (this.request == null || lastRegion == null || !textViewer.equals(this.lastViewer)
153-
|| offset < lastRegion.getOffset() || offset > lastRegion.getOffset() + lastRegion.getLength()) {
157+
158+
CompletableFuture<List<Hover>> locRequest = this.request;
159+
if (locRequest == null || locRequest.isDone() || lastRegion == null || !textViewer.equals(this.lastViewer)
160+
|| offset < lastRegion.getOffset() || offset >= lastRegion.getOffset() + lastRegion.getLength()) {
154161
initiateHoverRequest(textViewer, offset);
162+
locRequest = this.request;
155163
}
156164

157165
final IDocument document = textViewer.getDocument();
158166
if (document == null) {
159167
return null;
160168
}
161169

170+
if (locRequest == null) {
171+
return this.lastRegion = computeHeuristicRegion(document, offset);
172+
}
173+
162174
try {
163175
// Wait shortly for hover region result, fallback to heuristics if LS is laggy
164-
Range range = castNonNull(this.request).get(GET_HOVER_REGION_TIMEOUT_MS, TimeUnit.MILLISECONDS).stream() //
176+
List<Hover> hovers = locRequest.get(GET_HOVER_REGION_TIMEOUT_MS, TimeUnit.MILLISECONDS);
177+
178+
Range range = hovers.stream() //
165179
.filter(Objects::nonNull) //
166180
.map(Hover::getRange) //
167181
.filter(Objects::nonNull) //
168182
.reduce((first, second) -> second) //
169-
.get();
170-
int regionStartOffset = Math.max(0,
171-
LSPEclipseUtils.toOffset(range.getStart(), document));
172-
int regionEndOffset = Math.min(document.getLength(),
173-
LSPEclipseUtils.toOffset(range.getEnd(), document));
183+
.orElse(null);
184+
185+
if (range == null) {
186+
return this.lastRegion = computeHeuristicRegion(document, offset);
187+
}
188+
189+
int regionStartOffset = Math.max(0, LSPEclipseUtils.toOffset(range.getStart(), document));
190+
int regionEndOffset = Math.min(document.getLength(), LSPEclipseUtils.toOffset(range.getEnd(), document));
174191
return this.lastRegion = new Region(regionStartOffset, regionEndOffset - regionStartOffset);
175192
} catch (ExecutionException | BadLocationException e) {
176193
if (!CancellationUtil.isRequestCancelledException(e)) {

0 commit comments

Comments
 (0)