Bug 351956

Comment 1 Deepak Azad 2012-05-10 14:50:48 EDT

*** Bug 379168 has been marked as a duplicate of this bug. ***

Comment 2 Deepak Azad 2012-05-10 14:52:08 EDT

As a first step we could also convert the quick fix to a multi-fix.

Comment 3 Deepak Azad 2012-10-17 23:49:45 EDT

*** Bug 392040 has been marked as a duplicate of this bug. ***

Comment 4 Deepak Azad 2013-09-15 19:25:02 EDT

Pushed branch - http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/log/?h=dazad/bug-351956-Suggest-to-use-diamond-where-applicable

I added the cleanups to Unnecessary Code tab, but that can be changed easily if required.

Comment 5 Markus Keller 2013-09-26 14:36:02 EDT

Deepak, please
- move the group to the Code Style tab
- align the option label with the existing style (no title case; say "1.7 or higher")
- add a preview that shows all three option states. An additional type argument that never goes away (e.g. type of a variable) wouldn't hurt.

Manju, can you please review Deepak's code changes?


Everybody: Think about a better wording. The goal of the cleanup is to use the diamond <>. We can use the word "diamond", since it even shows up in the spec. But I think we should also write the <> chars, since this makes it immediately clear what the clean up is about.

"Use Type Arguments (Java 7 or higher)" sounds like type arguments are something new in Java 7. And turning the checkbox off could also be understood as "Remove all type arguments".

How about this:

[x] Use diamond <> (1.7 or higher)
   o Never       o If type arguments are redundant

Or this (2):

[x] Reduce type arguments to diamond <> (1.7 or higher)
   o Never       o Only if redundant

Or this (3):

[x] Use diamond <> instead of type arguments (1.7 or higher)
   o Never       o Only if redundant

Comment 6 Martin Mathew 2013-10-07 10:31:23 EDT

I would vote for 
[x] Reduce type arguments to diamond <> (1.7 or higher)
   o Never       o Only if redundant

Below are my review comments:
1. Assign keyboard short keys to the new preference option.
2. Update Copyright year
		MultiFixMessages.java
		MultiFixMessages.properties
		CleanUpMessages.java
		CleanUpMessages.properties
3. The changes in CleanUpTestCase does not look convincing.
4. TypeParametersFix#createInsertInferredTypeArgumentsFix
                    if (node == null)
			return null; 
    This should be the first line
5. CleanUpConstants#INSERT_INFERRED_TYPE_ARGUMENTS. The Javadoc looks a bit confusing. Shouldn't the Javadoc be "Insert Inferred Type Argument."

Markus, kindly do a quick review for TypeParametersFix and TypeParametersCleanUp. Most of the code in TypeParametersFix were refactored from existing code so that the new clean up option and quick fix go together.

Comment 7 Dani Megert 2014-03-28 05:37:28 EDT

Deepak, will you have time to finish this for M7?

Comment 8 Dani Megert 2014-04-22 05:14:45 EDT

Ping! M7 is next week.

Comment 9 Dani Megert 2014-04-28 14:14:50 EDT

Manju, please bring this to a good end.

Comment 10 Markus Keller 2014-05-02 13:42:03 EDT

*** Bug 433974 has been marked as a duplicate of this bug. ***

Comment 11 Markus Keller 2014-05-02 14:42:07 EDT

Rebased Deepak's branch on master: mkeller/bug351956

I don't think we really need the "Insert inferred type arguments" option in the cleanup. Once this is a multi-fix, it can also be applied via the Problems view if really necessary. This doesn't work with the proposed fix in a 1.6 project:

package snippet;
import java.util.ArrayList;
public class Snippet {
	ArrayList<String> al0 = new ArrayList<>();
	ArrayList<String> al1 = new ArrayList<>();
}
 
Then the remaining option is really a *clean*-up and can be on the "Unnecessary Code" tab:

[] Remove unnecessary type arguments

Comment 12 Markus Keller 2014-05-07 11:42:31 EDT

This becomes more and more important as more users move to Java 7 or 8.

Let's try to get the multi-fix part into 4.4 RC1. Leave the cleanup part that needs new UI out for now.

We have to make sure the code moved to TypeParametersFix actually matches the original code. The patch has been written quite a while ago, and I don't think git-rebase applies newer changes to the moved code.

Comment 13 Martin Mathew 2014-05-09 03:07:28 EDT

Created attachment 242881 [details]
Patch

The uploaded patch is essentially Deepak's patch taken from mkeller/bug351956. From that i have removed the UI code for the Clean Up option and also adapted "Insert inferred type arguments" as a multi-fix. 
Markus, kindly have a look.

Comment 14 Markus Keller 2014-05-13 13:32:57 EDT

Looks good, thanks everybody.

Fixed @since tags in CleanUpConstants.

The ImportRewrite in InsertTypeArgumentsOperation didn't fully work, because rewriteImports(..) was never called. This was already broken in the old implementation in QuickAssistProcessor#getInferDiamondArgumentsProposal(..).

	java.util.List<java.util.Date> foo() {
		return new java.util.ArrayList<>(); // insert inferred TArgs here
	}

Fixed by using cuRewrite.getImportRewrite().

The only other code difference I found was the loss of addLinkedPosition(..) in getInferDiamondArgumentsProposal(..). But that was not too interesting anyway.


Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=66203dfe5f2f4547ea186d8485dbe5db4308944a
and opened bug 434788 for the Clean Up.

In Luna (4.4), the multi-fix in the whole file works via Quick Fix hover or via Ctrl+1 popup and then using Ctrl+Enter. To apply the fix to a whole project, you can:
- select the problem in the Problems view
- select the whole project so that the Problems view shows problems from all selected resources
- activate the Problems view again and use Ctrl+1 or context menu > Quick Fix
- select the right fix, click Select All, and click Finish

Comment 15 Noopur Gupta 2014-05-19 06:25:08 EDT

Verified as working in build id: I20140515-1230.