Community
Participate
Working Groups
Build ID: N20090307-2000 You can ctrl/cmd-click on almost everything to navigate to its definition, but not on break and continue label identifiers. They should lead to the corresponding label. I guess one might argue that the click should lead to the labeled statement, which the break or continue statement is necessarily a part of, so there would be nothing to do, but in a long loop it might be helpful to locate the label itself.
*** This bug has been marked as a duplicate of bug 90276 ***
.
*** Bug 334253 has been marked as a duplicate of this bug. ***
When you click at a break statement, whether labeled or not, the head and tail of the statement being escaped from are highlighted. Likewise when you click at a continue statement, the head of the loop being started over is highlighted. In both cases, the highlighted points of source code is also marked in the navigation bar at the right-hand edge of the source code editor. Therefore I now think that no extra navigation is needed.
Created attachment 189462 [details] Patch The patch makes the label identifier in the break/continue statement clickable and the hyperlink jumps to the corresponding label 'declaration'. Works with OpenAction (Open Declaration/F3) as well.
Markus, could you pls review?
Deepak, please make the initial review.
Created attachment 189719 [details] tweaked patch The patch works fine. Code review - JavaElementHyperlinkDetector.detectHyperlinks(ITextViewer, IRegion, boolean): if (labelNode != null) {... } - this if block is a hack. - JavaElementHyperlinkDetector.getBreakOrContinueLabelNode(..) is called in each of JavaElementHyperlinkDetector , JavaElementHyperlink and OpenAction. If you Ctrl+click a label getBreakOrContinueLabelNode(..) will be called 3 times. I tried to find a solution to these issues in the attached patch. But I admit it is not much better. Raksha, Markus: Is there a good reason to add support for labels in JavaElementHyperlinkDetector, instead of adding a new hyperlink detector ?
(In reply to comment #8) > Created attachment 189719 [details] [diff] > tweaked patch > > The patch works fine. > > Code review > - JavaElementHyperlinkDetector.detectHyperlinks(ITextViewer, IRegion, boolean): > if (labelNode != null) {... } - this if block is a hack. > > - JavaElementHyperlinkDetector.getBreakOrContinueLabelNode(..) is called in > each of JavaElementHyperlinkDetector , JavaElementHyperlink and OpenAction. If > you Ctrl+click a label getBreakOrContinueLabelNode(..) will be called 3 times. > > I tried to find a solution to these issues in the attached patch. But I admit > it is not much better. > > Raksha, Markus: Is there a good reason to add support for labels in > JavaElementHyperlinkDetector, instead of adding a new hyperlink detector ? A new detector would end up on the 'Hyperlinking' preference page and I don't think this is what we want (we already have 6 there for the Java editor).
(In reply to comment #9) > A new detector would end up on the 'Hyperlinking' preference page and I don't > think this is what we want (we already have 6 there for the Java editor). 7 actually :) But if adding a 8th is a problem, then I guess we have to go with the tweaked patch, at least JavaElementHyperlink is hack free in this patch.
Created attachment 189982 [details] Tweaked Patch_2 (In reply to comment #8) > Created attachment 189719 [details] [diff] > tweaked patch > > The patch works fine. > > Code review > - JavaElementHyperlinkDetector.detectHyperlinks(ITextViewer, IRegion, boolean): > if (labelNode != null) {... } - this if block is a hack. Yes, it does seem unnecessary to associate the label with its declared type so as to make the element non-null. > - JavaElementHyperlinkDetector.getBreakOrContinueLabelNode(..) is called in > each of JavaElementHyperlinkDetector , JavaElementHyperlink and OpenAction. If > you Ctrl+click a label getBreakOrContinueLabelNode(..) will be called 3 times. > Hmm, it doesnt get called 3 times but twice - once when detecting and once for opening the link. This needs to be done since we have no other way of finding the label otherwise. But I think the point here is that the code to call getBreakOrContinueLabelNode(..) can be put in one place since the labels are declared in the same editor and hence passing the region of selection is sufficient and there's no need to compute the editor from the fElement either. Made some changes to the tweaked patch and committed to HEAD. Thanks Deepa> > > > > Raksha, Markus: Is there a good reason to add support for labels in > > JavaElementHyperlinkDetector, instead of adding a new hyperlink detector ? > > A new detector would end up on the 'Hyperlinking' preference page and I don't > think this is what we want (we already have 6 there for the Java editor). Also, a new detector for labels seems unnecessary since it will not be a part of multiple hyperlinks(now or in future) as it is specific to only labels.
We can't afford SharedASTProvider.WAIT_ACTIVE_ONLY, since that would freeze the UI in huge files where AST creation can be slow. Fixed in HEAD of JavaElementHyperlinkDetector by using SharedASTProvider.WAIT_NO, like NLSKeyHyperlinkDetector.
I'll revert this for the I-build. Hyperlinks should not show a dialog, and since you already have the AST, proper detection is not that expensive.
In JavaElementHyperlinkDetector, the test if (parent instanceof ContinueStatement || parent instanceof BreakStatement) is better written like this (no functional difference but a safer pattern): StructuralPropertyDescriptor location= simpleName.getLocationInParent(); if (location == ContinueStatement.LABEL_PROPERTY || location == BreakStatement.LABEL_PROPERTY) return simpleName; I think it would also be nice if break and continue statements without labels would be clickable (just jump to the beginning/end of the target statement). This has also been requested in bug 90276 and bug 153513. Test case: public static void main(String[] args) { aaa: for (int i= 0; i < args.length; i++) { String string= args[i]; bbb: for (int j= 0; j < string.length(); j++) { char ch= string.charAt(j); switch (ch) { case 'a': continue aaa; case 'b': break bbb; case 'c': continue; case 'd': break; } if (ch == 0) { System.out.println(ch); break; } } } }
Created attachment 190399 [details] Patch modified for hyperlinking break/continue keywords
Created attachment 190402 [details] Patch modified for hyperlinking break/continue keywords The previous one is not marked as patch. Pls take this one.
(In reply to comment #17) > Created attachment 190402 [details] [diff] > Patch modified for hyperlinking break/continue keywords (In reply to comment #14) > I'll revert this for the I-build. Hyperlinks should not show a dialog, and > since you already have the AST, proper detection is not that expensive. Added the target detection code to the detector so that the hyperlink is shown only if there's a valid target. (In reply to comment #15) > In JavaElementHyperlinkDetector, the test > > if (parent instanceof ContinueStatement || parent instanceof BreakStatement) > > is better written like this (no functional difference but a safer pattern): > > StructuralPropertyDescriptor location= simpleName.getLocationInParent(); > if (location == ContinueStatement.LABEL_PROPERTY > || location == BreakStatement.LABEL_PROPERTY) > return simpleName; Done. > > I think it would also be nice if break and continue statements without labels > would be clickable (just jump to the beginning/end of the target statement). > This has also been requested in bug 90276 and bug 153513. Agree. I missed to see that in the dup request. Added support for that so now ctrl+click on 1) 'break' (with or without label) jumps to the end of the target statement 2) 'continue' with label jumps to the label 3) 'continue' without label jumos to the beginning of the target statement Markus, could you pls review?
* JavaElementHyperlink: - Constructor Javadoc talks about "label". Should be "break or continue target". * JavaElementHyperlinkDetector#findBreakOrContinueTarget(..): - Javadoc: location*s* * OpenAction: - run(ITextSelection): - The new code just returns if something went wrong. It should set the OpenAction_error_messageBadSelection to the status line. - The call to JavaElementHyperlinkDetector.getAST(input) is unnecessary. Replace with "CompilationUnit astRoot= (CompilationUnit) node.getRoot();" and make the getAST(..) method private. BTW: Don't call a variable holding the AST root "ast" (since it's not an instance of AST). Better names are "root", "astRoot", "cu", etc. - jumpToTarget(..): - BreakContinueTargetFinder.getOccurrences() never returns null elements in the array, so the "safety check" is unnecessary and should be removed. - viewer.setSelectedRange(..) doesn't reveal the target. Use fEditor.selectAndReveal(..). When playing with the patch, I found it wrong that clicking a break label doesn't jump to the corresponding label. But it's good that clicking the "break" keyword jumps to the end. I think it's best if you combine JavaElementHyperlinkDetector#getBreakOrContinueNode() and JavaElementHyperlinkDetector#findBreakOrContinueTarget() into one helper that just returns one OccurrenceLocation or null.
Created attachment 190423 [details] Patch_2 modified for hyperlinking break/continue keywords (In reply to comment #19) > - jumpToTarget(..): > When playing with the patch, I found it wrong that clicking a break label > doesn't jump to the corresponding label. But it's good that clicking the > "break" keyword jumps to the end. I think it's best if you combine > JavaElementHyperlinkDetector#getBreakOrContinueNode() and > JavaElementHyperlinkDetector#findBreakOrContinueTarget() into one helper that > just returns one OccurrenceLocation or null. Oh Darn! I was confused between keeping the behaviour uniform for break and for labels.. I also had only one method to find the target but changed it at the end to make it more modular. But I agree combining is a better idea. Made the other changes as well. Markus, could you pls have a look again?
(In reply to comment #20) GO with one change: The patch still jumps to the label when I Ctrl+click "break" in "break bbb". In JavaElementHyperlinkDetector#findBreakOrContinueTarget(..), please add boolean labelSelected= false; , set it to true if the location is in the label, and change the last 'if' to: if (breakOrContinueNode instanceof BreakStatement && !labelSelected)
Created attachment 190439 [details] Patch_3 modified for hyperlinking break/continue keywords (In reply to comment #21) > (In reply to comment #20) > GO with one change: > > The patch still jumps to the label when I Ctrl+click "break" in "break bbb". > In JavaElementHyperlinkDetector#findBreakOrContinueTarget(..), please add > boolean labelSelected= false; > , set it to true if the location is in the label, and change the last 'if' to: > if (breakOrContinueNode instanceof BreakStatement && !labelSelected) Done. Patch_3 committed to HEAD. Thanks Markus.
*** Bug 90276 has been marked as a duplicate of this bug. ***
I've tweaked the code in HEAD a bit as I didn't like that - createHyperlink(...) allows to pass in a 'null' element - createHyperlink(...) is uselessly called on each subclass Raksha, please look at the changes.
Verified in 3.7 M7.
