Preserving star imports in Organize Imports

[m-froehlich]

When Organize Imports is triggered, existing star imports are preserved. None are added, if they don't exist before.

What it does

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[iloveeclipse]

Thanks for patch.

1. Please make sure your github account mail is the one you registered in ECA.
2. Please close first PR Preserving star imports in Organize Imports #341
3. Please don't create new PR's but update existing by force pushing on same branch.

[m-froehlich]

I have deleted the first pull request together with it's branch. Sorry, I'm not familiar with all this here. And there were no buttons to commit to the first branch to correct the indentation (produced by this UI).

Is the pull request valid even without my ECA agreement? I would love to see the patch in the next release. And I'm not sure, if the ECA agreemant will work in time. I have created a new account with the same email address as I used here. But I never got the confirmation mail. So I cannot login and agree.

[m-froehlich]

Ok, my second attempt to sign the ECA was successful. It's done now.

Please feel free to merge the patch into master.

[m-froehlich]

Any idea, if and when this patch is merged with master?

[jjohnstn]

First of all, you need to add a test case of some sort to show what this is fixing (in the comments is ok). Secondly, there are preferences for Organize Imports (Java -> Code Style -> Organize Imports) that govern when star imports are added/removed. Does your change account for this?

e.g.

import java.util.ArrayList;
import java.util.Currency;

public class Test3 {
	
	public void foo() {
		Currency c = Currency.getInstance("abc"); //$NON-NLS-1$
		ArrayList<String> b = new ArrayList();
//		java.util.Calendar d= Calendar.getInstance();
	}

}

With "number of imports needed for *" set to 3, un-commenting the Calendar line above will cause Organize Imports to change the imports to import java.util.*; If you subsequently comment out the line again, Organize Imports will result in the original two imports above as expected.

[m-froehlich]

Thanks for the reply. The "preserve star imports" approach is a little different than you might think. It doesn't add a star import, that wasn't there before. Hence it has nothing to do with the "number of imports needed for *" setting.

`import java.util.*;

public class Test3 {

public void foo() {
	Currency c = Currency.getInstance("abc"); //$NON-NLS-1$
	ArrayList<String> b = new ArrayList();
	java.util.Calendar d= Calendar.getInstance();
}

}`

With this code and the "preserve star imports" approach active an organize import operation would never replace the existing * import with resolved single imports for Currency, ArrayList and Calendar, but leave the existing star import in place.

It's always very annoying, if I have a star import for a changing number of used classes from that package and every hit on Organize Imports will resolve (or not resolve) the star. There's no reason to touch it.

Well, maybe preserving star imports should not be applied, if "number of imports needed for *" is set. Additionally there should be a preferences setting for "preserve star imports", that should possibly depend on "number of imports for *" being set. Well, I'll leave that up to you to decide.

[iloveeclipse]

@m-froehlich : please do following (in your local environment, not by github web interface):

1. Squash your 3 commits into one, providing good commit message
2. Fetch latest master branch state and rebase your "patch-2" branch on top of latest master commit
3. Add a dedicated commit to bump last segment in org.eclipse.jdt.core.manipulation bundle version by +100 in both manifest and pom files.
4. Force push your branch to github.

[jjohnstn]

Hi @m-froehlich I went ahead and added preferences support because such a change cannot change current behavior without the user's consent, but I find that the original patch code you added doesn't get invoked in your example. What is happening in your example is that there are no unresolved types and when TypeReferenceProcessor.process() is called, it sees that unresolvedTypes.size() is 0 and immediately returns false. It never calls TypeReferenceProcessor.processTypeInfo() where your change is. The importRewrite() then uses the number of imports setting.

Is there something I have missed or do you have another example you used in your testing?

[jjohnstn]

@m-froehlich I have updated the patch after rebasing and adding preference support.

[jjohnstn]

As mentioned, I can't get the original patch code to be run in the example scenario provided.

[iloveeclipse]

I've cleared 4.28 M2 milestone because M2 is done already. Feel free to set more suitable milestone.

[jukzi]

please finish work on this PR or close it