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

Completion items are not sorted when extra text typed after completion invocation #842

Open
BoykoAlex opened this issue Oct 9, 2023 · 8 comments

Comments

@BoykoAlex
Copy link
Contributor

BoykoAlex commented Oct 9, 2023

The LSContentAssistProcessor sorts completions based on the rank when completions are first computed. If extra chars are typed after the initial CA invocation then the list of proposals is filtered (as invalid proposals are filtered out) but never sorted again based on the document filter text. The generic editor ContentAssistant is null thus nothing is sorted after the initial invocation. The result is irrelevant completion proposals ends up at the top of the list.
It'd be great if somehow we could set the sorter on the ContentAssistant... I couldn't find a way unfortunately and ended up using reflection just to test the hypothesis.
After writing the code below in lsp4e i was able to achieve the desired effect:

	private void installSorterIfAbsetnt(ITextViewer viewer) {
		if (viewer instanceof SourceViewer) {
			try {
				Field f = SourceViewer.class.getDeclaredField("fContentAssistant"); //$NON-NLS-1$
				f.setAccessible(true);
				Object o = f.get(viewer);
				if (o instanceof ContentAssistant) {
					ContentAssistant contentAssitant = (ContentAssistant) o;
					Field sorterField = ContentAssistant.class.getDeclaredField("fSorter"); //$NON-NLS-1$
					sorterField.setAccessible(true);
					if (sorterField.get(contentAssitant) == null) {
						contentAssitant.setSorter(new ICompletionProposalSorter() {

							@Override
							public int compare(ICompletionProposal p1, ICompletionProposal p2) {
								if (p1 instanceof LSCompletionProposal && p2 instanceof LSCompletionProposal) {
									return proposalComparator.compare((LSCompletionProposal) p1, (LSCompletionProposal) p2);
								}
								return 0;
							}
						});
					}
				}
			} catch (Exception e) {
				// ignore
			}
		}
	}

	@Override
	public ICompletionProposal[] computeCompletionProposals(ITextViewer viewer, int offset) {
		installSorterIfAbsetnt(viewer);

		IDocument document = viewer.getDocument();

                ....

I'd be more than happy to provide a patch if someone points in the right direction to implement this elegantly :-)

@mickaelistria
Copy link
Contributor

One possible workaround can probably be to use isIncomplete: true int the completion results, so LSP4E would then re-invoke, and hopefully re-sort, the results.

@mickaelistria
Copy link
Contributor

The generic editor ContentAssistant is null thus nothing is sorted after the initial invocation.

Isn't it the root issue?

@rubenporras
Copy link
Contributor

While isIncomplete: true will probably work, we should avoid it, as it will cause additional roundtrips.

Not being able to reorder is an usability problem, so it would be nice if someone would have time to fix it. Maybe it requires a platform change?

@BoykoAlex
Copy link
Contributor Author

@mickaelistria isIncomplete: true was the first thing I've tried but it didn't work. After some digging i think i see why it didn't work for me.

isIncomplete only checked here:

	@Override
	public boolean isValidFor(IDocument document, int offset) {
		return (!isIncomplete || offset == this.initialOffset) && validate(document, offset, null);
	}

but looking at the code in CompletionProposalPopup classes i see this:

			if (proposal instanceof ICompletionProposalExtension2) {

				ICompletionProposalExtension2 p= (ICompletionProposalExtension2) proposal;
				try {
					if (p.validate(document, offset, event))
						filtered.add(proposal);
				} catch (RuntimeException e) {
					// Make sure that poorly behaved completion proposers do not break filtering.
				}
			} else if (proposal instanceof ICompletionProposalExtension) {

				ICompletionProposalExtension p= (ICompletionProposalExtension) proposal;
				try {
					if (p.isValidFor(document, offset))
						filtered.add(proposal);
				} catch (RuntimeException e) {
					// Make sure that poorly behaved completion proposers do not break filtering.
				}
			} else {
				// restore original behavior
				fIsFilteredSubset= false;
				fInvocationOffset= offset;
				fContentAssistant.fireSessionRestartEvent();
				fComputedProposals= computeProposals(fInvocationOffset);
				return fComputedProposals;
			}

Thus it never gets to p.isValidFor(document, offset) but instead calls p.validate(document, offset, event)

I put the following code in LSCompletionProposal and then it worked properly:

	@Override
	public boolean validate(IDocument document, int offset, DocumentEvent event) {
		if (item.getLabel() == null || item.getLabel().isEmpty()) {
			return false;
		}
		if (offset < this.bestOffset) {
			return false;
		}
		try {
			String documentFilter = getDocumentFilter(offset);
			if (!documentFilter.isEmpty()) {
				return /* CHANGE START*/!(isIncomplete && currentOffset != initialOffset) && /* CHANGE END */ CompletionProposalTools.isSubstringFoundOrderedInString(documentFilter, getFilterString());
			} else if (item.getTextEdit() != null) {
				return offset == LSPEclipseUtils.toOffset(getTextEditRange().getStart(), document);
			}
		} catch (BadLocationException e) {
			LanguageServerPlugin.logError(e);
		}
		return true;
	}

@sebthom
Copy link
Member

sebthom commented Sep 6, 2024

@BoykoAlex can we close this issue?

@BoykoAlex
Copy link
Contributor Author

BoykoAlex commented Sep 6, 2024

Hmm... I think so, but let me check this out first and get back to you

@BoykoAlex
Copy link
Contributor Author

Looks like the merged PR is only to address the support for isIncomplete: true. The issue is that we need completion proposals sorter for the isIncomplete: false as well. Seems that a platform change is required to get this fixed.

@martinlippert
Copy link
Contributor

Does anyone have a pointer here where the isIncomplete case is handled or - to be more precise - where the new attempt to grab an updated list of proposals from the language server is triggered after the user typed more characters?

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

No branches or pull requests

5 participants