Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Exclude certain packages from autocomplete/autoimport #1176

Merged
merged 1 commit into from
Sep 16, 2019
Merged

Exclude certain packages from autocomplete/autoimport #1176

merged 1 commit into from
Sep 16, 2019

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Sep 14, 2019

@snjeza snjeza requested review from gorkem and fbricon September 14, 2019 01:01
@snjeza snjeza mentioned this pull request Sep 14, 2019
Merged
fbricon
fbricon requested changes Sep 14, 2019
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/Preferences.java Outdated
@@ -801,6 +813,14 @@ public Preferences setImportOrder(List<String> importOrder) {
return this;
}

public Preferences setFilteredTypes(List<String> filteredTypes) {
this.filteredTypes = (filteredTypes == null || filteredTypes.size() == 0) ? JAVA_COMPLETION_FILTERED_TYPES_DEFAULT : filteredTypes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user decides to remove filters (sets empty list), we should not force our default filters

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/TypeFilter.java Outdated
private StringMatcher[] fStringMatchers;

/**
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty javadoc

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/Preferences.java Outdated
@@ -368,6 +374,8 @@
JAVA_IMPORT_ORDER_DEFAULT.add("javax");
JAVA_IMPORT_ORDER_DEFAULT.add("com");
JAVA_IMPORT_ORDER_DEFAULT.add("org");
JAVA_COMPLETION_FILTERED_TYPES_DEFAULT = new ArrayList<>();
JAVA_IMPORT_ORDER_DEFAULT.add("java.awt.*");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be JAVA_COMPLETION_FILTERED_TYPES_DEFAULT.add

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add com.sun.*

...clipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandlerTest.java Outdated
boolean exists = false;
for (CompletionItem item : list.getItems()) {
if ("List - java.util".equals(item.getLabel())) {
exists = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call fail() here

...e.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/commands/OrganizeImportsCommandTest.java Outdated

WorkspaceEdit rootEdit = new WorkspaceEdit();
command.organizeImportsInCompilationUnit(cu, rootEdit);
assertTrue(rootEdit.getChanges().size() == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertEquals

...e.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/commands/OrganizeImportsCommandTest.java Outdated
assertTrue(rootEdit.getChanges().size() == 0);
PreferenceManager.getPrefs(null).setFilteredTypes(Collections.emptyList());
command.organizeImportsInCompilationUnit(cu, rootEdit);
assertFalse(rootEdit.getChanges().size() == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsEmpty

@snjeza
Copy link
Contributor Author

snjeza commented Sep 14, 2019

@fbricon I have updated the PR.

fbricon
fbricon reviewed Sep 14, 2019
View reviewed changes
...e.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/commands/OrganizeImportsCommandTest.java
@@ -349,6 +397,9 @@ public void testOrganizeImportsInProject() throws CoreException, BadLocationExce

private String getOrganizeImportResult(ICompilationUnit cu, WorkspaceEdit we) throws BadLocationException, CoreException {
List<TextEdit> change = we.getChanges().get(JDTUtils.toURI(cu));
if (change == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'change' is null if there aren't any actions.

fbricon
fbricon reviewed Sep 16, 2019
View reviewed changes
...e.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/commands/OrganizeImportsCommandTest.java
PreferenceManager.getPrefs(null).setFilteredTypes(Collections.emptyList());
command.organizeImportsInCompilationUnit(cu, rootEdit);
assertFalse(rootEdit.getChanges().isEmpty());
assertEquals(buf.toString(), getOrganizeImportResult(cu, rootEdit));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you set an empty list for filtered types, how come the organize imports action managed to select java.util.List over java.awt.List?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeFilter is never called in this test. I don't understand how it even passes

Copy link
Contributor Author

@snjeza snjeza Sep 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you set an empty list for filtered types, how come the organize imports action managed to select java.util.List over java.awt.List?

java.awt.List doesn't exist when running tests.

TypeFilter is never called in this test. I don't understand how it even passes

See org.eclipse.jdt.core.manipulation.TypeNameMatchCollector.getStringMatchers()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I suspect the fake rt jars we use don't have the java.awt classes.
Thanks for the info about TypeNameMatchCollector

@fbricon fbricon merged commit 5f7a736 into eclipse-jdtls:master Sep 16, 2019
@fbricon fbricon added this to the Mid September 2019 milestone Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbricon fbricon fbricon requested changes

@gorkem gorkem Awaiting requested review from gorkem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Mid September 2019
Development

Successfully merging this pull request may close these issues.

Exclude certain packages from autocomplete/autoimport
2 participants
@snjeza @fbricon