Skip to content

CodeMining: reuse existing inlined annotations when minings are unchanged #3580

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.

Sign up for GitHub

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

Open
tobiasmelcher wants to merge 1 commit into
base: master
Choose a base branch
from
+36 −3
Open

CodeMining: reuse existing inlined annotations when minings are unchanged #3580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bundles/org.eclipse.jface.text/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: %pluginName
Bundle-SymbolicName: org.eclipse.jface.text
Bundle-Version: 3.29.100.qualifier
Bundle-Version: 3.30.0.qualifier
Bundle-Vendor: %providerName
Bundle-Localization: plugin
Export-Package:
Expand Down
4 changes: 4 additions & 0 deletions ....text/src/org/eclipse/jface/internal/text/codemining/CodeMiningLineContentAnnotation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,4 +216,8 @@ public boolean isInVisibleLines() {
public final boolean isAfterPosition() {
return afterPosition;
}

public List<ICodeMining> getMinings() {
return fMinings;
}
Comment on lines +220 to +222
Copy link
Contributor

@laeubi laeubi Dec 15, 2025

Choose a reason for hiding this comment

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

As the discussion shows I don't think anyone needs access to this data structure or should access it! This is even an internal class so no one should ever call any method on this outside the framework itself.

}
4 changes: 4 additions & 0 deletions ...e.text/src/org/eclipse/jface/internal/text/codemining/CodeMiningLineHeaderAnnotation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,4 +290,8 @@ public Consumer<MouseEvent> getAction(MouseEvent e) {
public boolean isInVisibleLines() {
return super.isInVisibleLines();
}

public List<ICodeMining> getMinings() {
return fMinings;
}
}
2 changes: 1 addition & 1 deletion ....eclipse.jface.text/src/org/eclipse/jface/internal/text/codemining/CodeMiningManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ private void renderCodeMinings(Map<Position, List<ICodeMining>> groups, ISourceV
CodeMiningMode mode= CodeMiningMode.createFor(minings);

// Try to find existing annotation
AbstractInlinedAnnotation ann= fInlinedAnnotationSupport.findExistingAnnotation(pos);
AbstractInlinedAnnotation ann= fInlinedAnnotationSupport.findExistingAnnotation(pos, minings);
if (ann == null || !mode.annotationType.isInstance(ann)) {
// The annotation doesn't exists or has wrong type => create a new one.
boolean afterPosition= false;
Expand Down
27 changes: 26 additions & 1 deletion ...clipse.jface.text/src/org/eclipse/jface/text/source/inlined/InlinedAnnotationSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.eclipse.core.runtime.Assert;

import org.eclipse.jface.internal.text.codemining.CodeMiningLineContentAnnotation;
import org.eclipse.jface.internal.text.codemining.CodeMiningLineHeaderAnnotation;

import org.eclipse.jface.text.BadLocationException;
import org.eclipse.jface.text.DocumentEvent;
Expand All @@ -58,6 +59,7 @@
import org.eclipse.jface.text.Position;
import org.eclipse.jface.text.Region;
import org.eclipse.jface.text.TextPresentation;
import org.eclipse.jface.text.codemining.ICodeMining;
import org.eclipse.jface.text.source.Annotation;
import org.eclipse.jface.text.source.AnnotationPainter;
import org.eclipse.jface.text.source.IAnnotationModel;
Expand Down Expand Up @@ -483,8 +485,15 @@ public void updateAnnotations(Set<AbstractInlinedAnnotation> annotations) {
* @return the existing codemining annotation with the given position information and null
* otherwise.
*/
@SuppressWarnings("unchecked")
public <T extends AbstractInlinedAnnotation> T findExistingAnnotation(Position pos) {
return findExistingAnnotation(pos, null);
}

/**
* @since 3.30
*/
@SuppressWarnings("unchecked")
public <T extends AbstractInlinedAnnotation> T findExistingAnnotation(Position pos, List<ICodeMining> minings) {
Copy link
Contributor

@mickaelistria mickaelistria Dec 14, 2025

Choose a reason for hiding this comment

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

Would it be possible and/or better to implement the caching directly in this manager instead of expecting all providers to implement it?
Basically, I imagine the manager could store a Map<ICodeMiningProvider, Map<Position, ICodeMining>> lastResults, and that findExistingAnnotation could take directly the provider as input, and become findExistingAnnotation(pos, provider)

Copy link
Contributor Author

@tobiasmelcher tobiasmelcher Dec 15, 2025

Choose a reason for hiding this comment

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

Thanks a lot for the very fast response.
I had a similar idea in mind. We need an indication from the ICodeMiningProvider implementation when we should keep the existining annotations and when to replace. The current API does not really provides this ability.
Let's take a look at the current JDT implementation:

@Override
	public CompletableFuture<List<? extends ICodeMining>> provideCodeMinings(ITextViewer viewer,
			IProgressMonitor monitor) {
		if (!editorEnabled) {
			return CompletableFuture.completedFuture(Collections.emptyList());
		}
		if (viewer instanceof ISourceViewerExtension5) {
			ISourceViewerExtension5 codeMiningViewer = (ISourceViewerExtension5)viewer;
			if (!JavaCodeMiningReconciler.isReconciled(codeMiningViewer)) {
				// the provider isn't able to return code minings for non-reconciled viewers
				return CompletableFuture.completedFuture(Collections.emptyList()); // <!--------------
			}
		}
		return CompletableFuture.supplyAsync(() -> {
			monitor.isCanceled();
			ITextEditor textEditor= super.getAdapter(ITextEditor.class);
	                ... // here the real calculation logic takes place

The Java implementation returns a CompletableFuture.completedFuture(Collections.emptyList()); when the reconciler is not yet finished and calculation of the code minings cannot be done. The current implementation replaces all existing annotations with the ones returned from the code mining provider. This means, the existing ones are deleted and the flickering appears.
We need to distinguish between "replace existing annotations with the ones I provide you" and "keep the existing annotations because I cannot calculate right now".
We would need to extend the API which could get quite complex.

I locally already started with a second Extension interface which allows to provide this information. But compatibility handling then got so complicated that I gave up. This pull request together with eclipse-jdt/eclipse.jdt.ui#2697 is much less complex and easier to follow. For sure, we move more complexity to the provider implementation.
Here the interface which I tried to introduce and implement:

public interface ICodeMiningProviderExtension {

    /**
     * Sealed result type for code mining calculations.
     * 
     * <p>Exactly one of {@link CodeMiningSuccess} or {@link CodeMiningDeferred} will be returned.
     * Use pattern matching (switch expressions) for exhaustive handling.</p>
     */
    public static sealed interface CodeMiningResult 
            permits CodeMiningSuccess, CodeMiningDeferred {}

    /**
     * <strong>SUCCESS</strong>: Code minings calculated successfully.
     * 
     * <p>The returned {@code minings} <strong>REPLACE</strong> any existing code minings
     * from previous invocations of this provider.</p>
     * 
     * <ul>
     * <li><strong>Empty list:</strong> Clears all code minings from this provider</li>
     * <li><strong>Null:</strong> Also clears all code minings (treated as empty)</li>
     * <li><strong>Non-empty:</strong> Displays the new code minings</li>
     * </ul>
     * 
     * @param minings New code minings to display (replaces existing ones)
     */
    public static final record CodeMiningSuccess(List<? extends ICodeMining> minings) 
            implements CodeMiningResult {}

    /**
     * <strong>DEFERRED</strong>: Code minings cannot be calculated right now.
     * 
     * <p><strong>CRITICAL:</strong> Existing code minings from previous successful calls
     * to this provider are <strong>PRESERVED</strong> and <strong>NOT REMOVED</strong>.</p>
     * 
     * <p>Typical reasons include:</p>
     * <ul>
     * <li>Document not fully reconciled</li>
     * <li>Operation timeout (e.g., &gt;30s)</li>
     * <li>Progress monitor cancelled</li>
     * </ul>
     * 
     * <p>Consumers should <strong>NOT</strong> clear the UI or update displays when receiving
     * this result. The system will retry automatically on the next reconciliation cycle.</p>
     */
    public static final class CodeMiningDeferred implements CodeMiningResult {}

    /**
     * Asynchronously computes code minings for the given viewer.
     * 
     * @param viewer The text viewer requesting code minings
     * @param monitor Progress monitor for cancellation checks
     * @return Future completing with either:
     * <ul>
     * <li>{@link CodeMiningSuccess} - New minings ready (replaces existing)</li>
     * <li>{@link CodeMiningDeferred} - Calculation deferred (keeps existing minings)</li>
     * </ul>
     * @throws CancellationException if the future is cancelled
     */
    CompletableFuture<CodeMiningResult> provideCodeMinings(ITextViewer viewer, 
            IProgressMonitor monitor);
}

A simpler alternative approach could be the introduction of a subclass of ICodeMining with name like KeepExistingCodeMinings which gives the indication to keep the existing ones.

What is your opinion here? In which direction should I go?

Copy link
Contributor

@laeubi laeubi Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The Java implementation returns a CompletableFuture.completedFuture(Collections.emptyList()); when the reconciler is not yet finished and calculation of the code minings cannot be done. The current implementation replaces all existing annotations with the ones returned from the code mining provider. This means, the existing ones are deleted and the flickering appears.
...
What is your opinion here? In which direction should I go?

As I mentioned in the other ticket already, this approach is fundamentally broken and completely contradicts the usage of CompletableFuture, so I don't think it is anywhere useful to further support that case.

The use case of CompletableFuture actually is to schedule something (maybe long running) in the background and then allow consumers to wait for the result. So returning something as "I'm not ready" is simply not needed, just return an (uncompleted) CompletableFuture and complete it whenever you are ready to do so!

This is then all what's needed to let the caller know "keep whats currently used because I have nothing new (yet) for you"... Also otherwise how should the "manager" no when it is time to ask for something new? That's all what completable future is for!

Copy link
Contributor Author

@tobiasmelcher tobiasmelcher Dec 15, 2025

Choose a reason for hiding this comment

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

@laeubi could you please take a look at eclipse-jdt/eclipse.jdt.ui@master...tobiasmelcher:eclipse.jdt.ui:wait_for_reconciler ? Are you proposing this approach?

I added a Thread.sleep(3000) by intention to show a sceanrio where reconciliation takes a longer time.

Here a screencast showing the JDT and Copilot code minings providing some results.
The current framework implementation waits until all CompletableFutures are available and is then triggering a redraw. This breaks the Copilot functionality from my point of view because the copilot proposals are shown too late.

java_reconciler_3_seconds_.mp4

What is your opiniion? What would you propose? Should we trigger a redraw for each CompletableFuture separately? I think that we then get complaints that we trigger redraw too much often.

Copy link
Contributor

@laeubi laeubi Dec 15, 2025

Choose a reason for hiding this comment

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

Are you proposing this approach?

This is not optimal and has some room for improvements but yes this is how it basically is supposed to work and correct.

I added a Thread.sleep(3000) by intention to show a scenario where reconciliation takes a longer time.

If reconcilation takes really that long we would get much more complains, but yes its valid for investigation of course. One still might question if this really happens at all?

The current framework implementation waits until all CompletableFutures are available and is then triggering a redraw.

This is a very simple approach what might can be optimized in several ways I think, e.g. one might wait only a certain time (e.g. 100ms) for providers and then already perform an update with whats there. Then one slow provider will not block all others, a total timeout might be useful as well (e.g. if a provider takes > 10 seconds cancel it), but this also depends on how the providers work, for example if the document constantly changing (during typing) then maybe even the document positions become stale anyways.

This breaks the Copilot functionality from my point of view because the copilot proposals are shown too late.

It all depends a bit on how we define "break". I would assume copilote is showing things based on the document model. If that currently is reconciled it would mean it is not up-to date, so even if we can show the things faster they might be wrong / outdated. So do we want to show something fast, or something more correct?

What is your opinion? What would you propose?

For the matter of this topic, see above. In general it might be useful to reconsider what copilot does here and possibly offering some specialized support for "ghosttext" that do not interfere with code mining. For example we have similar for auto-completion already, where typing more reduces the list of recommendations. e.g. in this particular case the editor can do much of the work already without the need of the provider, so in your example

  • // -> provider returns nothing
  • // co -> provider returns "comment"
  • // com -> we can reuse last return value "comment" and update the editor right now, then ask the provider if it now wants to supply a different thing (what might be slower than the user types) and update it once it is available.

Such an approach, specially crafted to "inline code completion" would not need to retrigger all codemining providers.

Copy link
Contributor Author

@tobiasmelcher tobiasmelcher Dec 15, 2025

Choose a reason for hiding this comment

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

This is a very simple approach what might can be optimized in several ways I think, e.g. one might wait only a certain time (e.g. 100ms) for providers and then already perform an update with whats there. Then one slow provider will not block all others, a total timeout might be useful as well (e.g. if a provider takes > 10 seconds cancel it), but this also depends on how the providers work, for example if the document constantly changing (during typing) then maybe even the document positions become stale anyways.

Very good proposal, I like it. I can take a look but this takes some time for me. You can also take over here if you want.

Copy link
Contributor Author

@tobiasmelcher tobiasmelcher Dec 15, 2025

Choose a reason for hiding this comment

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

For the matter of this topic, see above. In general it might be useful to reconsider what copilot does here and possibly offering some specialized support for "ghosttext" that do not interfere with code mining. For example we have similar for auto-completion already, where typing more reduces the list of recommendations. e.g. in this particular case the editor can do much of the work already without the need of the provider, so in your example

Why introducing a new API only for the Copilot plugin? I am not quite sure. You would then need to take over here. I will not do it.

Copy link
Contributor

@laeubi laeubi Dec 15, 2025

Choose a reason for hiding this comment

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

Do you want to continue here and provide the "optimal" solution? I can help if you provide more feedback.

Not quite yet, I just wanted to get some feedback.

Very good proposal, I like it. I can take a look but this takes some time for me. You can also take over here if you want.

Sure no one want to force you hear, I just wanted to mention it here because the problem itself has different angles and places where things can be improved independently.

Why introducing a new API only for the Copilot plugin? I am not quite sure.

Before you where fine to introduce new API as well... of course it is not for copilot plugin it would be for who ever wants to provide "ghos text" what would not be needing an AI at all (e.g. local variables, fields, whatever...)

You would then need to take over here. I will not do it.

No obligations here, I just wanted to mention it that the actual usage of codemining + copilot approach refreshing everything on every keypress might better be solved with a dedicated API.

So in general I would say: Fix the immediate JDT issue as described, if its getting slow for copilot its a different topic and should not be your primary concern here.

Copy link
Contributor Author

@tobiasmelcher tobiasmelcher Dec 15, 2025

Choose a reason for hiding this comment

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

@laeubi and @mickaelistria I created a pull request for the JDT project eclipse-jdt/eclipse.jdt.ui#2698 which should solve the flickering issue. Could you please be so kind and review the code? Do you have contacts in the JDT team who could support me with my request?

Copy link
Contributor

@mickaelistria mickaelistria Dec 16, 2025

Choose a reason for hiding this comment

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

I think the outcome of this discussion is great and the PR is good too.

if (fInlinedAnnotations == null) {
return null;
}
Expand All @@ -496,6 +505,22 @@ public <T extends AbstractInlinedAnnotation> T findExistingAnnotation(Position p
// Do nothing
}
}
if (minings == null) {
continue;
}
List<ICodeMining> existingMinings= null;
if (ann instanceof CodeMiningLineHeaderAnnotation lineHeader) {
existingMinings= lineHeader.getMinings();
} else if (ann instanceof CodeMiningLineContentAnnotation lineContent) {
existingMinings= lineContent.getMinings();
}
if (existingMinings != null && existingMinings.equals(minings) && ann.getPosition() != null && !ann.getPosition().isDeleted()) {
try {
return (T) ann;
} catch (ClassCastException e) {
// Do nothing
}
}
}
return null;
}
Expand Down
Loading