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

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

Merged
merged 2 commits into from Oct 18, 2023

Conversation

joaodinissf
Copy link
Contributor

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.

@joaodinissf joaodinissf marked this pull request as ready for review October 18, 2023 14:30
Copy link
Contributor

@rubenporras rubenporras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@rubenporras
Copy link
Contributor

As additional information, the same problem can be reproduced by opening the same file in two editors (instead of using an split editor).
The reason is that if there is more than a view on a text file buffer, the buffer is not discarded when closing the buffer. Then the annotations that are stored in the ITextFileBuffer survive the closing of the viewer.

@rubenporras
Copy link
Contributor

@joaodinissf , you need to bump the version in the MANIFEST.MF and pom.xml of org.eclipse.lsp4e.tests.mock for the build to pass.

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.
@rubenporras rubenporras merged commit a339f14 into eclipse:master Oct 18, 2023
2 checks passed
@joaodinissf joaodinissf deleted the fix-cached-highlights branch October 19, 2023 12:08
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.

None yet

2 participants