Add insert-replace support for completions #1809

predragnikolic · 2021-07-25T10:22:06Z

New:

setting for determining the completion_insert_mode(defaults to "insert") Available: "insert" or "replace".
enter/tab will commit the completion item with the default insert mode.
shift+enter/shift+tab will commit the completion item with the opposite insert mode.

I have tested this only with the LSP-angular server.

jwortmann · 2021-07-25T18:44:22Z

It's pretty cool, but at least Shift+Tab conflicts with a default keybinding when the autocomplete popup is visible (described in the ST preferences under "tab_completion"). Maybe it's better to leave the keybinding commented out by default, like most of the other keybindings? Additionally I would add another context {"key": "lsp.session_with_capability", "operand": "completionProvider"} to it.

An alternative could be Ctrl+Tab ("primary+tab"), which indeed has a default binding too (switch to next file tab), but not specific to when the autocompletion popup is visible. But it's not as easy to type as Shift+Tab.

The possible options "insert"/"replace" should probably also be mentioned in the comment which describes the setting.

rwols · 2021-07-25T18:46:59Z

I was thinking of adding additional text in the st_detail field to distinguish insert/replace variants, instead of a command. The current diff isn’t very discoverable. I wonder how many people will really use this feature.

LSP.sublime-settings

rchl · 2021-07-25T20:21:02Z

I was thinking of adding additional text in the st_detail field to distinguish insert/replace variants, instead of a command. The current diff isn’t very discoverable. I wonder how many people will really use this feature.

Even knowing about this feature, I probably wouldn't remember to pick the right insert mode by using a different shortcut. Especially if we don't indicate anywhere that the "replace" is supported by the given completion. Without indication, the user would never know if both the server and the given completion supports it and it would be a guessing game.

jwortmann · 2021-07-25T21:45:33Z

I would maybe even remove the setting and always use the "insert" mode with the regular Tab/Enter binding for completions - just like it is now, and because it's the way how completions work in general in ST.

Without indication, the user would never know if both the server and the given completion supports it and it would be a guessing game.

I agree, this should be indicated in some way. Otherwise it would be confusing why it doesn't work most of the time (probably most servers don't support it).

I was thinking of adding additional text in the st_detail field to distinguish insert/replace variants

If I understand it correctly, that would mean to duplicate the entries which use InsertReplaceEdit? I don't like this approach a lot, because multiple entries with the same label is the thing I dislike the most in autocompletions. It requires to read more info (from the annotation hint or details field) to pick the correct one, and it breaks the typing flow if you need to scroll down first in case the order is wrong (or if you want to see the details for the other entries). So I think I would prefer a keybinding for the "replace" mode, provided that it doesn't conflict with an existing default binding.

TerminalFi · 2022-06-21T04:51:50Z

This feature also looks really slick.

predragnikolic · 2022-06-22T19:10:09Z

I tried to mimic VS code behavior with this PR.

https://m.youtube.com/watch?v=QIze0qkugNc

In the PR description I haven't explained in detail why I implemented this the way I did
and that is simply that users switching from VS Code to ST could easily transfer their knowledge.

I wanted to create a second draft, that would address all the feedback from this draft and then I would let people decide between the 2 drafts.

Currently I do not think that I will do such a draft in the near time.

rchl · 2022-06-22T19:33:12Z

In case it makes it easier, I'm gonna state now that I'm fine with how it is now if you just update the setting description. I will look once again through the changes after it's rebased and addressed.

As far as disabling key bindings by default, I'm not sure... That would make the feature very niche. I think that it would be fine to have them enabled by default as long as the behavior fall backs to ST default if completion doesn't use InsertTextReplace. Not sure how easy that would be though...

When it comes to marking completion details with something to indicate that insert/replace is supported, this could be done without duplicating completions. The challenge would be to figure out what to show as it might be hard to find a short text/symbol that would be easily recognizable and meaningful to everyone.

…capability to context for shift+enter // When enabled, pressing tab will insert the best matching completion. // When disabled, tab will only trigger snippets or insert a tab. // Shift+tab can be used to insert an explicit tab when tab_completion is // enabled. "tab_completion": true,

predragnikolic · 2022-09-05T14:01:25Z

I removed shift+tab to not cause conflict with ST default behavior.

predragnikolic · 2022-09-05T14:48:39Z

I added the following text next to the More link in the autcomplete popup, to make it more noticeable.

But I am still unsure if I would like to clutter the autocomplete popup with this text...

Note: I had "completion_insert_mode": "replace" in LSP.sublimse-settings, and thus I saw "⇧ + ↵ to insert"

plugin/completion.py

Default.sublime-keymap

plugin/core/views.py

To allow the user to assign different keybindings, and to avoid showing a keybinding that is not true, "⇧ + ↵" section is removed from the popup.

predragnikolic · 2022-09-06T10:37:01Z

Should we rename
lsp_commit_completion_with_opposite_insert_mode to something more `short:

a) lsp_complete_with_opposite_insert_mode
b) lsp_insert_replace_complete

jwortmann · 2022-09-06T16:37:23Z

Default.sublime-keymap

+    {
+        "command": "lsp_commit_completion_with_opposite_insert_mode",
+        "keys": [
+            "shift+enter"


This also conflicts with the built-in key binding to insert a newline if the completion popup is visible.

It doesn't affect me personally, because I activated the "auto_complete_commit_on_tab" setting, but I would expect this to disturb many users if the built-in functionality suddenly isn't available anymore. It might be acceptable to provide key bindings which are direct replacements of built-in functionality, but with an override like this one here, I doubt that the LSP package would get accepted to Package Control if it had to re-apply for it.

Can you point to the conflicting keybinding?

There are of shift+enter keybindings in ST,
but none with a context when auto_complete_visible is visible.

{ "key": "auto_complete_visible", "operator": "equal", "operand": true },

So I did not see it conflicting.

The "context" only applys restrictions to a key binding. This one doesn't have a "context" restriction, so it is always activated. If the key binding from this PR gets added, it will be loaded after the default one, and therefore the default binding will be shadowed and won't work anymore.

Or do I have some misconception somewhere in my thoughts?

Nope, you are correct. In this case the lsp_commit_completion_with_opposite_insert_mode keybinding will have precedence. :)

But is that bad?
I mean other "shift+enter" ST keybinding also have contexts
and could also be seen as conflicting with this "shift+enter" insert \n keybinding, right?

In our case,
the lsp_commit_completion_with_opposite_insert_mode will conflict only if the auto complete popup is visible and the user is in a file supported by LSP.

Maybe this will make it more clear:
If you start ST in safe mode (all default settings), then you can commit a completion with Enter. But if you instead want to add a linebreak when the completion popup is visible, you can press Shift+Enter.
Now someone installs the LSP package and suddenly and without any warning Shift+Enter doesn't work anymore if a language server is running. Because it will then still commit the completion (with the opposite LSP insert mode), instead of the usual linebreak.

But is that bad? I mean other "shift+enter" ST keybinding also have contexts and could also be seen as conflicting with this "shift+enter" insert \n keybinding, right?

But the other ST key bindings are for when the command palette or find or replace panel has focus. So they are not relevant regarding auto completions.
If we start overriding built-in key bindings for different functionality, then I guess we could also hijack for example Ctrl+R for ... let's say e.g. LSP's formatting feature. And if users want to keep using ST's "Goto Symbol" they can just reassign another key binding for it :D
Or maybe let's reassing ST's key binding for toggle_comment to something different, because who needs that anyway... :)

In our case, the lsp_commit_completion_with_opposite_insert_mode will conflict only if the auto complete popup is visible and the user is in a file supported by LSP.

Well, but this is exactly the critical point. If the auto complete popup is not visible, then the built-in Shift+Enter binding is not useful anyway, because you can just press regular Enter without the Shift key to insert a linebreak.

You explained that well, thanks!
OK, I will remove the keybinding, and let the user configure it :)

predragnikolic · 2022-09-07T16:20:20Z

Closes #1413

rchl · 2022-09-07T16:38:27Z

Closes #1413

I think this needs to be added to the initial comment to actually have an effect on merging.

tests/test_completion.py

sublime-package.json

LSP.sublime-settings

sublime-package.json

LSP.sublime-settings

rchl

Given that it's a functionality that is not bound to a key binding by default and even when bound the discoverability is low (not clear when "replace" can be used), I find it a bit hard to justify.

But who am I judge, maybe someone will find it useful.
Let's merge after one more person gives an approval.

predragnikolic · 2022-09-09T09:11:00Z

How about "alt+enter" for the default keybinding?

rchl · 2022-09-10T19:46:20Z

Seems fine to me. Someone might still be using it to insert a line break but since shift+enter works it might not be a problem.

rchl · 2022-09-10T19:54:34Z

I'm still thinking how we could visualize if completion supports insert/replace...

It shouldn't mention specific key binding as we can't tell which one is defined.
It shouldn't take too much space.

Some ideas:

add [I/R] in the details field -- a bit too cryptic perhaps?
say "(supports replace)" if "insert" is the default mode and "(supports insert)" if "replace" is the default mode -- a bit long?
???

…the server supports insert replace

predragnikolic · 2022-09-12T08:14:38Z

How about the following?

Provide a default keybinding "alt+enter",
"Replace" or "Insert" will be show at the bottom of the AC popup. So that can give an indication that the server supports insert/replace.

rchl

I like the idea.

plugin/completion.py

rchl · 2022-09-12T08:31:05Z

plugin/core/views.py

+    text_edit = item.get('textEdit')
+    if text_edit and 'insert' in text_edit and 'replace' in text_edit:


Sorry, don't have time to check the spec now, but is it necessary to check for textEdit?

yes, there is a type error otherwise :)

- if text_edit and 'insert' in text_edit and 'replace' in text_edit: + if 'insert' in text_edit and 'replace' in text_edit:

docs/src/features.md

* main: Add insert-replace support for completions (#1809)

add insert replace support

944a1b6

rchl reviewed Jul 25, 2021

View reviewed changes

LSP.sublime-settings Outdated Show resolved Hide resolved

predragnikolic marked this pull request as draft September 29, 2021 20:54

predragnikolic added 3 commits September 5, 2022 15:00

Merge branch 'main' into completion-item-insert-replace-support

c7277ff

fix mypy

1122295

make it more obvious to the user if the server supports insert replace

727448e

rchl reviewed Sep 5, 2022

View reviewed changes

plugin/completion.py Outdated Show resolved Hide resolved

rchl reviewed Sep 5, 2022

View reviewed changes

Default.sublime-keymap Outdated Show resolved Hide resolved

plugin/core/views.py Outdated Show resolved Hide resolved

predragnikolic added 3 commits September 6, 2022 12:20

remove insert_replace_support_html in autocomplete popup

e66afbb

To allow the user to assign different keybindings, and to avoid showing a keybinding that is not true, "⇧ + ↵" section is removed from the popup.

remove trailing space

128cec4

rename shift_pressed to active

aed9449

predragnikolic marked this pull request as ready for review September 6, 2022 10:24

predragnikolic added 2 commits September 6, 2022 12:34

add to docs

b495e1f

sort order of keybindings

448f30f

jwortmann reviewed Sep 6, 2022

View reviewed changes

remove default keybindings

c6fac89

rchl reviewed Sep 7, 2022

View reviewed changes

tests/test_completion.py Outdated Show resolved Hide resolved

sublime-package.json Outdated Show resolved Hide resolved

use "used" instead of "enabled"

c88180f

rchl reviewed Sep 8, 2022

View reviewed changes

LSP.sublime-settings Outdated Show resolved Hide resolved

rchl reviewed Sep 8, 2022

View reviewed changes

sublime-package.json Outdated Show resolved Hide resolved

predragnikolic added 2 commits September 8, 2022 13:01

add backticks

288c281

sync setting descriptions

4aab370

rchl reviewed Sep 8, 2022

View reviewed changes

LSP.sublime-settings Outdated Show resolved Hide resolved

make it look similar to other settings description

3e7d116

rchl reviewed Sep 8, 2022

View reviewed changes

LSP.sublime-settings Show resolved Hide resolved

rchl approved these changes Sep 8, 2022

View reviewed changes

Merge branch 'main' into completion-item-insert-replace-support

ba88c2c

Merge branch 'main' into completion-item-insert-replace-support

9ba31f2

predragnikolic marked this pull request as draft September 11, 2022 12:56

add 'Insert' or 'Replace' command links to the autocomplete popup if …

537ca2b

…the server supports insert replace

predragnikolic added 2 commits September 12, 2022 10:18

document insert replace

af331f5

use different gif

0c17727

rchl reviewed Sep 12, 2022

View reviewed changes

fix lint

a9ce3c3

predragnikolic commented Sep 12, 2022

View reviewed changes

docs/src/features.md Outdated Show resolved Hide resolved

fix typo

73af44d

predragnikolic marked this pull request as ready for review September 12, 2022 09:03

cleanup

40515bc

rchl approved these changes Sep 12, 2022

View reviewed changes

rchl changed the title ~~Add insert replace support~~ Add insert-replace support for completions Sep 12, 2022

rchl merged commit 1be82e1 into main Sep 12, 2022

rchl deleted the completion-item-insert-replace-support branch September 12, 2022 19:30

rchl added a commit that referenced this pull request Sep 27, 2022

Merge branch 'main' into feat/diag-annotations

ea8186e

* main: Add insert-replace support for completions (#1809)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add insert-replace support for completions #1809

Add insert-replace support for completions #1809

predragnikolic commented Jul 25, 2021 •

edited

Loading

jwortmann commented Jul 25, 2021

rwols commented Jul 25, 2021

rchl commented Jul 25, 2021

jwortmann commented Jul 25, 2021

TerminalFi commented Jun 21, 2022

predragnikolic commented Jun 22, 2022

rchl commented Jun 22, 2022 •

edited

Loading

predragnikolic commented Sep 5, 2022

predragnikolic commented Sep 5, 2022

predragnikolic commented Sep 6, 2022

jwortmann Sep 6, 2022

predragnikolic Sep 6, 2022

jwortmann Sep 6, 2022

predragnikolic Sep 6, 2022

jwortmann Sep 6, 2022

predragnikolic Sep 6, 2022

predragnikolic Sep 6, 2022

predragnikolic commented Sep 7, 2022

rchl commented Sep 7, 2022

rchl left a comment

predragnikolic commented Sep 9, 2022 •

edited

Loading

rchl commented Sep 10, 2022

rchl commented Sep 10, 2022

predragnikolic commented Sep 12, 2022 •

edited

Loading

rchl left a comment

rchl Sep 12, 2022

predragnikolic Sep 12, 2022 •

edited

Loading

		text_edit = item.get('textEdit')
		if text_edit and 'insert' in text_edit and 'replace' in text_edit:

Add insert-replace support for completions #1809

Add insert-replace support for completions #1809

Conversation

predragnikolic commented Jul 25, 2021 • edited Loading

jwortmann commented Jul 25, 2021

rwols commented Jul 25, 2021

rchl commented Jul 25, 2021

jwortmann commented Jul 25, 2021

TerminalFi commented Jun 21, 2022

predragnikolic commented Jun 22, 2022

rchl commented Jun 22, 2022 • edited Loading

predragnikolic commented Sep 5, 2022

predragnikolic commented Sep 5, 2022

predragnikolic commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

predragnikolic commented Sep 7, 2022

rchl commented Sep 7, 2022

rchl left a comment

Choose a reason for hiding this comment

predragnikolic commented Sep 9, 2022 • edited Loading

rchl commented Sep 10, 2022

rchl commented Sep 10, 2022

predragnikolic commented Sep 12, 2022 • edited Loading

rchl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

predragnikolic Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

predragnikolic commented Jul 25, 2021 •

edited

Loading

rchl commented Jun 22, 2022 •

edited

Loading

predragnikolic commented Sep 9, 2022 •

edited

Loading

predragnikolic commented Sep 12, 2022 •

edited

Loading

predragnikolic Sep 12, 2022 •

edited

Loading