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

Improve UX on autocomplete and signature info #251

Closed
msaraiva opened this issue May 18, 2020 · 10 comments
Closed

Improve UX on autocomplete and signature info #251

msaraiva opened this issue May 18, 2020 · 10 comments

Comments

@msaraiva
Copy link
Collaborator

Following #241, I've been working on some other improvements regarding UX. Most of them have been in my TODO list since I've moved from Atom to VS Code. Before I send a PR, I'd like to get some feedback in order to avoid wasting my time in something that might not please everyone. So here it goes.

1. Improve readability of signature info

Currently, the documentation summary and the spec showed are not recognized as markdown and elixir code respectively. Example:

image

Suggestion:

  • recognize them as markdown/code
  • add an empty line between them
  • invert the order so the summary comes before the spec
  • format the spec using the elixir formatter

Example:

image

2. Improve readability of auto-complete info

Currently, the information showed for auto-complete is too noisy. Example:

image

The most important thing to see should be the signature. But most of them are clipped so we need to look at the spec, which is much more verbose, to figure out the arguments. Also, having the signature in the list makes it harder to read the function name.

Suggestion:

  • show only the function name/arity on the list
  • add information about the type (function or macro) and the module where the function/macro is defined
  • show the signature in the side box (instead of in the list)
  • add the formatted spec at the end (as suggested for the signature info)

Example:

image

Here's another example with a larger spec:

Before:

image

After:

image

Although the user might eventually need to scroll down to see the formatted spec, the information will be much more readable as we can see here:

image

3. Do not add arguments to the snippet when auto-completing

One of the main reasons I implemented the signature help feature in ElixirSense was to be able to get rid of this behaviour as 99% of the time we need to delete all the arguments suggested since the suggestions are just the name of the arguments, not the name of the variables you have in the scope.

Example:

image

As you can see, there's no variable enumerable and I might not even need the second argument. So I'll have to delete both of them. That's just extra unnecessary work.

Suggestion:

  • Do not add arguments to the snippet
  • trigger the signature help right after confirming the suggestion

Here's an example from a Java extension:

Autocomplete

And here is VS Code showing our signature info after completion:

image

4. Remove keyword completion

I think we need to start thinking about removing some of the suggestions as they mostly just add noise to the list. One example is the keyword completions. As the comment in the source code states, this is really not useful and it interferes with the auto-indentation for some cases.

Note: Another one we should probably remove in the future is the list of operators or maybe only keeping them when explicitly using Kernel..

@axelson and @lukaszsamson I'd love to hear your thoughts about those suggestions.

Cheers.

@axelson
Copy link
Member

axelson commented May 18, 2020

Wow, there's a bunch of great suggestions here!

  1. Improve readability of signature info

I think this these suggestions are great. I was a little hesitant about swapping the summary and the @spec, but since we're only showing the summary it makes sense.

  1. Improve readability of auto-complete info

I think changing the initially displayed panel makes sense given the very small width of the autocomplete window (although I wish it would be improved, and there is an upstream issue for that microsoft/vscode#29757). I'm not convinced on the specific format that you've chosen (in particular the function (Enum) and the | to the left of the signature.

I think my ideal would be to display macro vs function in the initial autocomplete window (but ideally in a slightly subdued color). I do think it is important to show the specific module (before any aliases) that the function/macro is coming from (especially important when it was imported!) but I'm not sure about the exact format that should take yet. And I do like the summary and formatted @spec, and I like that they're consistent with the formatting displayed in the signature info.

I think we should further discuss the specific changes for this in a new issue

  1. Do not add arguments to the snippet when auto-completing

I am definitely agreed on removing any optional arguments from being inserted, but I'm not as sure about non-optional arguments. Part of the reason for that is that not all clients will have as nice signature info help as VSCode, perhaps this could be made configurable via #226 (which I almost have a PR ready for).

  1. Remove keyword completion

Pretty much agreed, although I'd want to play with it before removing them completely. From the list I'm pretty sure that I'd want to keep do to keep auto insertion of the matching end.

@asummers
Copy link
Contributor

Re: Note: Another one we should probably remove in the future is the list of operators or maybe only keeping them when explicitly using Kernel..

ex_doc did something similar recently which stopped special casing the operators

https://hexdocs.pm/ex_doc/changelog.html#v0-22-0-2020-05-11

[HTML+EPUB] Remove auto-linking for local calls to Kernel & Kernel.SpecialForms, use fully qualified calls instead. (e.g. replace `==/2` with `Kernel.==/2`.)

@jayjun
Copy link
Contributor

jayjun commented May 18, 2020

Great work!

For reference, I still think atom-elixir’s autocomplete is the best I’ve used (thank you too).

atom-elixir

Not sure if reproducible in VS Code. Here’s why it works for me,

  1. Generous padding around each suggestion. Less choices.
  2. Faded module hint on the right. Useful when many imports.
  3. Long horizontal boxes. Should reduce (even eliminate) overflow and typespecs don’t get contorted.
  4. Suggestions and documentation have different backgrounds.

Horizontal boxes seem to be one CSS property away. microsoft/vscode#29757 (comment)

@msaraiva
Copy link
Collaborator Author

I'm not convinced on the specific format that you've chosen (in particular the function (Enum) and the | to the left of the signature... I think my ideal would be to display macro vs function in the initial autocomplete window (but ideally in a slightly subdued color)

Atom used to allow that but VS code, unfortunately, doesn't. As far as I know, It will automatically add whatever comes first only when the side box is not showing. That's why I placed that info at the beginning.

image

Regarding the | to left of the signature, I'm not a big fan either. it's just a workaround since there's no way to customize the CSS style of that damn box (note: Atom allowed us to do that too). I have tried to move the signature to inside the markdown part and use a blockquote but VS Code's markdown format is weird. It looks like this:

image

It would be perfect if the format was more like the Elixir docs:

image

Unfortunately, it's not. Open for suggestions here.

I am definitely agreed on removing any optional arguments from being inserted, but I'm not as sure about non-optional arguments. Part of the reason for that is that not all clients will have as nice signature info help as VSCode

We can create a configuration then. We could even have 3 options if you like:

  • Only function name
  • Function name + arguments
  • Function name + trigger signature help (should this be the default for VS Code?)

@msaraiva
Copy link
Collaborator Author

@jayjun yeah, that's gorgeous! I'm really proud to have achieved that in Atom. Unfortunately, VS Code is a pain when it comes to customizing its styles. Currently, we cannot do anything about the size of the windows, the colours nor the font sizes.

BTW, is that Atom plugin still working? I haven't done any improvement since I moved to VS Code. Has anyone forked and kept it alive?

@lukaszsamson
Copy link
Collaborator

Great work @msaraiva. Here's my small comments

  1. Improve readability of signature info

👍

. format the spec using the elixir formatter

It may be not possible/easy if the spec is taken from the docs. For erlang modules the spec is going to be in erlang. It's worth checking how ex_doc handles it.

  1. Improve readability of auto-complete info

👍

. show only the function name/arity on the list

It's already happening for 0-arity functions

  1. Do not add arguments to the snippet when auto-completing

I'm totally in favour. It also was on my TODO list

  1. Remove keyword completion

I'm not sure about that. I find if/else/end, with/do/end, case/do/end etc. pretty useful. It's a pitty LS API makes it difficult to get the formatting right.

@jayjun
Copy link
Contributor

jayjun commented May 18, 2020

@msaraiva I’m the guy who ported it to JavaScript if you remember 😄 msaraiva/atom-elixir#73. I did publish a fork but Atom wasn’t too happy it was the “same” package. Regardless, I moved to VS Code shortly after.

@msaraiva
Copy link
Collaborator Author

Hi @lukaszsamson!

Thanks for the feedback.

It may be not possible/easy if the spec is taken from the docs. For erlang modules the spec is going to be in erlang. It's worth checking how ex_doc handles it.

Nice reminder! I believe we can solve this by checking the type of the module (erlang vs elixir) where the function was originally defined. We already send that information from ElixirSense.

I'm not sure about that. I find if/else/end, with/do/end, case/do/end etc. pretty useful. It's a pitty LS API makes it difficult to get the formatting right.

Personally, aside from the do...end auto-insertion, I believe I never used any of the other suggestions. The if/else/end, with/do/end, case/do/end you've mentioned are not part of that list. They are separate snippets. The list I'm talking about is this one:

  @keywords %{
    "end" => "end",
    "do" => "do\n\t$0\nend",
    "true" => "true",
    "false" => "false",
    "nil" => "nil",
    "when" => "when",
    "else" => "else\n\t$0",
    "rescue" => "rescue\n\t$0",
    "catch" => "catch\n\t$0",
    "after" => "after\n\t$0"
  }

which is always added to the suggestions list except when you already have a non-matching prefix.

As you can see, they don't do anything special like showing docs or specs. Besides, what's the point in auto-completing such basic short keywords like end, nil, true or false?

As for else, rescue, catch and after, the only thing they do is to move the cursor to the next line + indentation, which I believe most editors already do.

So my suggestion would be to keep only do so we can leave the end auto-insertion behaviour.

The suggested changes are the first batch of many others that I have in mind to make the list of suggestions more useful to the user. The other, much more important change I'm working on right now, is refactoring ElixirSense.Providers.Suggestion to make the API more consistent so we can have an easier way to add suggestions and, most importantly, eliminate invalid ones according to the context.

@msaraiva
Copy link
Collaborator Author

@axelson @lukaszsamson while developing elixir-lsp/elixir_sense#99, I had the opportunity to play with the auto-complete behaviour in VS Code and could confirm that the do...end auto-complete stuff is done automatically by the editor, independently of the suggestion list. Can you please confirm that this is the behaviour in your editors? If so, I think we should remove the suggestions for keywords without hesitation or, in the worst case, add a configuration to enable/disable it.

axelson added a commit that referenced this issue May 31, 2020
* Remove keywords from completions

They were being returned regardless of context which made them very noisy.

Related:
* elixir-lsp/elixir_sense#99
* #251

* Add changelog entry

* Return isIncomplete = true for completions

* Fix tests and add a couple.
@axelson
Copy link
Member

axelson commented Aug 4, 2020

This was addressed in #259. Any further improvements can be discussed in a new issue.

@axelson axelson closed this as completed Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants