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

Implement Quick Info styles #35667

Merged
merged 10 commits into from
Jul 25, 2019
Merged

Implement Quick Info styles #35667

merged 10 commits into from
Jul 25, 2019

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented May 12, 2019

  • Support emphasis in Quick Info (<em> and <i> elements in XML documentation comments)
  • Support strong in Quick Info (<strong> and <b> elements in XML documentation comments)
  • Support code font in Quick Info (<c>, <code>, <tt> elements in XML documentation comments)
  • Support bullet and numbered lists in Quick Info
  • Support definition lists in Quick Info
  • Support navigating to cref items by clicking in Quick Info
  • Support navigating to href hyperlinks in Quick Info (<see href and <a href)

Images

⚠️ This picture is slightly outdated relative to current behavior (both links to https://google.com now render correctly, and the spacing for list items has changed slightly.
image

image

@CyrusNajmabadi

This comment has been minimized.

@sharwell

This comment has been minimized.

catch
{
// Ignore symbol resolution failures. It likely is just a badly formed URI.
return;
Copy link
Member

Choose a reason for hiding this comment

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

def not liking this.

  1. symbolkey shoudn't be throwing.
  2. 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.

Copy link
Member Author

@sharwell sharwell May 14, 2019

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

@jinujoseph jinujoseph added Area-IDE Need Design Review The end user experience design needs to be reviewed and approved. labels May 14, 2019
@sharwell sharwell force-pushed the quickinfo-styles branch 3 times, most recently from 4b9da8a to 3c3517e Compare May 24, 2019 16:41
@sharwell sharwell marked this pull request as ready for review May 24, 2019 16:42
@sharwell sharwell requested review from a team as code owners May 24, 2019 16:42
@sharwell
Copy link
Member Author

@CyrusNajmabadi @dotnet/roslyn-ide This is now ready for proper review

nestedElements.Skip(1)))));
}

continue;
Copy link
Member

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);
Copy link
Member

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?

@sharwell
Copy link
Member Author

sharwell commented Jul 3, 2019

Can we add href here?

I'll file a follow-up issue for this, and also for the other elements that are now supported by Quick Info.

When you use an entity that's invalid XML, like &nbsp;, we previously showed a squiggle for CS1570.

I have not made any changes in this space. The compiler is in control of the scenario.

Spacing within <code> blocks is not preserved (multiple adjacent spaces or newlines).

This is a known limitation; will file a follow-up issue.

You can't type <c> to get a c tag

IntelliSense is not allowed to ever make this replacement. Can you describe how you encountered this?

Also can't type <a href for the same reason.

This is a side effect of not having a in the completion list. It will be covered by the issue that includes this and other newly-supported elements.

Clicking on a link created using <a href sometimes causes an assert:

This is a debug-only side effect of #35667 (comment), and covered by the same follow-up issue.

Matching close tags for new constructs are not offered after </

This is an IntelliSense bug unrelated to this pull request.

You can make numbered lists that start at 0. Not sure if bug or feature.

It's not a bug or a feature. It's Quick Info's current handling of unspecified behavior for an incorrectly-formed <list> element. I'm fine with this edge case.

@dpoeschl
Copy link
Contributor

dpoeschl commented Jul 8, 2019

When you use an entity that's invalid XML, like  , we previously showed a squiggle for CS1570.

I have not made any changes in this space. The compiler is in control of the scenario.

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.

Copy link
Contributor

@dpoeschl dpoeschl left a comment

Choose a reason for hiding this comment

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

👍

@sandyarmstrong
Copy link
Member

sandyarmstrong commented Aug 6, 2019

@sharwell is it already a known issue that there there is no completion item for "href" or "a" when writing XML doc comments?

EDIT: OK, I see #37504 now, nevermind!

@vancura
Copy link

vancura commented Aug 20, 2019

Please, why don't you use semantically correct classnames, which are independent of the styling? <strong>, <em> and similar are too connected to the form/styling. It would be better to learn from the HTML/CSS world (when CSS is semantically correct) and use independent tokens.

This is where I am coming from (I am designer of VSMac, VS and VSCode):

image

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?

@vancura
Copy link

vancura commented Aug 20, 2019

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.

@CyrusNajmabadi
Copy link
Member

Please, why don't you use semantically correct classnames, which are independent of the styling?

Could you clarify who you are asking here? It's not clear to me what change you are asking for.

I am designer of VSMac, VS and VSCode

Sounds like something you could drive improvements in :)

@sharwell
Copy link
Member Author

sharwell commented Aug 22, 2019

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

@sharwell
Copy link
Member Author

It's not clear from your screenshot what your question is. Here's a copy of your screenshot showing the section of the Quick Info popup which is covered by this pull request. Note that the only change to the first line is the use of hyperlinks.

image

@sharwell
Copy link
Member Author

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

@vancura
Copy link

vancura commented Aug 22, 2019

@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! :)

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.

7 participants