Fix UI flickering in code mining by adding wait/notify reconciliation coordination

[tobiasmelcher]

Problem: Previous implementation returned emptyList() immediately when reconciliation wasn't complete, causing code minings to disappear/reappear during AST reconciliation → UI flickering

Solution:

• Add shared RECONCILE_LOCK and reconciledViewers Set for coordination
• Code mining provider now waits (up to 30s) for reconciliation to complete
• Reconciler signals completion with notifyAll() in reconciled()
• Remove viewer from reconciled set in aboutToBeReconciled() during edits

This pull request will solve following reported issue eclipse-platform/eclipse.platform.ui#2786 (comment)

[jjohnstn]

Works as stated and logic seems reasonable. I noticed in eclipse-platform/eclipse.platform.ui#2786 (comment) that you mention that you aren't sure about waiting for reconcile to finish and @laeubi states that waiting is never a good idea with a CompletableFuture, however, this patch does wait in the CompletableFuture. I didn't see you respond to @laeubi's comment. Can you comment here?

[mickaelistria]

That seems like a good change

[laeubi]

@jjohnstn yes that's right. But on the other hand of course in a legacy code base one might need to do it incrementally. To explain the problem here what will happen is that there is a thread allocated from the common pool just for waiting. This decreases the possible concurrency level and the jvm will allocate possibly more threads... not so good but I would assume that reconciliation is hopefully fast, 30 seconds sounds way to much. Also keep in mind that spurious wakeups are possible to 30sec is just the maximum it will wait... you usually will need a while loop in this case. Also currently it seems to use ONE lock for ALL editors... meaning one (unrelated) editor can block others (e.g. two windows showing different source files).

I just want to outline how it would suppose to work in the best case:

1. There should be some kind of method to acquire the current state (from the code it seems one is interested actually in IJavaElement[]) as a completable future from the editor, this would then work like this:

• synchronize on some lock object
• return the current (maybe already completed) CF to the caller
• if reconcilation is required, synchronize on the lock object, create a new CF (that will be completed one reconciliation is done with the expected result)

2. the codeminig (and probably others) would then fetch the CF and use an thenCompose(Async) (or similar) to transform the result (IJavaElement[]) to the final result (e.g. ICodeMinings[]) and return that.

That way one does not need an additional waiting thread or additional resource allocation and no need for basic synchronize primitives.

[tobiasmelcher]

@laeubi I’ve implemented the CompletableFuture-based changes you recommended with [3ab1bb4]. Could you please review the updated code when you have a chance?

[laeubi]

@tobiasmelcher thanks for your proposal I think it goes in the right direction already, I have added some comments below.

[laeubi]

I would more would use compute here and then check if there is already one (and cancel it) and creating always a fresh one.

[tobiasmelcher]

toBeReconciledViewers.compute(fSourceViewer, (viewer, existingFuture) -> {
			if (existingFuture != null) {
				existingFuture.cancel(false);
			}
			return new CompletableFuture<CompilationUnit>();
		});

I tried with this code; but then it started again flickering. Reason: the canceled future now no longer returns the code minings and therefore the framework is removing the previous code minings provided by the Java implementation.
Maybe, I didn't get what you want to achieve. But I think that calling "cancel" will introduce the flickering again. Please convince me from the opposite if you think that calling "cancel" is a good idea.

[laeubi]

See some comments below

[tobiasmelcher]

Thank you so much for taking the time to review my work and share your feedback; I really appreciate your effort and support.
I believe [8143972] addresses the changes you suggested. To make sure I can learn and improve more effectively, it would be extremely helpful if future feedback could include more concrete examples or pointers to the exact code changes. For instance, highlighting something like:

.handle((result, ex) -> {
    if (ex instanceof CompletionException ce &&
        ce.getCause() instanceof CancellationException) {
        monitor.setCanceled(true);
    }
    return result;
}

in JavaElementCodeMiningProvider, and explaining why monitor.setCanceled(true) needs to be called so that the CodeMiningFramework does not delete previous code minings from the Java provider and flickering occurs.
Maybe you already tried to convey this, but I didn’t fully understand it at first. Your guidance would make the process much easier and help me deliver better results.
Thanks again for your collaboration!

[laeubi]

Thanks for the improvements I think its overall going in a good direction, I added two more comments here.

[laeubi]

It looks almost good to me, just some more comments of some details from my side.

[iloveeclipse]

@tobiasmelcher : could you please follow up on review comments, mark all resolved and fix / discuss not resolved, so that we could target this for merge soon?

[tobiasmelcher]

@tobiasmelcher : could you please follow up on review comments, mark all resolved and fix / discuss not resolved, so that we could target this for merge soon?

I have already done the requested changes with [8246b94]

[BeckerWdf]

@laeubi: Can you pls. re-review?

[laeubi]

It looks fine to me

[jjohnstn]

LGTM

[vogella]

Thanks @tobias-melcher looking forward in using code minings more now.

[tobiasmelcher]

Thanks @tobias-melcher looking forward in using code minings more now.

@vogella please let me know if you find bugs or strange effects with rendering of the code minings

[vogella]

Thanks @tobias-melcher looking forward in using code minings more now.

@vogella please let me know if you find bugs or strange effects with rendering of the code minings

Thanks @tobias-melcher. AFAICS the flickering during save has been fixed. (Scenario: delete some code and methods outside the scope of the change would flicker as they remove and redrawn their code mining).

From a first test, it looks like code minings are not always drawn without resetting the preferences, attached a small exampe, let me know if I should open a new issue for this.

[tobiasmelcher]

Thanks @tobias-melcher looking forward in using code minings more now.

@vogella please let me know if you find bugs or strange effects with rendering of the code minings

Thanks @tobias-melcher. AFAICS the flickering during save has been fixed. (Scenario: delete some code and methods outside the scope of the change would flicker as they remove and redrawn their code mining).

From a first test, it looks like code minings are not always drawn without resetting the preferences, attached a small exampe, let me know if I should open a new issue for this.

Thanks a lot for reporting this issue @vogella . I was able to reproduce the scenario and provide a fix with eclipse-platform/eclipse.platform.ui#3689 . Could you please help with the review?