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

All symbols in completion #2644

Merged
merged 62 commits into from
Mar 27, 2017

Conversation

vasily-kirichenko
Copy link
Contributor

image

@vasily-kirichenko
Copy link
Contributor Author

I'm stuck on adding open statement. The problem is that this code https://github.com/Microsoft/visualfsharp/pull/2644/files#diff-d788d4b29ac0cab7497b91c678517720R319 does nothing:

let! sourceText = document.GetTextAsync(cancellationToken)
let text = sourceText.WithChanges(TextChange(TextSpan(0, ns.Length), ns))
UIThread.Run(Action (fun () ->
    document.Project.Solution.Workspace.TryApplyChanges(document.Project.Solution.WithDocumentText(document.Id, text)) |> ignore))

@Pilchie @cartermp please, help. I need to modify document inside CompletionProvider.GetChangeAsync. Maybe it's a wrong direction at all. So the initial task is to add open statement at a known position when user selected (committed) an item in completion list.

@cloudRoutine
Copy link
Contributor

This seems similar to the issues I've been having in #2603

Applying changes to the VisualStudioWorkspaceImpl has consistently thwarted me, as well as trying to connect its navigation services to internally stored subworkspaces.

@majocha
Copy link
Contributor

majocha commented Mar 17, 2017

I suspect the change you return is then applied to the old version of the document, the one you take as argument. Maybe Returning everything as a change would do.
Strangely, it is now obsolete (?):
http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/Completion/CompletionChange.cs,57

@Pilchie
Copy link
Member

Pilchie commented Mar 17, 2017

Tagging @CyrusNajmabadi and @rchande

@CyrusNajmabadi
Copy link
Member

Just return a COmpletionChange with a TextChange that manipulates the entire document and sets the caret position to where you want it to be. You effective get full control of what happens to the document here.

@vasily-kirichenko
Copy link
Contributor Author

@CyrusNajmabadi But I can return single TextChange, but I want two: adjusted completion item text (for some symbols we add double backticks around name) and open ....

@vasily-kirichenko
Copy link
Contributor Author

Ah, I see, CompletionItem can contains many changes. Thanks!

@CyrusNajmabadi
Copy link
Member

That all can be wrapped into a single text change. :)

/// Entity name parts with removed module suffixes (Ns.M1Module.M2Module.M3.entity -> Ns.M1.M2.M3.entity)
/// and replaced compiled names with display names (FSharpEntity.DisplayName, FSharpValueOrFucntion.DisplayName).
/// Note: *all* parts are cleaned, not the last one.
type internal AssymblySymbol =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AssemblySymbol?

@vasily-kirichenko
Copy link
Contributor Author

@majocha thanks! fixed

…tion

# Conflicts:
#	src/fsharp/vs/ServiceUntypedParse.fs
#	vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs
@vasily-kirichenko
Copy link
Contributor Author

I added keywords to completion because without them typing things like match x with<Enter> caused unrelated item to complete. Now it works quite nicely:

1

@dsyme
Copy link
Contributor

dsyme commented Mar 27, 2017

@cartermp Are you happy with the extra entries in the autocomplete lists being on by default?

I had generally thought we would want to be greatly reducing the number of entries (e.g. by removing *Attribute types by default) and that F# generally has a problem that autocomplete lists are too long and information-overloaded for beginners. Equally I understand the value of showing more possibilities and that it can really help people find stuff. But basically I don't have a firm decision either way, and am happy with either mechanism being available, it's just a question of defaults

So @cartermp it would be great if you could check this and help make a call on what you feel is the right default? thanks

@cartermp
Copy link
Contributor

cartermp commented Mar 27, 2017

@dsyme I'll pull it down today and try it out again. I tried it earlier and liked it, though @vasily-kirichenko was still in the midst of making more improvements. I approve of the extra keywords in the abstract, though.

Here's how I look at it:

  1. Lack of autocomplete for F# in the past (or at least recent past) means we have a bit of a green field to play on. I know that C# has to be super conservative with any improvements they make, because they have so many people who have developed muscle memory over how autocomplete behaves. We don't have this problem. Surely, there will be a few people who prefer no autocomplete, a more spartan editor, and just using ctrl+space from time-to-time. Use Nuget for VSSDK Packages and upgrade to stable package versions #2698 is the building block to having a good settings menu for these folks to turn off features, and it'll just be a point-in-time problem that they won't be able to turn stuff like this off.

  2. Some keywords throw off beginners. I've personally typed inherits multiple times. Having autocomplete take care of some of that is great.

  3. We have to tread carefully with let, do, yield, and their CE counterparts. VS for mac currently offers completion for let! when not in the scope of a CE, for example, which is annoying. This is something we can solve incrementally, though.

  4. Our nightly process allows for rapid iteration on the PR -> merge -> play -> feedback loop. Though we need to balance this with the VS ship schedule, we can already have hundreds of people trying out new behavior like this and giving feedback. People tend to be pretty noisy when something is a bad experience, so we'll probably hear about it via twitter or VS Feedback. Though changes like this will ultimately be thrust upon the bulk of the F# audience with a major VS update or release, we have more people to give us feedback before that happens than before. That makes me more open to experimentation.

@dsyme
Copy link
Contributor

dsyme commented Mar 27, 2017

@cartermp OK, thanks

  1. Our nightly process allows for rapid iteration on the PR -> merge -> play -> feedback loop. Though we need to balance this with the VS ship schedule, we can already have hundreds of people trying out new behavior like this and giving feedback. People tend to be pretty noisy when something is a bad experience, so we'll probably hear about it via twitter or VS Feedback.

One specific concern with this is that it only gets feedback from advanced F# programmers. For intellisense simplicity I'm most worried about total newcomers and beginners.

I know that one problem people have when coming to F# and the Visual F# Tools is that the information offered is totally overwhelming. Is it an technical option to have this turned off by default, or turned off at all? The whole point of those filters on the autocomplete dialog was to reduce the amount of information offered by default.

As a working principle, I think we should only be integrating new features that increase the information offered if we also have ways to tune the default settings (so we can later choose to have the information off-by-default, without needing to do technical work to add UI etc.). Otherwise we've found in the past that we just get more and more information offered (which can be good for some users for sure) but never have easy ways to simplify.

@Pilchie
Copy link
Member

Pilchie commented Mar 27, 2017

Another option is to show the entries dynamically based on matches. The basic idea would be calculate the full list, but treat it as already filtered to the set of items that are already in scope. Then, if the user types something that doesn't match an item in the list, but does match something that had been filtered, you expand to the full set.

Not sure how camel case/substring/etc matches might cause issues with this approach though (likely by preventing a switch to the full list).

Finally - another thought is to add a filter button to the bottom tray to control showing these items. I believe that is what C++ ended up doing for their "predictive IntelliSense" feature.

@vasily-kirichenko
Copy link
Contributor Author

I think people should just try it. I've used it for some time now and find it great. However, a setting should be added (after merging that @cloudRoutine PR which enables xaml based settings pages).

@cartermp
Copy link
Contributor

@dsyme I think it will turn out well. We have the tools to filter larger sets of information intelligently with a PR like this. @Pilchie's idea is also worth exploring. I also think that in the coming months, as settings are added and features are adjusted to be controlled by those settings, we'll be in a very good position. I'm conscious of the fact that nightlies have a bit of a bias towards power users, but I have faith that they're aware of problems newcomers have and can offer opinions which are helpful.

@forki
Copy link
Contributor

forki commented Mar 27, 2017

My2cents: we should definitely merge this and then improve on it iteratively (is that a word?).

@dsyme
Copy link
Contributor

dsyme commented Mar 27, 2017

@vasily-kirichenko @Pilchie @cartermp Super, thanks all, good plan

@KevinRansom
Copy link
Member

@vasily-kirichenko

thanks this is great work

Kevin

@KevinRansom KevinRansom merged commit e534e65 into dotnet:master Mar 27, 2017
@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2017

@vasily-kirichenko @forki Yes agreed. It is great to have this.

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* wip

* port Roslyn's C# completion triggering logic as is

* fixed: completion do not trigger at all

* fix position

* fix a test

* do not provide completion on certain places

* fix muting completion on Named(Wild) pat in binding

* do not provide completion on ctor / lambda parameter names + tests

* fix binding traversing in completion

* fix tests

* fix the last failing test

* wip

* wip

* fix compilation

* the list kind of works

* try to add open (fake)

* insert fake open statement

* works for dummy ns insert position

* it works (draft)

* wip

* pass proper namespace to open

* do not suggest operators

prefer already resolvable items

* fix compilation

* fix

* filter out unqualified types if there are same normal ones (UnqualifiedType (tcRef1 :: _) is equal to TType.Type_app (_, tcRef1) is tcRef1 = tcRef2)

* fix compilation

* do not open Microsoft.FSharp.xxx namespaces

add mandatory qualifier, which makes completion works for symbols like `Printf.kprintf`

* fix bugs

* wip

* wip

* Revert "wip"

This reverts commit fe3247e.

* fixing bugs with RCA modules

* bug fixing

* fix filter text

* optimize completion list sorting

* code cleanup and doc

* defer getting all entities (do not get them after dot completion at all)

* fix compilation

* remove dead code

* rename RawEntity to AssemblySymbol and use FSharpSymbol instead of Item in it

* do not call getAllSymbols unless necessary (now for real)

* AssymblySymbol -> AssemblySymbol

* add completion for keywords

* order completion items by IsResolved property

* add CompletionContext.OpenDeclaration, remove IsAtOpenDeclaration ad-hoc filtering

do not show keywords at open declaration position
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

Successfully merging this pull request may close these issues.