-
Notifications
You must be signed in to change notification settings - Fork 393
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
'Open Call Hierarchy' does not jump to reference where it is invoked at #1824
Conversation
test this please |
List<Range> ranges = toCallRanges(call.getMethodCall().getCallLocations()); | ||
result.add(new CallHierarchyIncomingCall(symbol, ranges)); | ||
Collection<CallLocation> callLocations = call.getMethodCall().getCallLocations(); | ||
List<Range> ranges = toCallRanges(callLocations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move down the if block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not fixed. ranges should only be computed if callLocations != null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. still not fixed. List<Range> ranges = toCallRanges(callLocations);
needs to go inside the if block.
Collection<CallLocation> callLocations = call.getMethodCall().getCallLocations();
if (callLocations != null) {
List<Range> ranges = toCallRanges(callLocations);
for (CallLocation location : callLocations) {
IOpenable openable = getOpenable(location);
Range callRange = getRange(openable, location);
CallHierarchyItem symbol = toCallHierarchyItem(call.getMember());
symbol.setSelectionRange(callRange);
result.add(new CallHierarchyIncomingCall(symbol, ranges));
}
}
@@ -181,6 +191,9 @@ private IMember getCallHierarchyElement(String uri, int line, int character, IPr | |||
return null; | |||
} | |||
|
|||
if (!wrapper.canHaveChildren()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be combined with previous if block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Collection<CallLocation> callLocations = call.getMethodCall().getCallLocations(); | ||
List<Range> ranges = toCallRanges(callLocations); | ||
if (callLocations != null) { | ||
for (CallLocation location : callLocations) { | ||
IOpenable openable = location.getMember().getCompilationUnit(); | ||
if (openable == null) { | ||
openable = location.getMember().getTypeRoot(); | ||
} | ||
int[] start = JsonRpcHelpers.toLine(openable, location.getStart()); | ||
int[] end = JsonRpcHelpers.toLine(openable, location.getEnd()); | ||
Assert.isNotNull(start, "start"); | ||
Assert.isNotNull(end, "end"); | ||
// Assert.isLegal(start[0] == end[0], "Expected equal start and end lines. Start was: " + Arrays.toString(start) + " End was:" + Arrays.toString(end)); | ||
CallHierarchyItem symbol = toCallHierarchyItem(call.getMember()); | ||
result.add(new CallHierarchyOutgoingCall(symbol, ranges)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid duplicate code, please refactor L208-L226 to a method call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@fbricon I have updated the PR. |
@fbricon I have updated the PR. |
One small issue I noticed. This seems to work fine for "Call Hierarchy", because it just relies on the selection range when jumping to an item. However, "Peek -> Call Hierarchy" relies on the first fromRange value and those are duplicated across calls that occur in the same method. Are we ok with this ? It seems like it should be possible to adjust this but might make the code a bit more complicated. |
@@ -221,13 +249,20 @@ private IMember getCallHierarchyElement(String uri, int line, int character, IPr | |||
|
|||
List<CallHierarchyOutgoingCall> result = new ArrayList<>(); | |||
for (MethodWrapper call : calls) { | |||
Collection<CallLocation> callLocations = call.getMethodCall().getCallLocations(); | |||
List<Range> ranges = toCallRanges(callLocations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, List<Range> ranges = toCallRanges(callLocations);
needs to go inside the if block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you, please, check it again?
Yeah that sucks. If it's easy to fix in this PR, let's do this, else, we'll fix it in a subsequent change. |
6649d53
to
38a131c
Compare
Iterator<CallLocation> iter = callLocations.iterator(); | ||
while (iter.hasNext()) { | ||
iter.next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is that helping since you're not using the current object from the iterator? Might has well check if callLocations is empty in the if block L253.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this. Feel free to merge when ready, and file a bug for #1824 (comment) to track it once this is merged..
Yes. This should be included in the next eclipse.jdt.ls (1.4.0) release. You could also try it out with http://download.eclipse.org/jdtls/snapshots/jdt-language-server-latest.tar.gz , once it gets built there. |
Thank you @rgrunber, that's exciting to hear. |
Fixes redhat-developer/vscode-java#2044
Signed-off-by: Snjezana Peco snjezana.peco@redhat.com