Proper Newlines in Hover #1918

akshita31 · 2017-12-11T19:03:58Z

Fixes : #1057

Added Code to allow VS Code to consume the "StructuredDocumentation" object returned by omnisharp-roslyn in typelookup and append newlines appropriately. Also added an integration test for the same.

Please review : @DustinCampbell @TheRealPiotrP @rchande

Omnisharp-roslyn side : OmniSharp/omnisharp-roslyn#1038

…up_new

DustinCampbell · 2017-12-11T19:49:52Z

It looks like there's something wrong with your branch in that it's picking up several changes from @rchande

rchande · 2017-12-11T20:44:02Z

@DustinCampbell That was intentional. We needed to make a few tweaks to make integration tests more reliable.

DustinCampbell · 2017-12-11T20:46:12Z

Got it.

DustinCampbell · 2017-12-11T20:56:44Z

src/features/hoverProvider.ts

+                            documentation += structDoc[val] + newline;
+                        } 
+                    }
+                );


I'm not sure I'm sold on the property loop. This will cause the order of the tooltip to change if the properties are ever returned in a different order. Also, if any other element that becomes an array would get the heading "Exceptions:". Should we just check the properties we care about and construct the tooltip text in a deterministic way? E.g.

if (structDoc.SummaryText) { documentation += structDoc.SummaryText; } if (structDoc.ParamElements && structDoc.ParamElements.length > 0)) { documentation += "Parameters:" + newLine; documentation += structDoc.ParamElements.join(newLine) + newLine; } etc...

DustinCampbell · 2017-12-11T21:00:16Z

@akshita31 : Could include a screenshot of the hover tooltip with your change?

akshita31 · 2017-12-11T21:25:20Z

Here is the look after the change.

rchande · 2017-12-11T21:55:47Z

I suspect tests are going to fail because we haven't shipped a new Omnisharp into VS Code in a while. @DustinCampbell mind if I do that this week? (Likely tomorrow)

DustinCampbell · 2017-12-11T22:08:12Z

@rchande: I have no problem with it. Check with the OmniSharp folks though. Somebody will need to go through and update the OmniSharp changelog for all of the merged changes since 1.28 as well.

DustinCampbell · 2017-12-11T22:26:22Z

@rchande: We should make this a topic during the OmniSharp meeting tomorrow.

DustinCampbell · 2017-12-12T16:55:57Z

@akshita31 : Do we want to include all of the tags in the hover tooltip? Visual Studio specifically doesn't do that.

Also, it doesn't include a "Summary" heading or the "Returns".

DustinCampbell · 2017-12-12T16:58:05Z

If we do want to include the parameters and other items like that, I'd recommend spending some time organizing the tooltip to look a bit better organized, rather than just putting a blank line in between everything. For example:

Checks if the object is tagged with the tag.

Parameters:
    gameObject: The game object.
    tagName: Name of the tag.

Returns: Returns true if the object is tagged with tag.

Also, note that there appears to be a bug in your screenshot where there is no space between "true" and "if".

akshita31 · 2017-12-12T21:17:10Z

I tried indenting the parameters but seems that vscode doesn't display any "\t" that I added.

However on adding "\t", the text color changes. I am not sure what is happening around here.

rchande · 2017-12-12T21:21:26Z

That looks like a classification bug for @DustinCampbell to discuss.
Regarding the spacing, I think we discovered earlier that the VS Code popup turns all whitespace characters into tabs. You'll probably want to add 4 spaces (or whatever you think looks right) instead of a \t.

DustinCampbell · 2017-12-12T23:55:16Z

@akshita31 : The "Summary" text is extra noise and a regression from previous behavior. Here's what it looks like today:

IMO, none of the properties on StructuredDocumentation should include a heading. Parameters, type parameters and exceptions should probably be arrays of objects with "Name" and "Documentation" properties. The text should just be the documentation string. If the host wants to add a heading during display, that's fine, but it seems strange that omnisharp-roslyn would do it. @rchande: What do you think?

rchande · 2017-12-13T00:09:20Z

@DustinCampbell I agree. Even if we wanted to present a heading, that's something that should be up to clients, not forced on them by OmniSharp. @akshita31 Can you help work on a change to Omnisharp that so we don't add "Summary" and so forth to the section text returned from OmniSharp? Sorry we didn't catch that during your code review!

akshita31 · 2017-12-13T00:17:20Z

Yes, I'll do that.

rchande · 2017-12-13T00:40:43Z

Also @DustinCampbell, should we wait for that change before we do the release we're planning this week?

DustinCampbell · 2017-12-13T01:54:09Z

It wouldn't take long to make the tweak. I'd just submit the PR to omnisharp-roslyn and we'll take it. After all, we're still waiting on the three Cake PRs.

DustinCampbell · 2017-12-13T01:55:56Z

And if we ship omnisharp-roslyn before the PR goes in, c'est la vie! I'm assuming that we'll be taking another omnisharp-roslyn before shipping omnisharp-vscode.

rchande · 2017-12-13T16:04:56Z

@DustinCampbell By classification, I meant that it looks like adding "\t" to a string literal turns all the tokens blue (https://user-images.githubusercontent.com/12554141/33908867-9d98a40a-df3e-11e7-84fe-6302aca85a8d.png) from above.

DustinCampbell · 2017-12-13T16:12:43Z

Oh yeah, that's weird. I wonder if VS Code's markdown grammar gets run over this.

rchande · 2018-01-10T01:49:29Z

@rchande We need to publish omnisharp to the CDN in order to merge this.

rchande

Couple of tweaks otherwise lgtm

rchande · 2018-01-12T22:06:42Z

src/features/documentation.ts

+    }
+
+    if (structDoc.RemarksText) {
+        documentation += structDoc.RemarksText + newLine;


Might be worth extracting out a little helper method that adds the newline for you.

rchande · 2018-01-12T22:07:10Z

src/features/hoverProvider.ts

@@ -21,9 +21,11 @@ export default class OmniSharpHoverProvider extends AbstractSupport implements H

        return serverUtils.typeLookup(this._server, req, token).then(value => {
            if (value && value.Type) {
-                let contents = [extractSummaryText(value.Documentation), { language: 'csharp', value: value.Type }];
+                let structDoc = value.StructuredDocumentation;


structDoc unused?

rchande · 2018-01-12T22:09:11Z

@akshita31 That unused variable causing your Travis build to fail.

…isharp-vscode into typelookup_changed

akshita31 and others added 12 commits December 6, 2017 11:29

Added the changes for structured documentation

2f9c784

Merge remote-tracking branch 'rchande/testImprovements' into typelook…

cc22739

…up_new

Test

0df87fa

First Test Running Properly

27f5dde

Integration Test Running

50aa8e8

Test Commit

007dba8

Add new test files next to the csproj, not in the root of the workspace

b66e5f1

Make it possible for tests to await restore

6873f90

Enable tests to wait for the omnisharp request queue to be empty

882d13b

Wait for queue empty in hover test

ccc21a6

Removed debug statements

d0acf84

Removed Unnecessary space

eebb979

akshita31 changed the title ~~Typelookup changed~~ Proper Newlines in Hover Dec 11, 2017

Added braces for if-else

0207222

DustinCampbell reviewed Dec 11, 2017

View reviewed changes

Added conditions for each field individually

66f5b7e

DustinCampbell added the Blocked on OmniSharp label Dec 12, 2017

DustinCampbell mentioned this pull request Dec 12, 2017

Handling Newlines in XML Documentation #1869

Closed

Changes for parameter as an object

05ed4bd

akshita31 mentioned this pull request Dec 13, 2017

Typelookup clean up and add objects for parameters and similar symbols OmniSharp/omnisharp-roslyn#1062

Merged

Add missing line

47129b7

akshita31 added 2 commits December 13, 2017 10:49

Changed to DocumentationItem

e289fb1

Merge remote-tracking branch 'upstream/master' into typelookup_changed

d7e0f35

jlmakes mentioned this pull request Dec 24, 2017

XML Documentation display errors #1940

Closed

akshita31 added 7 commits January 10, 2018 16:38

Extracted Documentation helper as a function in a separate file

c2a32f7

Removed unnecessary using

9e61bc9

Merge remote-tracking branch 'upstream/master' into typelookup_changed

b57e7a9

Code Clean up

ecec82c

Clean up

cbfde53

Fixed spacing

62e6c2e

Merge branch 'master' into typelookup_changed

5fe5694

rchande removed the Blocked on OmniSharp label Jan 12, 2018

rchande approved these changes Jan 12, 2018

View reviewed changes

akshita31 added 2 commits January 12, 2018 15:33

Removed unused variable

5e69bcb

Merge branch 'typelookup_changed' of https://github.com/akshita31/omn…

3e4b01a

…isharp-vscode into typelookup_changed

akshita31 merged commit eb66c4b into dotnet:master Jan 13, 2018

akshita31 deleted the typelookup_changed branch February 23, 2018 23:14

erichiller mentioned this pull request Sep 5, 2020

Documentation output missing sections, lacking formatting, lacking newlines. #4040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper Newlines in Hover #1918

Proper Newlines in Hover #1918

akshita31 commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017

rchande commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017

DustinCampbell Dec 11, 2017

DustinCampbell commented Dec 11, 2017

akshita31 commented Dec 11, 2017

rchande commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017 •

edited

Loading

DustinCampbell commented Dec 12, 2017

DustinCampbell commented Dec 12, 2017

akshita31 commented Dec 12, 2017

rchande commented Dec 12, 2017

DustinCampbell commented Dec 12, 2017

rchande commented Dec 13, 2017

akshita31 commented Dec 13, 2017

rchande commented Dec 13, 2017

DustinCampbell commented Dec 13, 2017

DustinCampbell commented Dec 13, 2017

rchande commented Dec 13, 2017

DustinCampbell commented Dec 13, 2017

rchande commented Jan 10, 2018

rchande left a comment

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande commented Jan 12, 2018

Proper Newlines in Hover #1918

Proper Newlines in Hover #1918

Conversation

akshita31 commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017

rchande commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017

DustinCampbell Dec 11, 2017

Choose a reason for hiding this comment

DustinCampbell commented Dec 11, 2017

akshita31 commented Dec 11, 2017

rchande commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017

DustinCampbell commented Dec 11, 2017 • edited Loading

DustinCampbell commented Dec 12, 2017

DustinCampbell commented Dec 12, 2017

akshita31 commented Dec 12, 2017

rchande commented Dec 12, 2017

DustinCampbell commented Dec 12, 2017

rchande commented Dec 13, 2017

akshita31 commented Dec 13, 2017

rchande commented Dec 13, 2017

DustinCampbell commented Dec 13, 2017

DustinCampbell commented Dec 13, 2017

rchande commented Dec 13, 2017

DustinCampbell commented Dec 13, 2017

rchande commented Jan 10, 2018

rchande left a comment

Choose a reason for hiding this comment

rchande Jan 12, 2018

Choose a reason for hiding this comment

rchande Jan 12, 2018

Choose a reason for hiding this comment

rchande commented Jan 12, 2018

DustinCampbell commented Dec 11, 2017 •

edited

Loading