-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Implement Quick Info styles #35667
Implement Quick Info styles #35667
Conversation
This comment has been minimized.
This comment has been minimized.
src/EditorFeatures/Core/Implementation/INavigateToLinkService.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
...eatures/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/QuickInfo/SyntacticQuickInfoSourceTests.cs
Outdated
Show resolved
Hide resolved
catch | ||
{ | ||
// Ignore symbol resolution failures. It likely is just a badly formed URI. | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def not liking this.
- symbolkey shoudn't be throwing.
- not happy about all this sync blocking. can we not be async here?
Worst case, when someone navigates, we can do a TWD and make it cancellable if something goes off the rails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not happy about all this sync blocking. can we not be async here?
Limited by the editor API. @gundermanc how would you handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: sync blocking is ok with me. it's more the lack of cancellability. That should hopefully be something we can fix up with a TWD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only option is to do whatever the equivalent of a Dispatcher.InvokeAsync()
is in this layer. From the IDE side, we usually use JTF.RunAsync(). I understand this is the portable editor features, so, there likely is no dispatcher.
For my own edification, would it be preferable to have actions like this be async? I didn't think it was necessary originally because we're de facto going to have to InvokeAsync() somewhere but I acknowledge things are a bit more constrained for Roslyn since this layer has to work on Mac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with this being async (which honestly makes sense to me given that any of thse clicks might cause major work to happen). Or, if sync, at least have it be under a TWD.
4b9da8a
to
3c3517e
Compare
@CyrusNajmabadi @dotnet/roslyn-ide This is now ready for proper review |
src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs
Outdated
Show resolved
Hide resolved
nestedElements.Skip(1))))); | ||
} | ||
|
||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we consider extracting out everything above (into something like StartContainerElement(...))? I thnk it would make it much easier to understand things if we just had a top level lopo, with the high level decision making, and then have the individual functions that are responsible for each piece of work. intermingling them makes it harder to keep track and understand what's going on for me.
@@ -39,5 +39,7 @@ public static class TextTags | |||
public const string EnumMember = nameof(EnumMember); | |||
public const string ExtensionMethod = nameof(ExtensionMethod); | |||
public const string Constant = nameof(Constant); | |||
internal const string ContainerStart = nameof(ContainerStart); | |||
internal const string ContainerEnd = nameof(ContainerEnd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you doc this? when would you use ContainerEnd for example?
...eatures/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs
Show resolved
Hide resolved
I'll file a follow-up issue for this, and also for the other elements that are now supported by Quick Info.
I have not made any changes in this space. The compiler is in control of the scenario.
This is a known limitation; will file a follow-up issue.
IntelliSense is not allowed to ever make this replacement. Can you describe how you encountered this?
This is a side effect of not having
This is a debug-only side effect of #35667 (comment), and covered by the same follow-up issue.
This is an IntelliSense bug unrelated to this pull request.
It's not a bug or a feature. It's Quick Info's current handling of unspecified behavior for an incorrectly-formed |
In the non-inheritdoc scenario, this is fine. I had inconsistent settings between test projects for generating the xml doc files. I'm not positive about the inheritdoc scenario. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
fd86d14
to
69b513f
Compare
69b513f
to
bcb448a
Compare
Please, why don't you use semantically correct classnames, which are independent of the styling? This is where I am coming from (I am designer of VSMac, VS and VSCode): See mainly on the right-hand side: just about this week I wanted to prepare a couple of comps on all our platforms how this output could work, and then propose a Roslyn JSON/XML output which could be then parsed and styled by the target application. Do you think your new output format could potentially do this, too? I am slightly nervous that Roslyn will recommend where the design should use bold, italics, and other stylistic treatments. This should be up to the target application (and the designer of it) to decide. Thoughts? |
Hm, I see it's too late to complain, this was already merged (I learned about the feature just a couple of minutes ago). Let's see how much can we parse and style the output in our products even with the style tags around. |
Could you clarify who you are asking here? It's not clear to me what change you are asking for.
Sounds like something you could drive improvements in :) |
@vancura This pull request does not require the use of any specific tags. It does not add the items to IntelliSense where users would be encouraged to use them. The only thing it does is apply styling in the IDE for users who already were using these elements, and for those users the style it is now showing matches the style they expected. |
@vancura Please include me in the discussion of documentation formats. I need to make sure the proposal will work with existing documentation produced by the compiler as well as post-processing tools like Sandcastle Help File Builder, and also works with proposals like dotnet/csharplang#891 with ongoing design work in https://gist.github.com/sharwell/ab7a6ccab745c7e0a5b8662104e79735. |
@sharwell Thank you for the clarification! I will draw a couple of pictures from all the products, so we have something to look at, and then it would be good to have a short talk on what we can do. Thanks! :) |
<em>
and<i>
elements in XML documentation comments)<strong>
and<b>
elements in XML documentation comments)<c>
,<code>
,<tt>
elements in XML documentation comments)cref
items by clicking in Quick Infohref
hyperlinks in Quick Info (<see href
and<a href
)Images