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.

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

Support advanced organizeImports to resolve ambiguous imports #966

Merged
merged 5 commits into from
Mar 26, 2019

Conversation

testforstephen
Copy link
Contributor

@testforstephen testforstephen commented Mar 20, 2019

Signed-off-by: Jinbo Wang jinbwan@microsoft.com

See PR redhat-developer/vscode-java#850 for vscode client implementation.
organizeImports

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
@testforstephen
Copy link
Contributor Author

test this please

Copy link
Contributor

@fbricon fbricon left a comment

Choose a reason for hiding this comment

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

we also probably want to have a default selection exposed in the API, for each list of choices (even though we might not want to return a default selection at this point. Think java.util.List vs java.awt.List

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
@fbricon
Copy link
Contributor

fbricon commented Mar 22, 2019

I'd like to see the server response to look something like:

OrganizeImportsSelection:

  • uri:string //file uri
  • imports: list //hold list of choices/ranges/default selection per ambiguous import
    • default (null | int) //the index of the candidate to preselect
    • candidates: list
      • candidate: ImportChoice
      • range: Range

I believe it'll be easier to understand the API and implement the client side

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
@testforstephen
Copy link
Contributor Author

Updated the client callback api schema:

  • uri: string //file uri
  • selections: list //hold list of choices/ranges/default selection per ambiguous import
    • candidates: ImportChoice[]
    • range: Range
    • defaultSelection: int //the index of the candidate to preselect

Optional<Either<Command, CodeAction>> organizeImports = convertToWorkspaceEditAction(params.getContext(), context.getCompilationUnit(), CorrectionMessages.ReorgCorrectionsSubProcessor_organizeimports_description,
CodeActionKind.SourceOrganizeImports, organizeImportsEdit);
addSourceActionCommand($, params.getContext(), organizeImports);
if (preferenceManager.getClientPreferences().isAdvancedOrganizeImportsSupported()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The if conditional could be put insider getOrganizeImportsAction to make logic simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if is to fall back to the old organizeImport logic if the JDT LS client doesn't implement chooseImports command yet. Don't belong to getOrganizeImportsAction function.

Copy link
Contributor

@yaohaizh yaohaizh left a comment

Choose a reason for hiding this comment

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

CIL

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
@fbricon
Copy link
Contributor

fbricon commented Mar 25, 2019

Now I'm having 2nd thoughts on the default selection. The problem with it is it adds more work onto clients to implement proper sorting, when it could be done at the server level, consistently for everyone. WDYT?

We could imagine the server will eventually guarantee a sort order based on a relevancy score.

@fbricon
Copy link
Contributor

fbricon commented Mar 25, 2019

@yaohaizh CIL?

@yaohaizh
Copy link
Contributor

When submit a review feedback, it requires comments, so CIL for comment in lines

@testforstephen
Copy link
Contributor Author

testforstephen commented Mar 26, 2019

Now I'm having 2nd thoughts on the default selection. The problem with it is it adds more work onto clients to implement proper sorting, when it could be done at the server level, consistently for everyone. WDYT?

We could imagine the server will eventually guarantee a sort order based on a relevancy score.

See

		 * Optional flag indicating if this item is picked initially.
		 * (Only honored when the picker allows multiple selections.)
		 *
		 * @see [QuickPickOptions.canPickMany](#QuickPickOptions.canPickMany)
		 */
		picked?: boolean;

vscode QuickPick only supports preselecting for multiple selections. And for single selection QuickPick, it will preselect the top one by default. So i did sorting to put the default selection to the top.

I like the idea of guarantee a sort order based on a relevancy score. If the server returns the sorted candidates list, then no need of defaultSelection field any more.

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
@testforstephen
Copy link
Contributor Author

About calculating the relevancy of the candidate, one idea is to record the user picking history, and put the candidate in the history to the top. Also, if some class under the same package is existing in the history, give the candidate high priority.

In this PR, i removed the default selection logic, and add TODO tag about sorting. Will let a new PR to explore the sorting for the import candidates.

@fbricon fbricon merged commit 63c41d8 into eclipse-jdtls:master Mar 26, 2019
@fbricon
Copy link
Contributor

fbricon commented Mar 26, 2019

Nice work @testforstephen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants