Ensure that active highlights are removed when a source view is closed (

#855)

Ensure that active highlights are removed when a source view is closed

This commit resolves a bug related to erroneously persisted highlights
in source files. It includes a unit test that verifies the correctness
of the fix, and a few minor improvements to the testing framework.

---

Bug Description: Before this change, source highlights would sometimes
persist after a source view was closed. This would happen, for example,
after the following sequence of steps:

1. Open a source. Ensure that some keyword is highlighted.
2. Activate the option "Toggle Split Editor (Horizontal)".
3. In the newly opened source view, highlight a different keyword.
4. At this point, both sets of highlights (the one from step 1 and the
one from step 3) should be visible in both source views. This is correct
behavior.
5. Close one of the source views, while keeping the other open. Before
this commit, both sets of highlights would persist. This is incorrect:
only the set of highlights related to the source view that remains open
should be displayed.

Cause: Each instance of HighlightReconcilingStrategy adds/removes
highlights independently, and keeps track internally of all the
highlights it has created. Before this commit, uninstalling a
HighlightReconcilingStrategy did not remove the active highlights.

Solution: When a source view is closed, all active highlights associated
with its HighlightReconcilingStrategy should be removed. We accomplish
this by calling `removeOccurrenceAnnotations()` during the `uninstall()`
method of `HighlightReconcilingStrategy.java`.

Testing: A unit test was created which approximately replicates the
sequence of steps outlined above. To implement this unit test, the
following changes were made in org.eclipse.lsp4e.test:

1. In HighlightTest.java, the function `assertAnnotationDoesNotExist`
was created (for consistency, the existing function
`assertHasAnnotation` was renamed to `assertAnnotationExists`).
2. The `mockDocumentHighlights` returned by the
`MockTextDocumentService` are now `Position`-dependent. The unit tests
in `HighlightTest.java` were adapted accordingly.

org.eclipse.lsp4e.test/META-INF/MANIFEST.MF

@@ -25,5 +25,7 @@ Require-Bundle: org.eclipse.core.runtime,
  org.eclipse.jdt.annotation;bundle-version="[2.0.0,3.0.0)",
  org.eclipse.ui.tests.harness,
  org.eclipse.ui.monitoring,
- org.eclipse.core.variables
+ org.eclipse.core.variables,
+ org.eclipse.e4.ui.model.workbench,
+ org.eclipse.e4.ui.workbench
 Automatic-Module-Name: org.eclipse.lsp4e.test

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/highlight/HighlightTest.java

@@ -9,6 +9,7 @@
  * Contributors:
  *  Michał Niewrzał (Rogue Wave Software Inc.) - initial implementation
  *  Sebastian Thomschke (Vegard IT GmbH) - refactoring to fix erratic test failures
+ *  Joao Dinis Ferreira (Avaloq Group AG) - Create testHighlightsInMultipleViewersForOneSource
  *******************************************************************************/
 package org.eclipse.lsp4e.test.highlight;
 
@@ -17,6 +18,7 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 
 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IProject;
@@ -26,6 +28,7 @@
 import org.eclipse.jface.text.source.Annotation;
 import org.eclipse.jface.text.source.IAnnotationModel;
 import org.eclipse.jface.text.source.ISourceViewer;
+import org.eclipse.lsp4e.LSPEclipseUtils;
 import org.eclipse.lsp4e.operations.highlight.HighlightReconcilingStrategy;
 import org.eclipse.lsp4e.test.utils.AllCleanRule;
 import org.eclipse.lsp4e.test.utils.TestUtils;
@@ -34,6 +37,7 @@
 import org.eclipse.lsp4j.DocumentHighlightKind;
 import org.eclipse.lsp4j.Position;
 import org.eclipse.lsp4j.Range;
+import org.eclipse.ui.IEditorReference;
 import org.junit.Assert;
 import org.junit.Assume;
 import org.junit.Before;
@@ -57,11 +61,12 @@ public void setUp() throws CoreException {
 	public void testHighlight() throws CoreException {
 		checkGenericEditorVersion();
 
-		MockLanguageServer.INSTANCE.setDocumentHighlights(List.of( //
-				new DocumentHighlight(new Range(new Position(0, 2), new Position(0, 6)), DocumentHighlightKind.Read),
-				new DocumentHighlight(new Range(new Position(0, 7), new Position(0, 12)), DocumentHighlightKind.Write),
-				new DocumentHighlight(new Range(new Position(0, 13), new Position(0, 17)), DocumentHighlightKind.Text) //
-		));
+		MockLanguageServer.INSTANCE.setDocumentHighlights(Map.ofEntries( //
+				Map.entry(new Position(0, 1), List.of( //
+						new DocumentHighlight(new Range(new Position(0, 2), new Position(0, 6)), DocumentHighlightKind.Read),
+						new DocumentHighlight(new Range(new Position(0, 7), new Position(0, 12)), DocumentHighlightKind.Write),
+						new DocumentHighlight(new Range(new Position(0, 13), new Position(0, 17)), DocumentHighlightKind.Text) //
+				))));
 
 		final IFile testFile = TestUtils.createUniqueTestFile(project, "  READ WRITE TEXT");
 		final ITextViewer viewer = TestUtils.openTextViewer(testFile);
@@ -72,9 +77,9 @@ public void testHighlight() throws CoreException {
 			sourceViewer.getTextWidget().setCaretOffset(1); // emulate cursor move
 
 			waitForAndAssertCondition(3_000, () -> {
-				assertHasAnnotion(annotationModel, HighlightReconcilingStrategy.READ_ANNOTATION_TYPE, 2, 4);
-				assertHasAnnotion(annotationModel, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 7, 5);
-				assertHasAnnotion(annotationModel, HighlightReconcilingStrategy.TEXT_ANNOTATION_TYPE, 13, 4);
+				assertAnnotationExists(annotationModel, HighlightReconcilingStrategy.READ_ANNOTATION_TYPE, 2, 4);
+				assertAnnotationExists(annotationModel, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 7, 5);
+				assertAnnotationExists(annotationModel, HighlightReconcilingStrategy.TEXT_ANNOTATION_TYPE, 13, 4);
 				return true;
 			});
 		} else {
@@ -85,10 +90,11 @@ public void testHighlight() throws CoreException {
 	@Test
 	public void testCheckIfOtherAnnotationsRemains() throws CoreException {
 		checkGenericEditorVersion();
-
-		MockLanguageServer.INSTANCE.setDocumentHighlights(List.of( //
-				new DocumentHighlight(new Range(new Position(0, 2), new Position(0, 6)), DocumentHighlightKind.Read) //
-		));
+
+		MockLanguageServer.INSTANCE.setDocumentHighlights(Map.ofEntries( //
+				Map.entry(new Position(0, 1), List.of( //
+						new DocumentHighlight(new Range(new Position(0, 2), new Position(0, 6)), DocumentHighlightKind.Read)
+				))));
 
 		final IFile testFile = TestUtils.createUniqueTestFile(project, "  READ WRITE TEXT");
 		final ITextViewer viewer = TestUtils.openTextViewer(testFile);
@@ -104,15 +110,85 @@ public void testCheckIfOtherAnnotationsRemains() throws CoreException {
 			viewer.getTextWidget().setCaretOffset(1); // emulate cursor move
 
 			waitForAndAssertCondition(3_000, () -> {
-				assertHasAnnotion(annotationModel, HighlightReconcilingStrategy.READ_ANNOTATION_TYPE, 2, 4);
-				assertHasAnnotion(annotationModel, fakeAnnotationType, fakeAnnotationPosition.offset,
+				assertAnnotationExists(annotationModel, HighlightReconcilingStrategy.READ_ANNOTATION_TYPE, 2, 4);
+				assertAnnotationExists(annotationModel, fakeAnnotationType, fakeAnnotationPosition.offset,
 						fakeAnnotationPosition.length);
 				return true;
 			});
 		}
 	}
 
-	private void assertHasAnnotion(IAnnotationModel annotationModel, String annotationType, int posOffset, int posLen) {
+	@Test
+	public void testHighlightsInMultipleViewersForOneSource() throws CoreException, InterruptedException {
+		checkGenericEditorVersion();
+
+		// Create a test file with two sets of highlights
+		final IFile testFile = TestUtils.createUniqueTestFile(project, "ONE\nTWO");
+
+		MockLanguageServer.INSTANCE.setDocumentHighlights(Map.ofEntries( //
+				Map.entry(new Position(0, 1), List.of( //
+						new DocumentHighlight(new Range(new Position(0, 0), new Position(0, 3)), DocumentHighlightKind.Write)
+				)),
+				Map.entry(new Position(1, 1), List.of( //
+						new DocumentHighlight(new Range(new Position(1, 0), new Position(1, 3)), DocumentHighlightKind.Write)
+				))));
+
+		// Open the first viewer
+		final ISourceViewer viewer1 = (ISourceViewer) TestUtils.openTextViewer(testFile);
+		final var annotationModel1 = viewer1.getAnnotationModel();
+
+		// Set the caret offset to activate the first set of highlights
+		viewer1.getTextWidget().setCaretOffset(1);
+
+		waitForAndAssertCondition(3_000, () -> {
+			assertAnnotationExists(annotationModel1, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 0, 3);
+			return true;
+		});
+
+		// Split the view in the active editor
+		List<IEditorReference> editorReferences = TestUtils.splitActiveEditor();
+
+		// Keep track of the newly opened editor, so we can close it later
+		ISourceViewer viewer2 = null;
+		IEditorReference editorToClose = null;
+		for (IEditorReference editorReference : editorReferences) {
+			ISourceViewer viewer = (ISourceViewer) LSPEclipseUtils.getTextViewer(editorReference.getEditor(false));
+			if (viewer != viewer1) {
+				editorToClose = editorReference;
+				viewer2 = viewer;
+				break;
+			}
+		}
+		Assert.assertNotNull(viewer2);
+
+		final var annotationModel2 = viewer2.getAnnotationModel();
+
+		// Set the caret offset to activate the second set of highlights
+		viewer2.getTextWidget().setCaretOffset(5);
+
+		// The annotation models of both viewers should now contain both sets of highlights
+		waitForAndAssertCondition(3_000, () -> {
+			assertAnnotationExists(annotationModel1, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 0, 3);
+			assertAnnotationExists(annotationModel1, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 4, 3);
+			assertAnnotationExists(annotationModel2, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 0, 3);
+			assertAnnotationExists(annotationModel2, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 4, 3);
+			return true;
+		});
+
+		// Close the second viewer
+		TestUtils.closeEditor(editorToClose.getEditor(false), false);
+
+		// Check that the highlights are the same as in the first case
+		waitForAndAssertCondition(3_000, () -> {
+			assertAnnotationExists(annotationModel1, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 0, 3);
+			assertAnnotationDoesNotExist(annotationModel1, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 4, 3);
+			assertAnnotationExists(annotationModel2, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 0, 3);
+			assertAnnotationDoesNotExist(annotationModel2, HighlightReconcilingStrategy.WRITE_ANNOTATION_TYPE, 4, 3);
+			return true;
+		});
+	}
+
+	private void assertAnnotationExists(IAnnotationModel annotationModel, String annotationType, int posOffset, int posLen) {
 		final var hasAnnotation = new boolean[] { false };
 		final var annotations = new ArrayList<String>();
 		annotationModel.getAnnotationIterator().forEachRemaining(anno -> {
@@ -129,8 +205,30 @@ private void assertHasAnnotion(IAnnotationModel annotationModel, String annotati
 		});
 
 		if (!hasAnnotation[0]) {
-			fail("Annotation of type [" + annotationType + "] not found at position {offset=" + posOffset + " length="
-					+ posLen + "}. Annotations found: " + annotations);
+			fail("Annotation of type [" + annotationType + "] not found at position {offset=" + posOffset + //
+					" length=" + posLen + "}. Annotations found: " + annotations);
+		}
+	}
+
+	private void assertAnnotationDoesNotExist(IAnnotationModel annotationModel, String annotationType, int posOffset, int posLen) {
+		final var hasAnnotation = new boolean[] { false };
+		final var annotations = new ArrayList<String>();
+		annotationModel.getAnnotationIterator().forEachRemaining(anno -> {
+			final var annoPos = annotationModel.getPosition(anno);
+			if (anno.getType().equals(annotationType) && annoPos.offset == posOffset && annoPos.length == posLen) {
+				hasAnnotation[0] = true;
+			}
+			annotations.add("Annotation[" + //
+					"type=" + anno.getType() + //
+					", text=" + anno.getText() + //
+					", offset=" + annoPos.offset + //
+					", length=" + annoPos.length + //
+					"]");
+		});
+
+		if (hasAnnotation[0]) {
+			fail("Unexpected annotation of type [" + annotationType + "] found at position {offset=" + posOffset + //
+					" length=" + posLen + "}. Annotations found: " + annotations);
 		}
 	}
 

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/utils/TestUtils.java

@@ -8,6 +8,7 @@
  *
  * Contributors:
  *  Michał Niewrzał (Rogue Wave Software Inc.) - initial implementation
+ *  Joao Dinis Ferreira (Avaloq Group AG) - Create splitActiveEditor
  *******************************************************************************/
 package org.eclipse.lsp4e.test.utils;
 
@@ -21,6 +22,7 @@
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 
@@ -31,6 +33,8 @@
 import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.NullProgressMonitor;
+import org.eclipse.e4.ui.model.application.ui.basic.MPart;
+import org.eclipse.e4.ui.workbench.IPresentationEngine;
 import org.eclipse.jface.text.ITextViewer;
 import org.eclipse.lsp4e.ContentTypeToLanguageServerDefinition;
 import org.eclipse.lsp4e.LSPEclipseUtils;
@@ -82,7 +86,20 @@ public static IEditorPart openEditor(IFile file) throws PartInitException {
 		part.setFocus();
 		return part;
 	}
-
+
+	public static List<IEditorReference> splitActiveEditor() {
+		IWorkbenchWindow workbenchWindow = UI.getActiveWindow();
+		IWorkbenchPage page = workbenchWindow.getActivePage();
+		IEditorPart part = page.getActiveEditor();
+
+		MPart editorPart = part.getSite().getService(MPart.class);
+		if (editorPart != null) {
+			editorPart.getTags().add(IPresentationEngine.SPLIT_HORIZONTAL);
+		}
+
+		return Arrays.asList(page.getEditorReferences());
+	}
+
 	public static IEditorPart openExternalFileInEditor(File file) throws PartInitException {
 		IWorkbenchWindow workbenchWindow = UI.getActiveWindow();
 		IWorkbenchPage page = workbenchWindow.getActivePage();

org.eclipse.lsp4e.tests.mock/META-INF/MANIFEST.MF

@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Mock Language Server to test LSP4E
 Bundle-SymbolicName: org.eclipse.lsp4e.tests.mock
-Bundle-Version: 0.16.5.qualifier
+Bundle-Version: 0.16.6.qualifier
 Bundle-Vendor: Eclipse LSP4E
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Require-Bundle: org.eclipse.lsp4j;bundle-version="[0.21.0,0.22.0)",

org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockLanguageServer.java

@@ -12,13 +12,15 @@
  *  Lucas Bullen (Red Hat Inc.) - Bug 508458 - Add support for codelens.
  *  Kris De Volder (Pivotal Inc.) - Provide test code access to Client proxy.
  *  Rubén Porras Campo (Avaloq Evolution AG) - Add support for willSaveWaitUntil.
+ *  Joao Dinis Ferreira (Avaloq Group AG) - Add support for position-dependent mock document highlights
  *******************************************************************************/
 package org.eclipse.lsp4e.tests.mock;
 
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CancellationException;
 import java.util.concurrent.CompletableFuture;
@@ -57,6 +59,7 @@
 import org.eclipse.lsp4j.LinkedEditingRanges;
 import org.eclipse.lsp4j.Location;
 import org.eclipse.lsp4j.LocationLink;
+import org.eclipse.lsp4j.Position;
 import org.eclipse.lsp4j.RenameOptions;
 import org.eclipse.lsp4j.ServerCapabilities;
 import org.eclipse.lsp4j.SignatureHelp;
@@ -237,7 +240,7 @@ public void setFormattingTextEdits(List<? extends TextEdit> formattingTextEdits)
 		this.textDocumentService.setMockFormattingTextEdits(formattingTextEdits);
 	}
 
-	public void setDocumentHighlights(List<? extends DocumentHighlight> documentHighlights) {
+	public void setDocumentHighlights(Map<Position, List<? extends DocumentHighlight>> documentHighlights) {
 		this.textDocumentService.setDocumentHighlights(documentHighlights);
 	}
 

org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockLanguageServerMultiRootFolders.java

@@ -11,12 +11,14 @@
  *  Mickael Istria (Red Hat Inc.) - added support for delays
  *  Lucas Bullen (Red Hat Inc.) - Bug 508458 - Add support for codelens
  *  Martin Lippert (Pivotal Inc.) - Bug 531030 - fixed crash when initial project gets deleted in multi-root workspaces
+ *  Joao Dinis Ferreira (Avaloq Group AG) - Add support for position-dependent mock document highlights
  *******************************************************************************/
 package org.eclipse.lsp4e.tests.mock;
 
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
@@ -41,6 +43,7 @@
 import org.eclipse.lsp4j.InitializeParams;
 import org.eclipse.lsp4j.InitializeResult;
 import org.eclipse.lsp4j.Location;
+import org.eclipse.lsp4j.Position;
 import org.eclipse.lsp4j.ServerCapabilities;
 import org.eclipse.lsp4j.SignatureHelp;
 import org.eclipse.lsp4j.SignatureHelpOptions;
@@ -186,7 +189,7 @@ public void setFormattingTextEdits(List<? extends TextEdit> formattingTextEdits)
 		this.textDocumentService.setMockFormattingTextEdits(formattingTextEdits);
 	}
 
-	public void setDocumentHighlights(List<? extends DocumentHighlight> documentHighlights) {
+	public void setDocumentHighlights(Map<Position, List<? extends DocumentHighlight>> documentHighlights) {
 		this.textDocumentService.setDocumentHighlights(documentHighlights);
 	}
 

org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockTextDocumentService.java

@@ -12,6 +12,7 @@
  *  Lucas Bullen (Red Hat Inc.) - Bug 508458 - Add support for codelens
  *  Pierre-Yves B. <pyvesdev@gmail.com> - Bug 525411 - [rename] input field should be filled with symbol to rename
  *  Rubén Porras Campo (Avaloq Evolution AG) - Add support for willSaveWaitUntil.
+ *  Joao Dinis Ferreira (Avaloq Group AG) - Add support for position-dependent mock document highlights
  *******************************************************************************/
 package org.eclipse.lsp4e.tests.mock;
 
@@ -21,6 +22,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.function.Function;
@@ -95,7 +97,7 @@ public class MockTextDocumentService implements TextDocumentService {
 	private SignatureHelp mockSignatureHelp;
 	private List<CodeLens> mockCodeLenses;
 	private List<DocumentLink> mockDocumentLinks;
-	private List<? extends DocumentHighlight> mockDocumentHighlights;
+	private Map<Position, List<? extends DocumentHighlight>> mockDocumentHighlights;
 	private LinkedEditingRanges mockLinkedEditingRanges;
 
 	private CompletableFuture<DidOpenTextDocumentParams> didOpenCallback;
@@ -165,8 +167,10 @@ public CompletableFuture<List<? extends Location>> references(ReferenceParams pa
 	}
 
 	@Override
-	public CompletableFuture<List<? extends DocumentHighlight>> documentHighlight(DocumentHighlightParams position) {
-		return CompletableFuture.completedFuture(mockDocumentHighlights);
+	public CompletableFuture<List<? extends DocumentHighlight>> documentHighlight(DocumentHighlightParams params) {
+		return CompletableFuture.completedFuture((mockDocumentHighlights != null) //
+				? mockDocumentHighlights.getOrDefault(params.getPosition(), Collections.emptyList())
+				: null);
 	}
 
 	@Override
@@ -390,7 +394,7 @@ public void setSignatureHelp(SignatureHelp signatureHelp) {
 		this.mockSignatureHelp = signatureHelp;
 	}
 
-	public void setDocumentHighlights(List<? extends DocumentHighlight> documentHighlights) {
+	public void setDocumentHighlights(Map<Position, List<? extends DocumentHighlight>> documentHighlights) {
 		this.mockDocumentHighlights = documentHighlights;
 	}
 
@@ -457,5 +461,5 @@ public void setFoldingRanges(List<FoldingRange> foldingRanges) {
 	@Override
 	public CompletableFuture<List<FoldingRange>> foldingRange(FoldingRangeRequestParams params) {
 		return CompletableFuture.completedFuture(this.foldingRanges);
-	} 
+	}
 }