Bugzilla – Full Text Bug Listing
| Summary: | [quick assist] Provide 'Add parentheses ....' quick assist | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Deepak Azad <deepakazad> | ||||||||||
| Component: | UI | Assignee: | Deepak Azad <deepakazad> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | enhancement | ||||||||||||
| Priority: | P3 | CC: | daniel_megert, markus.kell.r, rthakkar | ||||||||||
| Version: | 3.7 | Flags: | markus.kell.r:
review+
|
||||||||||
| Target Milestone: | 3.7 M6 | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
I like the idea, but I think this shouldn't be confined to cast expressions. It would also be handy in other situations like "foo(a - b)" with "a - b" selected. In the patch, the relevance of the proposal is too high. It should be below "Rename in file" and the "Extract ... " proposals, maybe even at the end of the list. (In reply to comment #1) > I like the idea, but I think this shouldn't be confined to cast expressions. It > would also be handy in other situations like "foo(a - b)" with "a - b" > selected. Yeah, I was thinking the same today. It could probably be a generic - select expression and add parenthesis - quick assist. Though the original idea was to have it available for cast expressions by just placing the caret inside one. > In the patch, the relevance of the proposal is too high. It should be below > "Rename in file" and the "Extract ... " proposals, maybe even at the end of the > list. The relevance is too high even for cast expressions ? Though I don't mind end of the list too much. The patch has some other problems as well - e.g the quick assist is offered on an already parenthesized expression. Created attachment 184932 [details]
fix + tests
The quick assist is now available for
- Cast expression
- Instanceof expression
- Infix expression
- Conditional expression
Do we need it for anything else ?
Another thing to check is if it interferes with 'Add paranoiac parenthesis for conditions' quick assist, or simply looks odd if both these are present.
(In reply to comment #3) > Another thing to check is if it interferes with 'Add paranoiac parenthesis for > conditions' quick assist, or simply looks odd if both these are present. I think these are fine, except maybe the following case 1. Use the following snippet ---------------------------------------------------------------- public void foo2(int a, Object s) { if (10 <= a && s instanceof String) { } } ---------------------------------------------------------------- 2. Select '10 <= a' and invoke quick assist => Both 'Add paranoiac parenthesis for conditions' and 'Put expression in parenthesis' are offered, but do the same thing. In this case the selection is a simple infix expression, hence I was thinking maybe in this case we could avoid offering 'Add paranoiac...' quick assist. Sorry for slipping this review, but I don't want to add this now, unless bug 335173 is fixed. > => Both 'Add paranoiac parenthesis for conditions' and 'Put expression in > parenthesis' are offered, but do the same thing. > > In this case the selection is a simple infix expression, hence I was thinking > maybe in this case we could avoid offering 'Add paranoiac...' quick assist. Yes. Please also think about renaming the "Add paranoiac parenthesis for conditions" quick fix. "Paranoiac" is not well-defined, and I also don't like the term "conditions" (honestly, I don't even remember what it stands for in this context). When the quick assists have similar names and have the same relevance, then hiding one of them in case they are equivalent is no problem any more. (In reply to comment #5) > but I don't want to add this now, unless bug > 335173 is fixed. I think the 2 bugs are a bit unrelated, my idea behind this quick assist was to always add parentheses around the selected expression because of one of the following reasons - I want the code to look that way, e.g. if( a > 1 && b ) => if( (a > 1) && b ) - I want to change the semantics of the code, e.g. a + b * c + d => (a + b) * c + d - Parentheses are needed to proceed further e.g. (String)object => ((String)object)._ The idea was *not* 'Add Parentheses if required'. 'Put 'instanceof' in parentheses' quick fix does something like that. I do not see how fix for bug 335173 will affect this quick assist or vice-versa. > Yes. Please also think about renaming the "Add paranoiac parenthesis for > conditions" quick fix. "Paranoiac" is not well-defined, and I also don't like > the term "conditions" (honestly, I don't even remember what it stands for in > this context). Yep, the name and the quick assist itself is confusing e.g. in the following 2 cases it is available for 1 but not for other System.out.println(b + 0 != a + 1); //not available System.out.println(a > 1 && b > 1); //available (In reply to comment #6) > > unless bug 335173 is fixed. > I think the 2 bugs are a bit unrelated You're right, they are not directly related. But adding this quick assist makes the quick assist popup look bad if the "Add paranoiac ..." quick assist is also shown. > - I want to change the semantics of the code, e.g. > a + b * c + d => (a + b) * c + d The latest patch does not implement that, and I don't think we should offer that as quick assist. This would better be implemented as an editor template "(${selection})", where ${selection} is a new variable that resolves to any non-zero selection. "Source > Surround With" can then also support this variable. (In reply to comment #3) > Created attachment 184932 [details] [diff] > fix + tests For infix expressions, the name "Put expression in parentheses" is confusing. E.g. in the example from comment 4, it's not always clear which expression is meant (and there are multiple that could be applicable). I guess if you mention the concrete operator, this becomes much clearer, e.g. "Put '<' expression in parentheses". OK to go if you add the operator to the name and resolve the conflicts with "Add paranoiac ...". Maybe rename the latter to "Put expressions in parentheses" and replace the last "operator == ..." checks in MissingParenthesisVisitor#needsParentesis(ASTNode) with "return true;"? Created attachment 187803 [details]
fix + tests v2.0
- Added operator to quick assist name, e.g "Put '<' expression in parentheses"
- Added operator to the quick fix name (see LocalCorrectionsSubProcessor)
- Resolved conflicts with "Add paranoic". If both "Add paranoic.." and "Put expression in parenthesis" result in the same change then "Add paranoic.." quick assist is hidden. (see the change in ExpressionsFix.createAddParanoidalParenthesisFix(CompilationUnit, ASTNode[]))
- Renamed "Add paranoic.." to "Put expressions in parentheses"
- Replaced the last "operator == ..." checks in
MissingParenthesisVisitor#needsParentesis(ASTNode) with "return true;". Note that this affects the cleanup as well, and I have renamed the cleanup's description as well. The preference for this cleanup is labeled as "Use parentheses in expressions", it is not "Use parentheses in conditional expressions". Hence I think the new implementation is consistent with this preference.
- Both quick assists appear together, and in the last position in the proposal popup.
'Enclose expressions in parentheses' probably sounds better than 'Put expressions in parentheses'. But we have been using 'put' for sometime and in a number of places.. Plus in FixMessages.properties 'parenthesis' is used incorrectly, it should be 'parentheses'. Almost ready: - AdvancedQuickAssistTest: Please use "parentheses" (not "...is") in the names of new tests. > Plus in FixMessages.properties 'parenthesis' is used incorrectly, it should be > 'parentheses'. Yes, please correct the wrong messages. OK to release with these changes (and with the one below, if you want). > 'Enclose expressions in parentheses' probably sounds better than 'Put > expressions in parentheses'. But we have been using 'put' for sometime and in a > number of places.. I agree that "Put ... in parentheses" is a bit awkward. But "Enclose ... in parentheses" is not that much better either, since it still splits the action in two parts and stuffs the target object in between. How about "Add parentheses around '<' expression"? That would also be in line with the existing "Remove extra parenthesis" (another wrong "...is"). Created attachment 187899 [details] final fix + tests (In reply to comment #10) > > 'Enclose expressions in parentheses' probably sounds better than 'Put > > expressions in parentheses'. But we have been using 'put' for sometime and in a > > number of places.. > I agree that "Put ... in parentheses" is a bit awkward. But "Enclose ... in > parentheses" is not that much better either, since it still splits the action > in two parts and stuffs the target object in between. How about > "Add parentheses around '<' expression"? That would also be in line with the > existing "Remove extra parenthesis" (another wrong "...is"). I have not done this - "Add parentheses..." - looks a bit wordy - "Put '<' expression.... " - in the quick fix/assist popup the ()+ icon and "'<' expression" provide all the info, you do not have to really read the last 2 words - "in parentheses". With "Add parentheses..." you sort of have to read the whole thing. Point is "Put ... in parentheses" feels crispier. Fixed in HEAD. (In reply to comment #8) > - Replaced the last "operator == ..." checks in > MissingParenthesisVisitor#needsParentesis(ASTNode) with "return true;". Note > that this affects the cleanup as well, and I have renamed the cleanup's > description as well. This caused a test failure in CleanUpStressTest as well. It is fixed in HEAD. Verified with I20110307-2110 |
Created attachment 184534 [details] fix v0.9 Currently this is available as a quick fix (i.e. when there is an error and adding parenthesis fixes the error). It would be extremely useful to have this available as a quick assist as well. Attached patch should do the job.