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

[VS Code] JS/TS always selects the first suggestion #53

Closed
svipas opened this issue Jan 25, 2019 · 24 comments
Closed

[VS Code] JS/TS always selects the first suggestion #53

svipas opened this issue Jan 25, 2019 · 24 comments
Labels
enhancement New feature or request
Milestone

Comments

@svipas
Copy link

svipas commented Jan 25, 2019

Product and Version [VS/VSCode]: VS Code
OS Version [macOS/Windows]: macOS 10.14.3
IntelliCode Extension Version: 1.1.2
Target Platform or Language [e.g. Node.js]: JavaScript & TypeScript

Steps to Reproduce / Scenario:

  1. Make sure you set recentlyUsed in editor.suggestSelection which is default in VS Code.
  2. Disable this extension and try to use auto-complete and select second or any item from the list you want except the first one.
  3. Again use auto-complete to render suggestions list on the same object and it should automatically point to your recently used suggestion.
  4. Enable this extension and try to repeat 2 and 3 step. As you can see this extension disables it and acts like it's set to first in editor.suggestSelection.

I expect this feature to work properly with this extension enabled.

recentlyUsed: Select recent suggestions unless further typing selects one, e.g. `console.| -> console.log` because `log` has been completed recently.

Thank you!

@svipas
Copy link
Author

svipas commented Jan 25, 2019

Also, saw if I add . (dot) in the string I will get auto-complete which is not correct especially with such context:

screenshot 2019-01-25 at 11 06 49

@jkeech
Copy link
Collaborator

jkeech commented Jan 25, 2019

I believe the first issue is by design, since the IntelliCode extension moves the recommended items to the top of the completion list, and by default it selects the most-likely recommendation. In many cases (but not all), the most likely recommendation does match the recently used member.

I can also reproduce the issue with IntelliSense popping up inside the string literal, but only when coming back to it after completing the statement. When typing it out initially, IntelliSense does not pop up. Thanks for reporting this bug.

js2

/cc @minestarks

@svipas
Copy link
Author

svipas commented Jan 28, 2019

No problem. I actually like recentlyUsed feature because it saves some time especially in cases like this when you're repeating function calls, etc. and I'm really missing it.

As I understand this extension suggests you the most used ones instead of recently.

Also, sometimes I get this:

screenshot 2019-01-28 at 08 24 00

As you can see * suggestions is not at the top.

@minestarks
Copy link
Collaborator

minestarks commented Jan 29, 2019

@jkeech who should "win" in this case? IntelliCode or VS Code? They're both doing their best to push most relevant suggestions to the top 😄

(Even if the answer is ideally IntelliCode, I'm actually not sure if a plugin like IntelliCode can override the final sorting done by VS Code.)

@svipben Do you have an example for the behavior in your original description, i.e. where IntelliCode appears to override/disable the recentlyUsed option?

@jkeech
Copy link
Collaborator

jkeech commented Jan 29, 2019

@minestarks, I would expect IntelliCode suggestions to show up at the top if the extension is enabled and it offers suggestions. Do you know why the IntelliCode suggestions are normally sorted at the top, but in this case, VS Code pushed two suggestions higher?

@svipben, the extension recommends the most likely (popular) suggestion based on the current context. Since the context continually changes as you type, the suggestions can adapt. So for instance, if you are repeating the same member multiple times, and if that pattern is common among other codebases, then IntelliCode would also recommend that pattern in your case.

For example, if you start typing in a function without much context, IntelliCode might recommend one thing, but you choose a different path. It then adapts and recommends the next likely thing based on what you have done so far, even though it was different than the initial suggestion. It doesn't necessarily recommend the exact same members every time. Here's an example showing what I mean. Notice how the recommendations change as I go:

intellicode-adapttocontext

@markw-t
Copy link
Contributor

markw-t commented Jan 29, 2019

@minestarks +1 to what @jkeech says: keeping the starred IntelliCode suggestions always at the top means they are a clear group to be picked from, then the regular list cuts in. In this way it's easy to skip straight to the list if IC doesn't have what you want, and the regular list feels like it always did.

@svipas
Copy link
Author

svipas commented Jan 29, 2019

Problem is, it doesn't even respect recentlyUsedByPrefix, it would be way easier to get what you want instead of typing more.

What I mean is

recentlyUsedByPrefix:

Select suggestions based on previous prefixes that have completed those suggestions, e.g. co -> console and con -> const.

So if I type 'text'. and select bold from the list, next time when I type b I will get bold (expected behavior) instead of first suggestion big (which is current behavior). So I believe there's problem of totally disabling VS Code editor.suggestSelection which is bad because no matter what always first suggestion is selected.

Expected (recentlyUsedByPrefix)
expected

Current (recentlyUsedByPrefix)
current


If this recentlyUsedByPrefix is fixed it would be 100% better than it's now, and I kinda think it would be even better than recentlyUsed since you get benefits of this extension and VS Code recentlyUsedByPrefix which means all suggestions will be rendered like it was with IntelliCode at the top and if you type letter it will return recent one. Win-win?

@svipas
Copy link
Author

svipas commented Jan 29, 2019

Sorry for creating 3 issues in one. I separated string literal bug and order bug in two different issues.

@minestarks
Copy link
Collaborator

@svipben oh thanks, that's really helpful. So this one is just tracking the fact that IntelliCode appears to disable/ignore the editor.suggestSelection setting correct?

@svipas
Copy link
Author

svipas commented Jan 29, 2019

@minestarks Yeah, it looks like it doesn't work at all with IntelliCode. But I think you should carefully think of recentlyUsed because second time after you already selected something suggestions list will point to the recent one after typing . (dot) instead of showing suggestions list with IntelliCode suggestions at the top (like it's right now) and I kinda think it's correct behavior, BUT recentlyUsedByPrefix should work as expected which I mentioned above.

So long story short:
recentlyUsed: disable and keep like it's, select always first suggestion no matter what because we care about order and view of our IntelliCode suggestion list
recentlyUsedByPrefix: fix it as I mentioned above
first: works as supposed to 😄

@vivlimmsft
Copy link
Collaborator

Yes, the IntelliCode extension declares configurationDefaults for each language we support, which set "editor.suggestSelection": "first".

You can override the setting by specifying a language-specific override setting for editor.suggestSelection. Could you please try the below, reload vscode, and try some suggestions?

    "[javascript]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    },
    "[typescript]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    },
    "[java]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    },
    "[python]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    }

@svipas
Copy link
Author

svipas commented Jan 30, 2019

@vivlimmsft you're right, with this configuration it works. I understand why you kept first here, but maybe it's better to remove configurationDefaults and create a config like this:

{
  "vsintellicode.suggestSelection": {
    "type": "string",
    "enum": [
      "first",
      "recentlyUsed",
      "recentlyUsedByPrefix"
    ],
    "default": "first",
    "description": "Controls how suggestions are pre-selected when showing the suggest list."
  }
}

Then user can decide which one is better for him. And as you can see default will be still first.

@svipas
Copy link
Author

svipas commented Feb 7, 2019

Guys any news on this? I'm not pushing you though, but I'm interested if you're thinking about my suggestion to introduce that setting mentioned above. Thanks!

/cc @vivlimmsft @minestarks @markw-t @jkeech

@jkeech
Copy link
Collaborator

jkeech commented Feb 7, 2019

Hey @svipben, yes, we are planning on improving this scenario soon. We discussed the options offline the other day and came up with this solution:

  • Continue to provide the per-language "first" defaults we have
  • When the IntelliCode extension is activated, check the global value of editor.suggestSelection. If the value is anything other than first or recentlyUsed (today that is just recentlyUsedByPrefix), show a notification that has a button which you can click to add all language-specific overrides for the chosen editor.suggestSelection to their settings.

The reason we have to continue using the editor setting instead of a special vsintellicode setting is because the behavior change is controlled by the editor itself -- it's not something that IntelliCode can alter when providing completions. It's just that some combinations of that setting are incompatible with IntelliCode, so we try to fix that issue. But we can detect if you had a different (compatible) setting in place and try to make sure the IntelliCode defaults honor that setting and give you the choice on how to proceed.

The end state is that if you had an incompatible setting, we automatically change it to be compatible. If you had a different compatible setting, we will help you configure IntelliCode to keep that setting in place.


A different way we might implement this is to remove all of the default settings in IntelliCode, and then at activation, detect if you are using an incompatible setting, and if so, only then set the specific override to a compatible setting. This would avoid the separate notification when you are on an existing compatible setting that is different than "first".

@svipas
Copy link
Author

svipas commented Feb 9, 2019

@jkeech Thanks for response, so in all scenarios it would add these in my settings?

    "[javascript]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    },
    "[typescript]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    },
    "[java]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    },
    "[python]": {
        "editor.suggestSelection": "recentlyUsedByPrefix"
    }

@jkeech
Copy link
Collaborator

jkeech commented Feb 9, 2019

In the last scenario, it wouldn’t override your settings at all because you are already using a compatible setting at the global level.

@svipas
Copy link
Author

svipas commented Feb 9, 2019

I like the last scenario then. It checks if it's recentlyUsedByPrefix or first if yes it doesn't do anything, but if it's a recentlyUsed it will change it to a first by overriding it like mentioned above. And as far as I understand this will be done at the startup of VS Code.

@svipas
Copy link
Author

svipas commented Feb 10, 2019

Also, you should consider this situation:

"editor.tabCompletion": "on",
"editor.quickSuggestions": false,

In this situation, it should respect whatever editor.suggestSelection you have because you don't see any suggestions and you're completing them by pressing tab. So in this situation, even recentlyUsed is possible. Currently, I'm using it and I guess I will stick with it, because suggestion popup is too noisy and distracting, if I really don't know something I manually invoke suggestions. What do you think about this? With the settings mentioned above, I can use recentlyUsed since I can't see suggestions list only if I invoke it manually?

/cc @vivlimmsft @minestarks @markw-t @jkeech

@jkeech
Copy link
Collaborator

jkeech commented Feb 11, 2019

I think the general principle should be that we do not alter when the completion list shows up, but when it does show up, we ensure that the IntelliCode suggestions are the top/default options if you have the IntelliCode extension enabled.

In the scenario where you want different completion list sorting behavior that is conditioned on whether the list is visible or not -- I don't think this is possible without a VS code editor change. The sorting is something that the editor itself is doing, and as far as I know, it's not configurable based on when the completion list is being invoked, other than the per-language overrides. I think you have to pick one setting or the other, and it applies equally whether or not the list is visible. So in this case, it would still have to be first or recentlyUsedByPrefix so that if you manually invoke the completions, the IntelliCode suggestions are displayed properly.

Even if it was possible to conditionally change the sorting based on completion list visibility, I'm not sure that it's the best approach. The overall idea is that, on average, the top IntelliCode suggestion should be more accurate than the top recentlyUsed suggestion. So even if you are blindly selecting the first option without really looking, the IntelliCode suggestion should be correct more often, which would make users more productive.

@svipas
Copy link
Author

svipas commented Feb 11, 2019

@jkeech Sounds good, waiting for an update then! Thank you 🔥

@jkeech
Copy link
Collaborator

jkeech commented Mar 6, 2019

We found out that extensions cannot programmatically set language-specific setting overrides using the VS Code APIs, so we are going to change the implementation slightly. The end result should be the same for you though -- if you have "editor.suggestSelection": "recentlyUsedByPrefix" set, IntelliCode will not interfere with that.

Here's the new logic, which should be available soon in the next update of the extension (1.1.4):

  1. The IntelliCode extension will provide a default value for editor.suggestSelection, but not the language-specific overrides that it currently provides in 1.1.3. We will continue to use first as the default value, but if we hear enough feedback from users that they would generally prefer recentlyUsedByPrefix, we can always update to that as the default.
"configurationDefaults": {
    "editor.suggestSelection": "first"
},
  1. Since this is only a default, if you have explicitly set your own value for editor.suggestSelection, it will win. The order of precedence is user-defined settings (at the workspace folder, workspace, or global level), extension-provided defaults, VS Code-provided defaults.
  2. With this logic, a user could explictly set a value that is incompatible with IntelliCode. During extension activation, we will check the current setting value and print a warning to the output window if the setting that is being used will interfere with the expected behavior of IntelliCode, but we will not automatically change the setting to a different value. This gives users full control. We try to provide good defaults that work well with IntelliCode, but users can always override it if they want.

@jkeech jkeech added this to the 1.1.4 milestone Mar 6, 2019
@jkeech jkeech added the enhancement New feature or request label Mar 6, 2019
@vivlimmsft
Copy link
Collaborator

We changed the implementation from what @jkeech mentioned above, because we found that configurationDefaults declared in package.json can only be applied to specific languages.

The new implementation, which is in 1.1.4, removes all defaults from our package.json and instead will apply 'editor.suggestSelection': 'first' as needed, under the following conditions:

  • If editor.SuggestSelection has not been manually configured to be recentlyUsed, and is getting that value by default, we will write 'editor.suggestSelection': 'first' without asking. This replicates previous behavior, with the side effect of meaning that you cannot use recentlyUsed with any language unless you use a language-specific setting to do so.
  • If editor.SuggestSelection was manually configured by you to be recentlyUsed, we will show a notification asking if you'd like it to be changed to first automatically. You can also decline, in which case we will not prompt again.

We introduced a new setting, vsintellicode.modify.editor.suggestSelection, which keeps track of:

  • whether you have declined to have editor.suggestSelection updated by IntelliCode.
  • whether we have updated editor.suggestSelection on your behalf, and if so, which of the above conditions was true at the time. We're keeping track of those conditions in case we need to change this behavior in the future.

To opt out of this behavior, you can add "vsintellicode.modify.editor.suggestSelection": "disabled", to your settings.json.

If you still want to use recentlyUsed with specific languages, you can set language-specific settings. For example:

    "[csharp]": {
        "editor.suggestSelection": "recentlyUsed"
    },

@svipas
Copy link
Author

svipas commented Mar 7, 2019

Thanks, guys for the update. I'm really happy with these changes and I understand the reason. Good job!

@jkeech
Copy link
Collaborator

jkeech commented Mar 7, 2019

Version 1.1.4 has been published to the marketplace. Please give it a shot and let us know what you think. Thanks!

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

No branches or pull requests

5 participants