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

The AC system sometimes deletes too many characters when committing a completion. #3296

Closed
deathaxe opened this issue Apr 26, 2020 · 6 comments

Comments

@deathaxe
Copy link
Collaborator

deathaxe commented Apr 26, 2020

Description

This issue continues the discussion about ST removing too many characters before calling a command_completion which was started at discord and continued by the comments at https://gist.github.com/jskinner/4dfa6d86f848e1e1e583883507369b67#gistcomment-3270107

It intends to provide some examples which hopefully help to find a proper solution.

For some reason ST removes the leading < even though typing started behind. As it is not part of the response of a language-server it gets lost.

Used plugins

LSP / st4000-exploration branch
LSP-lemmix

Used file

<?xml version="1.0"?>
<xs:schema 
    targetNamespace="http://sublimehq.com/schemas/sublime-snippet"
    xmlns:xs="http://www.w3.org/2001/XMLSchema"
    elementFormDefault="qualified">

    <xs:element name="snippet">
    </xs:>

</xs:schema>

Steps to reproduce

  1. Install LSP and LSP-lemminx
  2. Restart ST
  3. Create a folder with an debug.xsd provided below
  4. Complete the closing xml tag.

Expected behavior

Sublime Text should not delete too many characters. It may provide some API to either leave the whole text replacement up to a commit or let a plugin provide syntax specific details which may help ST's AC system to decide what to remove. The latter one would mean to pass the textEdit ranges to ST.

By replacing the label by filterText - as discussed in the gist - the completions are applied as expected.

Animation

Note: Whether filterText should be used as trigger or which options would be possible to support the label part should be discussed in a separate issue.

Actual behavior

Let me provide another example in which the closing tag </xs:> is not completed as expected as ST deletes the leading < which is not part of the textEdit range returned by the LemMinX server.

Animation

LSP payload logs

  1. Type the : character

    textDocument/didChange: {
        'contentChanges': [
            {
                'range': {
                    'start': {'character': 8, 'line': 7}, 
                    'end': {'character': 8, 'line': 7}
                }, 
                'rangeLength': 0,
                'text': ':'
            }
        ], 
        'textDocument': {
            'uri': 'file:///C:/Data/xmldebug/debug.xsd', 
            'version': 1
        }
    }
    
  2. Character : triggers completions

    textDocument/completion(5): {
        'position': {'character': 9, 'line': 7},
        'textDocument': {'uri': 'file:///C:/Data/xmldebug/debug.xsd'}
    }
    
  3. Receiving the following respons

    textDocument/completion-response(5): {
        'isIncomplete': False, 
        'items': [
            {
                'filterText': '/xs:',
                'kind': 10,
                'insertTextFormat': 1,
                'textEdit': {
                    'range': {
                        'start': {'character': 5, 'line': 7},
                        'end': {'character': 9, 'line': 7}
                    },
                    'newText': '/xs:'
                },
                'label': "End with '</xs:>'"
            },
            {
                'filterText': '/xs:element',
                'kind': 10,
                'insertTextFormat': 1,
                'textEdit': {
                    'range': {
                        'start': {'character': 5, 'line': 7},
                        'end': {'character': 9, 'line': 7}
                    },
                    'newText': '/xs:element'
                },
                'label': "End with '</xs:element>'"
            }
        ]
    }
    
  4. Passing the following completion items to ST (EDIT: added 2020/04/27)

    CompletionItem(
        trigger=End with '</xs:>',
        annotation='',
        completion=lsp_select_completion_item {"filterText": "/xs:", "insertTextFormat": 1, "kind": 10, "label": "End with '</xs:>'", "native_region": [223, 227], "textEdit": {"newText": "/xs:", "range": {"end": {"character": 9,    "line": 7}, "start": {"character": 5, "line": 7}}}},
        completion_format=2,
        kind=(7, 'ρ', 'Property'),
        details=''
    )
    
    CompletionItem(
        trigger=End with '</xs:element>',
        annotation='',
        completion=lsp_select_completion_item {"filterText": "/xs:element", "insertTextFormat": 1, "kind": 10, "label": "End with '</xs:element>'", "native_region": [223, 227], "textEdit": {"newText": "/xs:element", "range": {"end":    {"character": 9, "line": 7}, "start": {"character": 5, "line": 7}}}},
        completion_format=2,
        kind=(7, 'ρ', 'Property'),
        details=''
    )
    
  5. Sending text modification notification caused by commit_completion command.

    textDocument/didChange: {
        'contentChanges': [
            {'range': {'start': {'character': 4, 'line': 7}, 'end': {'character': 9, 'line': 7}}, 'rangeLength': 5, 'text': ''}, 
            {'range': {'start': {'character': 4, 'line': 7}, 'end': {'character': 4, 'line': 7}}, 'rangeLength': 0, 'text': '/'}, 
            {'range': {'start': {'character': 5, 'line': 7}, 'end': {'character': 5, 'line': 7}}, 'rangeLength': 0, 'text': 'x'}, 
            {'range': {'start': {'character': 6, 'line': 7}, 'end': {'character': 6, 'line': 7}}, 'rangeLength': 0, 'text': 's'}, 
            {'range': {'start': {'character': 7, 'line': 7}, 'end': {'character': 7, 'line': 7}}, 'rangeLength': 0, 'text': ':'}, 
            {'range': {'start': {'character': 8, 'line': 7}, 'end': {'character': 8, 'line': 7}}, 'rangeLength': 0, 'text': 'e'}, 
            {'range': {'start': {'character': 9, 'line': 7}, 'end': {'character': 9, 'line': 7}}, 'rangeLength': 0, 'text': 'l'}, 
            {'range': {'start': {'character': 10, 'line': 7}, 'end': {'character': 10, 'line': 7}}, 'rangeLength': 0, 'text': 'e'}, 
            {'range': {'start': {'character': 11, 'line': 7}, 'end': {'character': 11, 'line': 7}}, 'rangeLength': 0, 'text': 'm'}, 
            {'range': {'start': {'character': 12, 'line': 7}, 'end': {'character': 12, 'line': 7}}, 'rangeLength': 0, 'text': 'e'}, 
            {'range': {'start': {'character': 13, 'line': 7}, 'end': {'character': 13, 'line': 7}}, 'rangeLength': 0, 'text': 'n'}, 
            {'range': {'start': {'character': 14, 'line': 7}, 'end': {'character': 14, 'line': 7}}, 'rangeLength': 0, 'text': 't'}
        ], 
        'textDocument': {
            'uri': 'file:///C:/Data/xmldebug/debug.xsd',
            'version': 13
        }
    }
    

Another example is by requesting completions with the caret not being placed at the end but in the middle of the incomplete tag.

Animation

  1. Press ctrl+space to trigger completions

    textDocument/completion(18): {
        'position': {'character': 6, 'line': 7},
        'textDocument': {
            'uri': 'file:///C:/Data/xmldebug/debug.xsd'
        }
    }
    
  2. Receive the completions from the server

    textDocument/completion-response(18): {
        'isIncomplete': False,
        'items': [
            {
                'filterText': '/xs:',
                'kind': 10,
                'insertTextFormat': 1,
                'textEdit': {
                    'range': {
                        'start': {'character': 5, 'line': 7},
                        'end': {'character': 9, 'line': 7}
                    },
                    'newText': '/xs:'
                },
                'label': "End with '</xs:>'"
            },
            {
                'filterText': '/xs:element',
                'kind': 10,
                'insertTextFormat': 1,
                'textEdit': {
                    'range': {
                        'start': {'character': 5, 'line': 7},
                        'end': {'character': 9, 'line': 7}
                    },
                    'newText': '/xs:element'
                },
                'label': "End with '</xs:element>'"
            }
        ]
    }
    
  3. Passing the following completion items to ST (EDIT: added 2020/04/27)

    CompletionItem(
       trigger=End with '</xs:>',
       annotation=,
       completion=lsp_select_completion_item {"filterText": "/xs:", "insertTextFormat": 1, "kind": 10, "label": "End with '</xs:>'", "native_region": [223, 227], "textEdit": {"newText": "/xs:", "range": {"end": {"character": 9, "line": 7}, "start": {"character": 5, "line": 7}}}},
       completion_format=2,
       kind=(7, 'ρ', 'Property'),
       details=''
     )
    
    CompletionItem(
       trigger=End with '</xs:element>',
       annotation=,
       completion=lsp_select_completion_item {"filterText": "/xs:element", "insertTextFormat": 1, "kind": 10, "label": "End with '</xs:element>'", "native_region": [223, 227], "textEdit": {"newText": "/xs:element", "range": {"end": {"character": 9, "line": 7}, "start": {"character": 5, "line": 7}}}},
       completion_format=2,
       kind=(7, 'ρ', 'Property'),
       details=''
     )
    
  4. Send text change notification after running commit_completion command

    textDocument/didChange: {
        'contentChanges': [
            {'range': {'start': {'character': 4, 'line': 7}, 'end': {'character': 6, 'line': 7}}, 'rangeLength': 2, 'text': ''}, 
            {'range': {'start': {'character': 4, 'line': 7}, 'end': {'character': 7, 'line': 7}}, 'rangeLength': 3, 'text': ''}, 
            {'range': {'start': {'character': 4, 'line': 7}, 'end': {'character': 4, 'line': 7}}, 'rangeLength': 0, 'text': '/'}, 
            {'range': {'start': {'character': 5, 'line': 7}, 'end': {'character': 5, 'line': 7}}, 'rangeLength': 0, 'text': 'x'}, 
            {'range': {'start': {'character': 6, 'line': 7}, 'end': {'character': 6, 'line': 7}}, 'rangeLength': 0, 'text': 's'}, 
            {'range': {'start': {'character': 7, 'line': 7}, 'end': {'character': 7, 'line': 7}}, 'rangeLength': 0, 'text': ':'}, 
            {'range': {'start': {'character': 8, 'line': 7}, 'end': {'character': 8, 'line': 7}}, 'rangeLength': 0, 'text': 'e'}, 
            {'range': {'start': {'character': 9, 'line': 7}, 'end': {'character': 9, 'line': 7}}, 'rangeLength': 0, 'text': 'l'}, 
            {'range': {'start': {'character': 10, 'line': 7}, 'end': {'character': 10, 'line': 7}}, 'rangeLength': 0, 'text': 'e'}, 
            {'range': {'start': {'character': 11, 'line': 7}, 'end': {'character': 11, 'line': 7}}, 'rangeLength': 0, 'text': 'm'}, 
            {'range': {'start': {'character': 12, 'line': 7}, 'end': {'character': 12, 'line': 7}}, 'rangeLength': 0, 'text': 'e'}, 
            {'range': {'start': {'character': 13, 'line': 7}, 'end': {'character': 13, 'line': 7}}, 'rangeLength': 0, 'text': 'n'}, 
            {'range': {'start': {'character': 14, 'line': 7}, 'end': {'character': 14, 'line': 7}}, 'rangeLength': 0, 'text': 't'}
        ],
        'textDocument': {
            'uri': 'file:///C:/Data/xmldebug/debug.xsd',
            'version': 86
        }
    }
    

Environment

  • Build: 4073
  • Operating system and version: Windows 10 x64 1903
@jskinner
Copy link
Member

In terms of the referenced gist, Sublime Text is working as designed there: Given text in the buffer of "acd", and a completion "abcd", then pressing tab and selecting the "abcd" option will result in the buffer containing "abcd", and not "acdabcd".

Now clearly that's not what the intention behind a </foo> style completion is. I could add heuristics, such as any leading prefix of non-word characters on a trigger must match exactly, but it's unlikely to be satisfying (for example, rules such as all non-word characters match match exactly fail on otherwise quite reasonable triggers).

The next build with have an experimental flag that can be specified per-command completion item to have Sublime Text not remove the prefix at all, in which case the plugin will be responsible for that (including removing any characters the user typed after the completion was queried)

With regards to the specific issue raised in this issue, I can't really comment on that without knowing the specific CompletionItem that the LSP plugin generated.

@deathaxe
Copy link
Collaborator Author

Thanks for looking into that tricky edge case.

I've added the CompletionItem representations to the initial post.

I fully agree with language independed heuristics to be a dangerous way which might create more edge cases that it was possible to solve.

Looking forward to the "experimental flag".

@deathaxe
Copy link
Collaborator Author

The approach described at https://discordapp.com/channels/280102180189634562/645268178397560865/704081503004524666 sounds quite interesting after an birds eye on it.

It would enable a plugin to reset the buffer to the state the completion request was sent to a lsp server. This would enable plugins to apply the textEdit region returned by the server in a correct fashion as if no time went by and user wouldn't have typed anything. The textEdit regions already contain all information required to properly replace the prefix then.

def on_query_completions():
    completion_list = sublime.CompletionList()

    change_id = view.change_id() # reference to a particular entry in the undo stack

    completio_items = [
        sublime.CompletionItem.command_completion(
            trigger="hello",
            command="select_completion_item",
            args={
                "change_id": change_id  # we assing the change id to the completion item
            }
        )
    ]

    sublime.set_timeout(lambda: completion_list.set_completions(items), 5000) #emulate slow response

    return completion_list

class SelectCompletionItemCommand(sublime_plugin.TextCommand):
    def run(self, edit, change_id):

        self.view.time_travel(edit, change_id) 
        # pop all changes from the undo stack till we reached the particular change_id reference. This will restore the view state, in a state like it was when change_id was called (in LSP case, when the request was send). Note that we passed an edit token, sublime will use that token to change the view contents if it necessary.

@rwols
Copy link

rwols commented Apr 27, 2020

That's exactly what sublime.COMPLETION_FLAG_KEEP_PREFIX promises to do :)

I'll quote here for people that don't (want to) use discord:

The next build of sublime text will have a per-completion item flag you can specify (for command completions), sublime.COMPLETION_FLAG_KEEP_PREFIX, that will ensure the text buffer is in exactly the state it was in when on_query_completions was called. In other words, the prefix won't be removed, and any text the user types while filtering the completions will be reverted.

@deathaxe
Copy link
Collaborator Author

Honestly, more and more I agree with https://forum.sublimetext.com/t/discord-server-vs-officially-hosted-forum/50920

Discord is a black hole for information. A bad place to store information in.

@rwols
Copy link

rwols commented Apr 28, 2020

With COMPLETION_FLAG_KEEP_PREFIX it's possible to handle any textEdit. I believe this can be closed. I have hacked in an initial concept:

Peek 2020-04-28 16-34

The fact that > is a trigger char somewhat bothers me, but oh well.

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

No branches or pull requests

4 participants