Fix java completion sort for not-imported items.

[mbien]

LazySortText implements CharSequence but misses a toString() method.

Without it, it would use Object#toString(), and since it is used as comparator input, it would lead to undesired results.

This was the case with 'List.of' for example. ('List' must not have an import yet)

before

after

test

public class Mavenproject1 {
    public static void main(String[] args) {
        // use completion after 'of'
        java.util.Set.of; // sort works
        List.of;          // sort fails
    }
}

---

assignment (enclSortText can be of type String or LazySortText):

netbeans/java/java.editor/src/org/netbeans/modules/editor/java/JavaCompletionItem.java

Lines 1564 to 1568 in 03aa32d

	if (this.autoImportEnclosingType) {
	this.enclSortText = new LazySortText(elem.getEnclosingElement().getSimpleName().toString(), null, ElementHandle.create((TypeElement)elem.getEnclosingElement()), referencesCount);
	} else {
	this.enclSortText = ""; //NOI18N
	}

search token:

netbeans/java/java.editor/src/org/netbeans/modules/editor/java/JavaCompletionItem.java

Line 1913 in 03aa32d

sortText = simpleName + "#" + enclSortText + "#" + ((cnt < 10 ? "0" : "") + cnt) + "#" + sortParams.toString(); //NOI18N

comparator:

netbeans/ide/editor.completion/src/org/netbeans/modules/editor/completion/CompletionItemComparator.java

Lines 59 to 60 in 03aa32d

	int alphabeticalDiff = compareText(i1.getSortText(), i2.getSortText());
	return alphabeticalDiff;

[mbien]

i checked if I could add a test but couldn't find existing tests which verify full completion results. They seem to typically implement one result only.

[matthiasblaesing]

I think this need review from @dbalek. Looking at the charAt method I have to conclude, that string concatenation was deemed expensive, else this whole class makes no sense and that raises the question how often the new codepath is actually hit.

If I see this correctly this codepath will be hit by FieldItem and MethodItem, which both modify the sortText with the enclosing element, while ClassItem, LazyJavaCompletionItem and StaticMemberItem.

Looking at LazySortText further I see enclName as parameter. It is unclear ,why it FieldItem and MethodItem don't use that.

In general I see why this change is done, I can reproduce the problem and verified it fixes the problem, so I'm leaning to a +1.

[mbien]

I believe it is safe to say (given the name) that the purpose of the class is delayed evaluation of the full char sequence. My hypothesis is that charAt was only implemented since it was required by the interface, toString has been overlooked though - since Object has a default impl - so this would be plausible.

I could be wrong but my guess is that charAt is a red hering - the main purpose of this is lazy init. (even if the original idea was to sort by partially evaluate the char sequence via charAt, it doesn't seem to be used that way now - it only cares about the full string)

regarding performance,

netbeans/java/java.editor/src/org/netbeans/modules/editor/java/JavaCompletionItem.java

Line 1899 in 03aa32d

public CharSequence getSortText() {

the value is cached, so toString should only be called once per item - i am pretty sure I checked this by setting a breakpoint.

Looking at LazySortText further I see enclName as parameter. It is unclear ,why it FieldItem and MethodItem don't use that.

yes I did wonder about this too

[mbien]

but this shows again that impls need documentation, a single comment on top of that class would have made this much easier!

[matthiasblaesing]

Ok - lets get this in - it fixes a real problem.

[mbien]

made a small change since the sortText value for FieldItem wasn't cached like in MethodItem.

I did also take another look at this and LazySortText#charAt was never called before this change here, due to the fact that the CharSequence got concatenated to other Strings using its default toString() method.

Now its called while toString() is building the String, which happens once per item.

[mbien]

I implemented it properly with a utility class which can link CharSequences. This comes close to the original performance while also sorting correctly and fixing this issue. Tested with some synthetic files which had 10k fields and methods.

[matthiasblaesing]

Looks sane to me and the new solution looks elegant.