Bug 506391

Comment 1 Dani Megert 2016-10-22 12:39:32 EDT

Works in 4.4.2. Broken since 4.5.

Comment 2 Noopur Gupta 2016-10-27 05:20:19 EDT

(In reply to Dani Megert from comment #0)
> Added imports expand the folded imports and shifts text.
> 
> package p;
> 
> import java.util.HashMap;
> import java.util.Map;
> 
> public class C {
> 	Map m;
> 	HashMap h;
> 	List
> }
> 
> 
> - make sure the imports are folded
> - do content assist after "List"

If java.awt.List is chosen from the proposals, the imports expand and are folded back automatically. It is the same behavior in 4.4.2 also.

If java.util.List is chosen, the imports do not expand in 4.4.2. But they expand in 4.7 and are not folded back.

Dani, any clue where the problem could be?

Comment 3 Noopur Gupta 2016-10-27 08:46:00 EDT

On debugging the 4.4 and master code, I can see that the difference is in the creation of TextEdits and handling of delimiters between imports (ImportEditor.placeResultantImports(..)).

In 4.4, a single text edit was created:
  {InsertEdit} [41,0] <<import java.util.List;
with text: "import java.util.List;\r\n"

In master, two text edits are created:
  {InsertEdit} [39,0] <<
with text: "\r\n"
followed by:
  {InsertEdit} [39,0] <<import java.util.List;
with text: "import java.util.List;"

Adding the delimiter after the text to be inserted (similar to 4.4) and adjusting the offset fixes this issue.  

(In reply to Noopur Gupta from comment #2)
> If java.awt.List is chosen from the proposals, the imports expand and are
> folded back automatically. It is the same behavior in 4.4.2 also.

This has the same behavior in 4.4 and master because here the delimiter text edit is added after the import text edit in master:
  {InsertEdit} [14,0] <<import java.awt.List;
with text: "import java.awt.List;"
followed by:
  {InsertEdit} [14,0] <<
with text: "\r\n"

Moving to JDT Core based on the above analysis.

Comment 4 Jay Arthanareeswaran 2016-12-06 01:00:49 EST

Moving out to M5.

Manoj, please take a look.

Comment 5 Manoj N Palat 2017-03-09 00:01:38 EST

(In reply to Jay Arthanareeswaran from comment #4)
> Moving out to M5.
> 
> Manoj, please take a look.

moving out due to lack of time - hope to look into this in the next milestone.

Comment 6 Manoj N Palat 2017-03-31 19:40:52 EDT

The behavior has nothing to do with List being from awt.List or util.List - essentially it is package-agnostic [ To verify use import java.awt.ActiveEvent as the first import and try completing, the same behavior as that of java.util.List is observed ] - however, the issue is about the position of the import - depending on whether this is the first import or an import inside existing imports. With the current algorithm (implemented with the top bug 430303) ImportEditor.placeResultantImports() determines the place at which the imports are placed. This goes through the list of imports, primary inputs being the last import and the current import. In the case of the inserted import being the first import, the preceding delimiter will be a zero length string, and the first text edit will just be the import. Next it goes to the second import which is existing and adds a standard delimiter as a insertEdit. So we have a multi-text edit. In the case of an import within an existing import, the preceding delimiter is calculated as a standard delimiter and then this import will be inserted forming the two components of multi-text edit.In the 4.4 times, a line delimiter is always added to the new import. Moreover, this addition was done in the  string and then a single text edit was created instead of the multi-text edit we have in the master which explains the two step text edit for master. Given the fact that, the text edits given by jdt/core are correct, and that the folding is done by jdt.ui, shouldn't this be handled by jdt.ui? - don't want to get it into a ping-pong mode - so not reassigning back for now - let know your thoughts on this.

Comment 7 Manoj N Palat 2017-04-17 09:32:26 EDT

Created attachment 267819 [details]
patch used for testing jdt.core

Comment 8 Manoj N Palat 2017-04-17 10:12:38 EDT

(In reply to Manoj Palat from comment #7)
> Created attachment 267819 [details]
> patch used for testing jdt.core

Wrong bug..

Comment 9 Noopur Gupta 2017-04-20 05:14:18 EDT

Analyzed this issue with Markus. The problem happens when a delimiter is added at the end of the first import in the folded imports container, which now happens based on the new text edits from bug 430303.

This issue happens in other cases also. 

Example 1:

package p;

import java.io.InputStream;
import java.io.OutputStream;

public class D {
	void foo(InputStream is, OutputStream os) {}
}

package p;

import java.io.BufferedInputStream;
import java.io.StringWriter;

public class E extends D {
	BufferedInputStream bufferedInputStream;
	StringWriter sw;
	foo|
}

- Fold the imports in E.java.
- Invoke content assist at | to override #foo.
=> Imports are expanded.

Example 2:

package p;

import java.util.HashMap;import java.util.List;
import java.util.Map;

public class C {
	Map m;
	HashMap h;
	List e;
}

- Fold the imports in C.java.
- Invoke Organize Imports.
=> Imports are expanded.


The code that handles this expansion is in Platform Text:
org.eclipse.jface.text.source.projection.ProjectionViewer.handleVisibleDocumentChanged(DocumentEvent event):
if (numberOfLines > 1 || fDeletedLines > 1)
    fProjectionAnnotationModel.expandAll(master.getOffset(), replaceLength);

We tried to avoid the expansion here only for the imports container as the expansion is relevant in all other cases. But it will need this information to be propagated from 
org.eclipse.jdt.ui.text.folding.DefaultJavaFoldingStructureProvider.JavaProjectionAnnotation and will require multiple changes in the framework APIs. Hence, this is not a good solution.

Comment 10 Manoj N Palat 2017-05-04 02:28:26 EDT

Further offline analysis alongwith Markus: We may leave this as  an acceptable behavior and leave it as won't fix.

Comment 11 Sasikanth Bharadwaj 2017-05-10 04:51:16 EDT

Verified for 4.7 M7