[#934] fix removed folding annotations on document change #935

ghentschke · 2024-02-28T11:05:45Z

The method
ProjectionAnnotationModel.modifyAnnotations(Annotation[], Map<? extends Annotation, ? extends Position>, Annotation[]) does not consider Position changes in any cases. On large annotations the positions won't get updated properly which leads to removed folding annotations.

fixes #934

The method ProjectionAnnotationModel.modifyAnnotations(Annotation[], Map<? extends Annotation, ? extends Position>, Annotation[]) does not consider Position changes in any cases. On large annotations the positions won't get updated properly which leads to removed folding annotations. fixes eclipse-lsp4e#934

ghentschke · 2024-02-28T11:08:23Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/folding/LSPFoldingReconcilingStrategy.java

@@ -269,9 +269,7 @@ protected void updateAnnotations(Annotation existingAnnotation, Position newPos,
 				Position oldPos = theProjectionAnnotationModel.getPosition(foldingAnnotation);
 				// only update the position if we have to
 				if (!newPos.equals(oldPos)) {
-					oldPos.setOffset(newPos.offset);
-					oldPos.setLength(newPos.length);
-					modifications.add(foldingAnnotation);


modifications can be removed from the arguments list of this method. But this would be an API break.

I do not think there will be many subclasses of LSPFoldingReconcilingStrategy, so I would prefer that we modify the signature accordingly (or probably just remove the method, as it does not update anything any longer.)

@mickaelistria , what do you think?

FYI Mickael is not available this week.

Since Mickael is not available this week, could you mark the method as deprecated for removal and document on the javadoc that the parameter is unused? Then we can cleanup on a later iteration.

rubenporras · 2024-02-29T07:38:58Z

Are we fixing a problem with ProjectionAnnotationModel.modifyAnnotations? Or how is it that "On large annotations the positions won't get updated properly which leads to removed folding annotations"?

ghentschke · 2024-02-29T09:01:25Z

Are we fixing a problem with ProjectionAnnotationModel.modifyAnnotations?

Not really. IMO we haven't used the proper method. And theProjectionAnnotationModel.modifyAnnotationPosition(foldingAnnotation, newPos)
does exactly what we want: update the position of a Annotation.

rubenporras · 2024-02-29T09:20:46Z

Are we fixing a problem with ProjectionAnnotationModel.modifyAnnotations?

Not really. IMO we haven't used the proper method. And theProjectionAnnotationModel.modifyAnnotationPosition(foldingAnnotation, newPos) does exactly what we want: update the position of a Annotation.

I guess that you mean that call projectionAnnotationModel.modifyAnnotations(deletions.toArray(new Annotation[1]), additions, modifications.toArray(new Annotation[0])) is not the proper method. Why not?

ghentschke · 2024-02-29T09:43:30Z

I guess that you mean that call projectionAnnotationModel.modifyAnnotations(deletions.toArray(new Annotation[1]), additions, modifications.toArray(new Annotation[0])) is not the proper method. Why not?

That was my assumption I made after some debugging to determine why the folding annotations disappear as described in my issue #934.
Unfortunately, I had not the time to determine the root cause why the previous method does not work on large files. The theProjectionAnnotationModel.modifyAnnotationPosition(foldingAnnotation, newPos) seems to be more appropriate for what we wanted and it works as expected also on large files.

- Users should use the new method with updated signature.

rubenporras · 2024-03-04T09:11:58Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/folding/LSPFoldingReconcilingStrategy.java

@@ -151,7 +150,7 @@ private void applyFolding(List<FoldingRange> ranges) {
 			if (ranges != null) {
 				Collections.sort(ranges, Comparator.comparing(FoldingRange::getEndLine));
 				for (FoldingRange foldingRange : ranges) {
-					updateAnnotation(modifications, deletions, existing, additions, foldingRange.getStartLine(),
+					updateAnnotation(deletions, existing, additions, foldingRange.getStartLine(),


If we think anyone could be using the deprecated method, we still need to call it, otherwise we would be breaking the customized code.
For me, if you could revert to use the old method, we could merge.

we still need to call it, otherwise we would be breaking the customized code

Hm, I am not sure I am understand you here. The modifications is always empty, because its not written anymore in LSPFoldingReconcilingStrategy.updateAnnotations(existingAnnotation, newPos, deletions). The updateAnnotation which is called here in line 153 is a private method.

Theres no need to call the deprecated LSPFoldingReconcilingStrategy.updateAnnotations(existingAnnotation, newPos, modifications, deletions) method here.

Hm, I am not sure I am understand you here. The modifications is always empty, because its not written anymore in LSPFoldingReconcilingStrategy.updateAnnotations(existingAnnotation, newPos, deletions)

Yes, the method as we have rewritten it will return empty annotations, but if some subclass subclassed the method, we still need to call it, otherwise we would break the customization.

rubenporras · 2024-03-04T10:01:33Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/folding/LSPFoldingReconcilingStrategy.java

@@ -167,8 +167,7 @@ private void applyFolding(List<FoldingRange> ranges) {
 			}
 			// send the calculated updates to the annotations to the
 			// annotation model
-			theProjectionAnnotationModel.modifyAnnotations(deletions.toArray(new Annotation[1]), additions,
-					modifications.toArray(new Annotation[0]));
+			theProjectionAnnotationModel.modifyAnnotations(deletions.toArray(new Annotation[1]), additions, new Annotation[0]);


We still need to pass modifications to theProjectionAnnotationModel.modifyAnnotations, as a subclass can have added some there, should we not?

applyFolding is a private method. Subclasses can't have added something here. Or not?

Ah, I think I got your point here now :-)

rubenporras · 2024-03-04T10:36:18Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/folding/LSPFoldingReconcilingStrategy.java

@@ -238,7 +238,7 @@ private void updateAnnotation(List<Annotation> modifications, List<FoldingAnnota
 		final var newPos = new Position(startOffset, endOffset - startOffset);
 		if (!existing.isEmpty()) {
 			FoldingAnnotation existingAnnotation = existing.remove(existing.size() - 1);
-			updateAnnotations(existingAnnotation, newPos, modifications, deletions);
+			updateAnnotations(existingAnnotation, newPos, deletions);


Suggested change

updateAnnotations(existingAnnotation, newPos, deletions);

updateAnnotations(existingAnnotation, newPos, modifications, deletions);

rubenporras · 2024-03-04T10:37:00Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/folding/LSPFoldingReconcilingStrategy.java

@@ -139,7 +139,7 @@ public void reconcile(IRegion subRegion) {
 	private void applyFolding(List<FoldingRange> ranges) {
 		// these are what are passed off to the annotation model to
 		// actually create and maintain the annotations
-		final var modifications = new ArrayList<Annotation>();
+		final var modifications = new ArrayList<Annotation>(); // not used anymore


Suggested change

final var modifications = new ArrayList<Annotation>(); // not used anymore

final var modifications = new ArrayList<Annotation>(); // not used anymore, can be removed later with the deprecated updateAnnotations method

rubenporras

Thanks a lot!

ghentschke · 2024-03-04T11:00:31Z

@rubenporras thank you for quick review!

ghentschke commented Feb 28, 2024

View reviewed changes

ghentschke requested a review from rubenporras February 28, 2024 11:09

Mark old method signature as deprecated

a22ad82

- Users should use the new method with updated signature.

rubenporras reviewed Mar 4, 2024

View reviewed changes

keep API stable

c900e65

rubenporras reviewed Mar 4, 2024

View reviewed changes

Prevent old API

9a307fe

rubenporras reviewed Mar 4, 2024

View reviewed changes

ghentschke added 2 commits March 4, 2024 11:38

Call old method to keep API stable

084e662

Update comment

4573ef1

rubenporras approved these changes Mar 4, 2024

View reviewed changes

rubenporras mentioned this pull request Mar 4, 2024

[#930] fix broken semantic highlight unit test #940

Merged

ghentschke merged commit 0beab27 into eclipse-lsp4e:master Mar 4, 2024
1 of 2 checks passed

ghentschke deleted the fix-folding branch March 4, 2024 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#934] fix removed folding annotations on document change #935

[#934] fix removed folding annotations on document change #935

ghentschke commented Feb 28, 2024

ghentschke Feb 28, 2024 •

edited

Loading

rubenporras Feb 29, 2024

akurtakov Feb 29, 2024

rubenporras Mar 1, 2024

rubenporras commented Feb 29, 2024

ghentschke commented Feb 29, 2024 •

edited

Loading

rubenporras commented Feb 29, 2024

ghentschke commented Feb 29, 2024 •

edited

Loading

rubenporras Mar 4, 2024

ghentschke Mar 4, 2024

rubenporras Mar 4, 2024

rubenporras Mar 4, 2024

ghentschke Mar 4, 2024

ghentschke Mar 4, 2024

rubenporras Mar 4, 2024

rubenporras Mar 4, 2024

rubenporras left a comment

ghentschke commented Mar 4, 2024

	updateAnnotations(existingAnnotation, newPos, deletions);
	updateAnnotations(existingAnnotation, newPos, modifications, deletions);

	final var modifications = new ArrayList<Annotation>(); // not used anymore
	final var modifications = new ArrayList<Annotation>(); // not used anymore, can be removed later with the deprecated updateAnnotations method

[#934] fix removed folding annotations on document change #935

[#934] fix removed folding annotations on document change #935

Conversation

ghentschke commented Feb 28, 2024

ghentschke Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenporras commented Feb 29, 2024

ghentschke commented Feb 29, 2024 • edited Loading

rubenporras commented Feb 29, 2024

ghentschke commented Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenporras left a comment

Choose a reason for hiding this comment

ghentschke commented Mar 4, 2024

ghentschke Feb 28, 2024 •

edited

Loading

ghentschke commented Feb 29, 2024 •

edited

Loading

ghentschke commented Feb 29, 2024 •

edited

Loading