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

tooltip fix for hover and code completion on VS code #9862

Merged
merged 5 commits into from
Jan 29, 2024

Conversation

jordi1215
Copy link
Member

@jordi1215 jordi1215 commented Jan 25, 2024

Make the tooltip show the full tagHelperTypeName instead of the reduced name in LSP factory. This method is used in both hover service and code completion service.
Fixes #9638

Summary of the changes

  • To make the TryCreateTooltip method in DefaultLSPTagHelperTooltipFactory return the tag helper’s full type name with its namespace, I added the namespace to the descriptionBuilder variable. (see picture below for visualization)
  • I also changed the corresponding unit tests in DefaultLSPTagHelperTooltipFactoryTest.cs and HoverInfoServiceTest.cs to test for the now desired behavior with the full namespace shown in tag helper with the type name in bold.
  • This change only affects the VS code user experience and does not affect that of Visual Studio.

Fixes:

  • This fixes GitHub issue In VSCode hovering over a component tag should show the full component type name #9638 where hovering on a razor element tag on VS code shows the reduced element name and not the full name with the name space included.
  • Since the method is also used in code completion in VS code, it also shows the full name in the description box that pops out when the user previews a code completion option that happens to be a razor element.

Testing:

  • Visual Studio Code version: 1.85.2
  • I checked that the increased length does not pose an issue on the hover display in VS code as the tag name wraps around nicely for up to 1000 characters.
  • I also checked the code completion display where the tag name gets truncated at about 100 characters. But that shouldn't be too much of an issue for users since namespace is not usually that long.

Before

9862_bfore

After

HeadOutLet_after

Make the tooltip show the full tagHelperTypeName instead of the reduced name in LSP factory. This method is used in both hover service and code completion service.
@jordi1215 jordi1215 requested a review from a team as a code owner January 25, 2024 20:00
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

I'm interested to know, what makes ReduceTypeName(...) return a different value on VS Code vs. VS? It feels like the answer to that question would lead to the best fix. Since there are existing unit tests that verify the current behavior, I suspect that changing the user experience isn't the best fix.

-DefaultLSPTagHelperTooltopFactoryTest.cs: changed three unit tests so we are testing for showing the full name space
-  HoverInfoServiceTest.cs: changed the test to match the scenario of child hover show the namespce of parent.
@jordi1215
Copy link
Member Author

I'm interested to know, what makes ReduceTypeName(...) return a different value on VS Code vs. VS? It feels like the answer to that question would lead to the best fix. Since there are existing unit tests that verify the current behavior, I suspect that changing the user experience isn't the best fix.

Thank you for your comment @DustinCampbell. Thanks to @davidwengier who pointed out in issue #9638, there are "two different code paths for generating hovers". We can re-produce the reduced TypeName behavior in Visual Studio by "forcing the isVSClient variable to be false in the HoverInfoService".

I think the difference exists because VS Code takes markdown text as an input to display hover information to the user whereas in Visual Studio it accepts ContainerElement for hover and ClassifiedTextElement for code completion. This allows the users to see highlighted text and icon pictures when they hover on Razor elements. The code does that by reducing the TypeName, classify it, and then re-appending it to the full namespace. That part of the code remains untouched by this pull request.

I think the end goal is for VS Code users to have the same amount of information on the UI side as the Visual Studio users. An immediate future step is to investigate how we can also "classify" the text in hover description for VS code.

- implemented simple logic to make the reducedTypeName bold while still keeping the namespace but leaving it intact
- changed unit tests in DefaultLSPTagHelperTooltipFactoryTest and HomeInfoServiceTest to reflect such desired behavior
@DustinCampbell
Copy link
Member

I think the end goal is for VS Code users to have the same amount of information on the UI side as the Visual Studio users.

I'm definitely on board with that. I just want to make sure that this PR only changes the VS Code user experience and doesn't actually change VS experience. Is that the case?

- using the range operator instead for getting the namespace of the TagHelper
- other fixes: reducing extra white line, fixing comment
@jordi1215
Copy link
Member Author

jordi1215 commented Jan 29, 2024

I think the end goal is for VS Code users to have the same amount of information on the UI side as the Visual Studio users.

I'm definitely on board with that. I just want to make sure that this PR only changes the VS Code user experience and doesn't actually change VS experience. Is that the case?

As far as I understand that is correct. The tooltip for Visual Studio is being handled in DefaultVSLSPTagHelperTooltipFactory.cs (note "VS") where as the tooltip for VS Code is handled in DefaultLSPTagHelperTooltipFactory.cs (note no "VS").

@jordi1215 jordi1215 changed the title tooltip fix for hover and code completion on VS code (issue #9638) tooltip fix for hover and code completion on VS code Jan 29, 2024
deleted an extra comment
@jordi1215 jordi1215 merged commit 8742d80 into main Jan 29, 2024
12 checks passed
@jordi1215 jordi1215 deleted the dev/jordi1215/tooltip-fix(#9638) branch January 29, 2024 23:54
@ghost ghost added this to the Next milestone Jan 29, 2024
@Cosifne Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024
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.

In VSCode hovering over a component tag should show the full component type name
5 participants