Fixes textEdit completions

[isundaylee]

Currently the original replacement prefix sent by the LSP server via the textEdit field is overwritten in getSuggestions(). This, for example, causes the following case:

foo->

to autocomplete to

foo->->bar()

instead of the expected

foo->bar()

This PR fixes the issue by saving the original replacement prefix (-> in the example above), and modifying getSuggestions() to append to instead of overwrite this original replacement prefix.

[isundaylee]

BTW are tests currently disabled? I was trying to add a regression test for this, but bothnpm test and the CIs give:

Finished in 0 seconds
0 tests, 0 assertions, 0 failures, 0 skipped

[Aerijo]

So I'm coincidentally adding autocomplete to my own server now, making this a timely PR. I've only just gone over the LSP spec, so I'll need more time to get familiar with it.

I don't like the idea of adding a new map for each suggestion, when it should be possible to store the info directly on the suggestion itself. I don't know how this will jive with TypeScript though.

BTW are tests currently disabled?

No idea, but that doesn't look right. I don't think it concerns this PR, but I'll maybe look into it later.

[isundaylee]

I agree – I don't particularly like having a separate map either. But each suggestion itself is an ac.AnySuggestion, and it's kind of awkward to tag along a field that is not part of the autocomplete-plus struct. Maybe merge originalReplacementPrefixMap into the existing suggestionMap? Or add an originalReplacementPrefix field to PossiblyResolvedCompletionItem?

[winstliu]

Tests are running again if you want to update your branch.

[isundaylee]

@50Wliu Thanks, will do!

[Aerijo]

RIght, so I've been struggling with this module and Latex completions (which start with a \ -_-). I've currently implemented an analog of this PR, but redone most types from ac.AnySuggestion to Suggestion, which is derived from the first but is locally defined and so can have the extra properties.

I don't know how "elegant" this is (there are a few as ac.AnySuggestion conversions; the specs have a penchant for creating ac.AnySuggestion suggestions literally), but it works and it doesn't need a separate map for the extra properties.

@50Wliu You've been relatively active here today, do you know any best practices for this problem?

The ideal solution would also have autocomplete-plus allowing a replacement range, as an alternative to a replacement prefix. It would need to work with multiple cursors, but still.

[winstliu]

@50Wliu You've been relatively active here today, do you know any best practices for this problem?

Nope, I know nothing about how this all works, sorry. Just wanted to smooth out the development process to make it easier for contributors.

[isundaylee]

@Aerijo Do you have your changes up in Github somewhere? Would love to test your version to check if it fixes my cases.

[Aerijo]

@isundaylee https://github.com/Aerijo/atom-languageclient/tree/autocomplete-huh

[isundaylee]

@Aerijo Thanks! It works for my cases too. If you make a PR I can close this one in favor of that one.

One minor thing: Do you need to export LSPTextSuggestion et friends? Seems like they're only used internally.

[Aerijo]

That was just to get things working. I hadn't put thought into the exact types and exports. I believe they are supposed to be used in place of ac.TextSuggestion, but I'll go through and clean it up before making a PR.

[isundaylee]

@Aerijo Ah just came across a subtle issue - the original replacement prefix might not be completely before the cursor. Consider the following case:

#include "coro/|"

where we trigger auto-completion at |. The language server might return the following completion:

replace the range of text containing
#include "core/"             <-- replacement prefix
to
#include "core/Support.h"

Note that in this case the last character of the replacement prefix is actually after the cursor. Suppose the user now types "Sup", making the current text:

#include "coro/Sup|"

In this case, the correct replacementPrefix we should send to autocomplete-plus is #include "coro/Sup. The current fix gives #include "coro/"Sup.

Since autocomplete-plus doesn't support replacing text after the cursor, one way to fix this is that when we get a suggestion from the server, we strip the after-cursor part from both the replacement prefix and completion text. Any other idea?

[Aerijo]

Yeah, I was thinking about that. The problem is that autocomplete-plus only takes a text replacement, and applies it to all cursors that match the text. Ideally it would allow a range based replacement too, but I'm not sure how that should work with multiple cursors (or how it should work currently with multiple cursors).

Does making the change in this PR cause that issue, or has it always existed?

[isundaylee]

@Aerijo Yeah I agree - the thing I proposed would only be a workaround.

The issue existed before. But for some reason it went away after my commits (I haven't yet investigated how it went away - I did not explicitly fix that case in my commits).

[Aerijo]

@isundaylee

Do you need to export LSPTextSuggestion et friends?

They are used when constructing a Suggestion from the CompletionItem, so yes.

The issue existed before. But for some reason it went away after my commits (I haven't yet investigated how it went away - I did not explicitly fix that case in my commits).

Is this reproducible? AFAIK autocomplete plus mostly lacks support for replacing anything right of the cursor, so it may have been something else you were seeing. There is one exception, where it removes the trailing text if it matches the rest of the prefix (e.g., l|og will still expand to console.log(|);, but l|og() expands to console.log(|);())

strip the after-cursor part from both the replacement prefix and completion text. Any other idea?

You mean strip the after-cursor text before expanding the completion? I don't think that's possible right now either; the only event we can act on is after expansion. It may still be possible to detect trailing text that was supposed to be deleted and remove it, but that could get complicated (and be a pain with undo/redo).

Anyways, I'll likely be merging a PR tomorrow that converts to the custom Suggestion type, allowing the replacementPrefix information to be stored there. The after-cursor prefix text handling can be solved at some other time, especially as it was an existing issue before this PR.

[Aerijo]

@isundaylee OK, the new types are merged. Do you want to make a PR for it, or I can make one later?

[isundaylee]

@Aerijo That's great! You can go ahead and make a PR if you have the fix more-or-less ready. Otherwise, let me know and I can make one.

Regarding my previous comments, by

strip the after-cursor part from both the replacement prefix and completion text. Any other idea?

I mean, if the server returns the following:

Replace
#include "|"
to
#include "Something.h"

we can detect that it is trying to replace 1 character after the cursor. We then remove 1 character from both the replacement text and the new text, modifying the completion to as follows:

Replace
#include "|
to
#include "Something.h

This is a workaround that will only work if the after-cursor part is the same in the replacement text and the new text (as in my #include case).

[Aerijo]

Oh, I think I understand what you mean about the trailing text now. So it covers a specific subset of possible ranges (which is still better than not at all).

As for the topic of this PR, the triggerPoint should actually be enough to recalculate the prefix for most completions. If we differentiate between completions that return in the range TP to request BP (likely the majority), and ones that have a custom range, then we can take the replacement text of the first set to be whatever is between TP (constant position) and BP (position for given request).

It’s not perfect, because the user can move the cursor around, but it’s better than the existing behaviour. Coupled with the after-text trickery you suggested, it should work for most reasonable completions.

[isundaylee]

@Aerijo Yeah that sounds good. Do you wanna do the PR? Or should I?

[laughedelic]

Hi! Can we revive this PR? is it still relevant after the latest changes in master? The problem still exists and it would be nice to merge this if it is a solution.

[Aerijo]

@laughedelic Does the current master branch fix laughedelic/atom-ide-scala#79?