Skip to content

Conversation

ijklam
Copy link
Contributor

@ijklam ijklam commented May 28, 2024

Description

Extracted from #17252

Fixes part 2 of #14375

  1. For entity in class or module, instead of trying to open the class/module, it will now include the class/module name in DisplayName, so it won't try to open a class now. For modules, it will only try to open the nearest parent namespace/module.

GIF 2024-5-29 0-38-13

  1. It won't insert duplicated open now. And completions can now also be filter by class/module name.

GIF 2024-5-29 0-41-45

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated:

@ijklam ijklam requested a review from a team as a code owner May 28, 2024 16:45
Copy link
Contributor

github-actions bot commented May 28, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.400.md
vsintegration/src docs/release-notes/.VisualStudio/17.11.md

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Hi @Tangent-90 - thanks for another good contribution!

I get what the issue is about, yet for some reason, I cannot reproduce the exact problem described in the bug. This PR has a bit different setting though - so could I ask you to also record a small "before" gif, how things currently look on your machine?

Ideally we'd have some compiler service tests here as well - have you looked around there if there is something we can build on?

@ijklam
Copy link
Contributor Author

ijklam commented May 29, 2024

I get what the issue is about, yet for some reason, I cannot reproduce the exact problem described in the bug. This PR has a bit different setting though - so could I ask you to also record a small "before" gif, how things currently look on your machine?

I have no other VS installation, so I record this GIF in VSCode (but it is the same bug).

GIF 2024-5-29 19-58-45

You can see that the class System.BitConverter or System.Collections.Immutable.ImmutableArray is opened, and it may insert a duplicated one though there is a same open. The former one is a bug in FCS, and the latter one is in editor extension(FSharp.Editor or fsautocomplete)

@ijklam
Copy link
Contributor Author

ijklam commented May 29, 2024

maybe we can add test to here

@psfinaki
Copy link
Member

Yeah, let's try those ones. Note they have just moved to the Compiler.Service project, but haven't changed otherwise.

@psfinaki
Copy link
Member

psfinaki commented Jun 3, 2024

@Tangent-90 sorry it's taking this long - I am really trying my best to verify this. Among other things, it will be helpful when writing the release notes for VS.

I have checked out your branch and I am trying to repro the last example you've shared but seems like I am still getting the incorrect behavior:

Recording.2024-06-03.145038.mp4

How does this look for you? And what could I be doing wrong?

@ijklam
Copy link
Contributor Author

ijklam commented Jun 3, 2024

@Tangent-90 sorry it's taking this long - I am really trying my best to verify this. Among other things, it will be helpful when writing the release notes for VS.

I have checked out your branch and I am trying to repro the last example you've shared but seems like I am still getting the incorrect behavior:

Recording.2024-06-03.145038.mp4

How does this look for you? And what could I be doing wrong?

This pr is solving problem in completion list of unresolved symbols (press Ctrl+J in VS or Alt+Right). The bug show in your video is of the code fix, and I haven't try that before 😂.

@psfinaki
Copy link
Member

psfinaki commented Jun 3, 2024

@Tangent-90 sorry it's taking this long - I am really trying my best to verify this. Among other things, it will be helpful when writing the release notes for VS.
I have checked out your branch and I am trying to repro the last example you've shared but seems like I am still getting the incorrect behavior:

Recording.2024-06-03.145038.mp4

How does this look for you? And what could I be doing wrong?

This pr is solving problem in completion list of unresolved symbols (press Ctrl+J in VS or Alt+Right). The bug show in your video is of the code fix, and I haven't try that before 😂.

Okay fair enough, I was looking at the wrong place :)

But now, hopefully, I am looking at the right place and still don't notice the change... Trying Ctrl + J here:

Recording.2024-06-03.154513.mp4

@ijklam
Copy link
Contributor Author

ijklam commented Jun 3, 2024

But now, hopefully, I am looking at the right place and still don't notice the change... Trying Ctrl + J here:

You may need to enable this option. This is disabled by default.

图片

And then you will get a very long completion list like this:

图片

@psfinaki
Copy link
Member

psfinaki commented Jun 3, 2024

Ohhhh I see now. Yeah that was it, now I see the difference. All good, thanks for the explanations and the testing!

@psfinaki psfinaki merged commit bcffdf7 into dotnet:main Jun 3, 2024
@ijklam ijklam deleted the fix-14375 branch June 4, 2024 18:19
vzarytovskii added a commit that referenced this pull request Jun 21, 2024
Co-authored-by: Kevin Ransom (msft) <[email protected]>
Co-authored-by: Petr <[email protected]>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Brian Rourke Boll <[email protected]>
Co-authored-by: Vlad Zarytovskii <[email protected]>
Co-authored-by: Viktor Hofer <[email protected]>
Co-authored-by: Florian Verdonck <[email protected]>
Co-authored-by: Jakub Majocha <[email protected]>
Co-authored-by: ijklam <[email protected]>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dawe <[email protected]>
Co-authored-by: Tom Deseyn <[email protected]>
Fix AOT (#17238)" (#17264)
Fix full source-build product build when running R2R (#17259)
Fix plain build of FSC fsproj (#17270)
Fix #14375 by showing and inserting correct name of entities from unopened namespace/module (#17261)
fix #14375
Fix a typo in docs (#17273)
Fix sbom generation (#17275)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants