Community
Participate
Working Groups
3.0m9 it's pretty confusing sometimes what a 'continue' or 'break' refers to. it would be cool to have the control statement highlighted when i select the 'continue' or 'break'. it'd also take labels into account.
Not for 3.0.
sure. just came to my head and i entered before i'd forget.
Sounds like a good idea!
Daniel, are you taking care of this ?
see also bug 79238.
any chance for this? :) It'd come really handy sometimes.
Hey Adam, what about sending in a patch ;-)
can look at it but not before september 9th (ICSE deadline) :)
labels will be a problem. see bug 33739
Created attachment 25654 [details] patch here's the patch. Currently, all *Finders select whole AST nodes so this one does too. It looks a little strange sometimes. I did not add a preference for it. I can, if you decide to incorporate this. (we might want to recode *Finders to return just source positions instead of ASTNodes to allow finer grain marking - in this case you really just want to select the 'for/do/while/switch' keyword or the label, rather than the whole statement)
patch is against jdt.ui 3.1
Dani, do you have test cases for these things? If not, I can write an initial suite for this guy and the others.
yep, see org.eclipse.jdt.text.tests/.../MarkOccurrenceTest
ok. but not to mess up your world and to make it more modular I'll create a separate test class just for the BreakContinueTargetFinder, ok? It'd be independent of the editor - just eating testing that for a given ast and selection you get the right thing selected.
Not messing up is always fine ;-)
Created attachment 25947 [details] patch against jdt ui (3.1) patch corrected to handle EnhancedForStatement
Created attachment 25948 [details] tests - patch against jdt.text.tests here're the tests - they just check that when break/continue is marked in the editor than the right thing gets selected. The other finders in jdt ui have some tests too but they're tied to the editor. The finding function is actually independent and this suite check only the functionality.
Reviewed the patch: It currently highlights the full code which is really ugly (see attached picture). It should only highlight the top-level 'switch', 'for' or 'while' statement. Like with all other occurrence markings it must be configurable and hence the following things added: - preference constant for this occurrence marking - preference UI (see MarkOccurrencesConfigurationBlock) - add code to the Java editor to react to the preference change
Created attachment 26294 [details] Shows the current result
re comment 18: I know. See comment 10. In order to change that, all other Finders would have to be uniformly modified - currently everyone return an ASTNode but there's none to return if you want to highlight the 'for' or 'while' or 'switch' keyword. Please advise here - what you you want to do? Change all Finders to return array of source ranges (sounds best to me) or make this one Finder special and treat him specially in the UI? BTW I know about all the other additional little things :). Just wanted the funtionality approved before I spend time on productization.
There's a trick how select the non-existing nodes. Here's an example: SimpleName name= fAST.newSimpleName("xxxxx"); //$NON-NLS-1$ name.setSourceRange(node.getStartPosition(), 5); fResult.add(name); After thinking about it again I think it is even better to select the next statement that will be executed i.e. for 'continue' select the statement's expression and for 'break' select the closing brace of the statement.
ok, i can to 'the trick' (pretty nasty but ok with me) ok, how about we select the enclosing construct's keyword (like 'for'/'while') for continue and the closing brace for break "The statement's expression" varies depending what it is: - for for-loops : it'd be the incement statement (if one exists - otherwise the first statement in the block (block *must* be there because we selected 'continue' and that must live somewhere) - for foreach: the first statement in the block - for 'while' - the entry condition - for 'do' - the exit condition So I think it may be confusing. But maybe not. What do you think?
Yes, selecting the keyword is fine with me. The other solution would be only needed if we don't want to do the hack. But since we already use that hack, e.g. for in the MethodExitsFinder we can go for the keyword.
ok, i'm busy with some other stuff but this is very simple so let's plan for m2
Personally, I'd prefer for break to also highlight the keyword, not the closing brace. I think that's the more important information to me as the developer. (And in fact, there doesn't need to be a closing brace at all.)
(In reply to comment #25) > Personally, I'd prefer for break to also highlight the keyword, not the closing > brace. So maybe both the keyword and the closing brace should be highlighted then?
I'd like to see where the code continues after the 'break'. If there's no '}' we could select the next statement or the whitespace in front of it.
(In reply to comment #26) > (In reply to comment #25) > > Personally, I'd prefer for break to also highlight the keyword, not the closing > > brace. > So maybe both the keyword and the closing brace should be highlighted then? Or make it configurable?
>Or make it configurable? No additional option.
Created attachment 26440 [details] patch - next cut next cut of patch against jdt ui (3.1)
Created attachment 26441 [details] tests - patch against jdt.text.tests tests - next cut
agreed - no new options. I disagree with selecting "the next thing that would happen" after break. It'd quickly get very confusing. So here's what I did: 1. what gets selected is the first token in the target statement (for, do, while, switch or label) 2. selection works now also inside the label names (i.e. you can select 'foo' in 'break foo' and it should work too) 3. changed tests to reflect this Dani, please give it a whirl and let me know what you think about this behavior. It looks pretty good to me. Once we agree on the functionality I'll do all those productization tweaks. For reference: here's the test case I use when playing around with settings: class A { void foo(int[] i) { for (int j = 0; j < i.length; j++) { break; } label: while (i != null) { if (true) break; do { if (false) continue; if (false) continue label; } while(i.length>9); continue; } } }
oh, one more thing: in order to get the correct length of the thing to select, I need to scan. And in order to scan I need the file contents. Currently, I use document.get() inside the JavaEditor but have no idea if this call is cheap or expensive. I did not look inside the call - if you tell me it's expensive I'll try to find something else.
Looks nice. Some comments: - don't pass the document via initialize but get source from the AST using root.getJavaElement(), check this for ISourceReference and use getSource() - don't set the full source to the scanner and reset it but only give it directly the correct code snippet (also reduce the end offset). To be even smarter you could reduce the length of the source via first child of the statement - set fIsBreak already in the initialize method to reduce the number of instance checks Please also mark the ending '}' if there's any (don't mark the next statement or whitespace). In the slightly modified example from comment 32 select the first break. As you can see it will be really handy to see the '}', which is almost at the bottom, highlighted. public class BreakOccurrenceMarking { void foo(int[] i) { for (int j = 0; j < i.length; j++) { if (false) break; label: while (i != null) { if (true) break; do { if (false) continue; if (false) continue label ; if (false) break; } while(i.length>9); continue; } } } }
Dani, I just got back and am swapmed with stuff. May not make it for M2
FYI: The test candidate build is already on Monday.
Moving to next milestone.
Created attachment 27865 [details] jdt ui patch Dani, here's the almost final cut. I did all (?) the productization things and addressed your comments. What's still missing is the highlighting of the closing brace.
Created attachment 27866 [details] and the tests
aha, right. The ui too is missing :)
I assume an updated patch with UI and highlighting of the closing brace will appear soon ;-)
Created attachment 27948 [details] full version here it is. check it out and let me know if I missed something.
Created attachment 27949 [details] the tests
Released the patches with minor changes (typos, some code reorg). Thanks Adam!
Verified using I20051031-2000
