eclipse / lsp4e

Language Server Protocol support in Eclipse IDE
Eclipse Public License 2.0
63 stars 54 forks source link

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

Open BoykoAlex opened 1 year ago

BoykoAlex commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Isn't it the root issue?

rubenporras commented 1 year ago

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 commented 1 year ago

@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 commented 1 month ago

@BoykoAlex can we close this issue?

BoykoAlex commented 1 month ago

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

BoykoAlex commented 1 month ago

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 commented 3 weeks ago

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?