Skip to content

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Mar 23, 2023

Wire up the new width setting (#14891) :
image

image

@majocha majocha requested a review from a team as a code owner March 23, 2023 11:52
@majocha
Copy link
Contributor Author

majocha commented Mar 23, 2023

This needs finishing the translations etc. but should already work for testing.

@majocha
Copy link
Contributor Author

majocha commented Mar 23, 2023

BTW the process of putting together quickinfo content in VS is super convoluted, this should be really mostly delegated to the service. And why is the code in GoToDefinition.fs?

@majocha
Copy link
Contributor Author

majocha commented Mar 23, 2023

Ready when green, unless I forgot something.

Copy link
Contributor

@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.

As for your questions:

BTW the process of putting together quickinfo content in VS is super convoluted, this should be really mostly delegated to the service.

Agree, this is worth refactoring probably.

And why is the code in GoToDefinition.fs?

Hard to say from the top of my head, also needs an investigation, GoToDefinition seems to be full of terrible code (and most likely, bugs).

@majocha
Copy link
Contributor Author

majocha commented Mar 23, 2023

As for all the boxing in UI code, this can't be helped. WPF is "object oriented" like this. Our OptionPages are strongly typed, so this is safely contained.

@psfinaki
Copy link
Contributor

As for all the boxing in UI code, this can't be helped. WPF is "object oriented" like this. Our OptionPages are strongly typed, so this is safely contained.

"Object oriented" :) Alright, let it be then. If you feel for it, you can add some tests for the conversion code.

@psfinaki
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro
Copy link
Member

T-Gro commented Mar 24, 2023

I would prefer to have the description a lot more explicit about:

  • which description it is (mouseover quickinfo)
  • what does it cause (a line break)
  • what are the units its specifies (no. of characters)

@majocha
Copy link
Contributor Author

majocha commented Mar 25, 2023

I would prefer to have the description a lot more explicit about:

  • which description it is (mouseover quickinfo)
  • what does it cause (a line break)
  • what are the units its specifies (no. of characters)

The first and the last point are already covered.

As for the second one, there is not a simple answer and this feature in fact needs an (experimental) label.
What this PR does is just expose the description width setting that we now have in FCS. It's good to test that feature in VS and just that.
This option in VS can be confusing. As a user I would expect a width setting for quickinfo to always work, but it won't always work:
The tooltip will look nice in situations when there is a short or inexistent xml doc, but still super wide when there are long paragraphs of xml docs.

I think there is a simple way to handle the docs width properly. I'll try to expand this PR to do this. If it won't work, I'll and an "experimental" label.

OK, I'm waaay out of the loop and I didn't notice VS now (finally) wraps tooltips approximately to the width of the editor. It does not make sense to fight that built-in behavior, the FCS width setting this PR exposes should work fine with it. I'll add a tooltip explaining what it does exactly.

@majocha
Copy link
Contributor Author

majocha commented Mar 27, 2023

image
Ready if this is acceptable.

Copy link
Contributor

@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.

Hah, great idea with the tooltip - this didn't come to me. I wonder how accessible they are but maybe we should add them to other places in the options.

Anyway, thanks! Merging.

@psfinaki psfinaki merged commit 3d4eb5c into dotnet:main Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants