Bugzilla – Full Text Bug Listing
| Summary: | [extract method] Extract Method refactoring fails if trailing ';' is also part of selection | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Deepak Azad <deepakazad> | ||||||
| Component: | UI | Assignee: | Deepak Azad <deepakazad> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | markus.kell.r, nchen, reprogrammer, snegara2 | ||||||
| Version: | 3.7 | Flags: | markus.kell.r:
review+
|
||||||
| Target Milestone: | 3.7 M4 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
Accepting a selection that includes the ";" in this case is technically wrong, but I agree that it would be handy if the refactoring still worked.
However, we should only implement this if the fix is not too complicated. And it should only consider trailing ";" and maybe ",", but not other characters like "(" or cases like "this.i = /*]*/122 * /*[*/ 42;".
Also take care that the enablement is still correctly computed (e.g. in the context menu).
> press Shift + End
A better way to do this is Edit > Expand Selection To > Next Element.
Created attachment 182024 [details] fix + tests v0.9 With this patch you can select "122;" in the above snippet and perform the refactoring. The refactoring will silently ignore the ";" at the end. (In reply to comment #1) > However, we should only implement this if the fix is not too complicated. And > it should only consider trailing ";" and maybe ",", but not other characters > like "(" or cases like "this.i = /*]*/122 * /*[*/ 42;". Markus currently I have implemented this only for trailing ";", do we want to extend it to other characters as well ? Looks good, please release (with the change below, if you agree). While playing with it, I found that method arguments or array initializer elements are exactly the same case, so I think we should also support ',', e.g. like this in the StatementAnalyzer (this also avoids useless creation of new String objects): char[] token= scanner.getCurrentTokenSource(); //see https://bugs.eclipse.org/324237 if (start < lastNodeEnd && token.length == 1 && (token[0] == ';' || token[0] == ',')) { (In reply to comment #3) > Looks good, please release (with the change below, if you agree). > > While playing with it, I found that method arguments or array initializer > elements are exactly the same case, so I think we should also support ',' Yes, we should support ','. But then we should also support the case when the ',' is in the beginning of the selection, no? The fix for leading ',' case would go in StatementAnalyzer.checkSelectedNodes() and should be simple enough. I can attach another patch by tomorrow morning. > But then we should also support the case when the
> ',' is in the beginning of the selection, no?
I'm not too keen on that. It would make the fix more complicated, and I don't think that a comma in front is as often an issue as a comma or semicolon at the end. Usually, there's a space after a comma, so it's not that hard to get the selection right. The argument about extending the selection with Shift+End also doesn't apply. We're already stretching the rules with the current fix, and I don't think we should add more guesswork.
Created attachment 182679 [details] final fix (In reply to comment #5) > > But then we should also support the case when the > > ',' is in the beginning of the selection, no? > > I'm not too keen on that. It would make the fix more complicated, and I don't > think that a comma in front is as often an issue as a comma or semicolon at the > end. Usually, there's a space after a comma, so it's not that hard to get the > selection right. The argument about extending the selection with Shift+End also > doesn't apply. We're already stretching the rules with the current fix, and I > don't think we should add more guesswork. Fair enough. The patch contains changes from comment 3 and an additional test. Fixed in HEAD. I was going to suggest this enhancement based on the results of the CodingSpectator <http://codingspectator.cs.illinois.edu/> study. But, I noticed that Deepak has already proposed the enhancement and implemented it. I'm glad to see this enhancement already implemented. Since there has been some discussions on what trailing characters to support, I decided to report the results of our study that supports the decision you've already made regarding what trailing characters to exclude automatically. Two of our participants ran into a problem similar to the one described in comment 0. Each of these two participants encountered this problem once. In total, the message "The end of selection contains characters that do not belong to a statement." was reported to these two participants four times because they sometimes tried the refactoring multiple times. In three of the four cases, our participants had selected an extra trailing semicolon. The other case was not due to an extra trailing character. Therefore, I think automatically excluding the trailing semicolon avoids most instances of this error message. (In reply to comment #8) > results of our study that supports the decision you've already made regarding > what trailing characters to exclude automatically. Good to know! |
I20100831-1001 ok, I have faced this too many times now... Steps: - Use this snippet and select "122;" package p; class A { int i; void foo() { this.i = 122; } } - Alt + Shift + M (Extract Method) => The end of selection contains characters that do not belong to a statement. -- bad!! Selecting the trailing ';' is very easy - I generally place the cursor right after '=' and then press Shift + End (to select till end of the line). Extract Method refactoring should simply ignore the trailing ';' instead of complaining about it.