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

Correctly report completed progress for subsequent MethodWrapper calls. #2827

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

rgrunber
Copy link
Contributor

  • When a particular MethodWrapper is queried again for its method calls (eg. incoming/outgoing), it does not report progress
  • Use the IProgressMonitor String representation to determine if the MethodWrapper updated progress, and report it as done if not

How to reproduce

package org.example;

public class Test {
    public static void main(String[] args) {
        doSomething();
        doSomething();
    }

    private static void doSomething() {
    }
}
  1. Invoking call hierarchy on the doSomething method declaration (at the bottom) and it will produce 2 child elements under the main node.
  2. Expand one of the doSomething elements causes the computation to occur and progress is reported
  3. Expanding the other doSomething element simply uses the cached MethodWrapper, which won't report progress.

The result (eg. in VS Code) is that progress-based UI elements can spin forever.

I think this highlights a general class of issues we're going to have between JDT-LS and JDT/Eclipse API. When we pass in a progress monitor to some API, we expect it to always report progress end. Sometimes this doesn't happen.

When I fixed #1722 the main issue was to stop us from incorrectly reporting (eg. 1000% complete). However I introduced an inaccuracy. Specifically : https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/055a935ec3f04c17f5b1ba6a2a8c95380ca5226c/org.eclipse.jdt.core.manipulation/core%20extension/org/eclipse/jdt/internal/corext/callhierarchy/MethodWrapper.java#L86-L100

So if fElements (the returned method calls) are cached from a previous call, then we don't report progress.

Note that expanding/collapsing a given element also shouldn't trigger this because the client-side UI already has the data. There needs to be 2 instances of the same element in an incoming/outgoing call.

@rgrunber

This comment was marked as outdated.

@rgrunber
Copy link
Contributor Author

@hopehadfield found another possible issue, we should address here. One sec.

- When a particular MethodWrapper is queried again for its method calls
  (eg. incoming/outgoing), it does not report progress
- Call monitor.done() to ensure progress completion

Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
@rgrunber
Copy link
Contributor Author

rgrunber commented Aug 31, 2023

If this fails on org.eclipse.jdt.ls.core.internal.managers.GradleUtilsTest.testGetMajorJavaVersion , I'm still going to merge. I think I've seen this failure since #2825 , which is an odd PR for it to occur on.

Update: False alarm. Just some unstable tests we may need to investigate.

@rgrunber rgrunber merged commit 3e36308 into eclipse-jdtls:master Aug 31, 2023
6 checks passed
@rgrunber rgrunber deleted the call-hierarchy-progress branch August 31, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong status in 'language/progressReport' notification when processing call hierarchy requests
2 participants