Changing the "# References" to display before the set of attributes(if any) #1938
Conversation
akshita31
requested review from
DustinCampbell,
filipw,
TheRealPiotrP and
rchande
|
Omnisharp-roslyn side : OmniSharp/omnisharp-roslyn#1075 |
|
The "run test" and "debug test" now show up as a separate codelens. |
|
@akshita31 Yes, we need to move the test codelens too. |
src/features/codeLensProvider.ts
Outdated
| let column = document.positionAt(node.AttributeSpanStart).character; | ||
| let endLine = document.positionAt(node.AttributeSpanEnd).line; | ||
| let endColumn = document.positionAt(node.AttributeSpanEnd).character; | ||
| return new Range(line, column, endLine, endColumn); |
There was a problem hiding this comment.
I think there's a range constructor that takes 2 Positionss (instead of 2 l,c coordinates)
src/features/codeLensProvider.ts
Outdated
| this.fileName = fileName; | ||
| this.actualRange = actualrange; |
There was a problem hiding this comment.
casing doesn't match that of attributeRange
src/features/codeLensProvider.ts
Outdated
| return []; | ||
| } | ||
|
|
||
| return serverUtils.currentFileMembersAsTree(this._server, { FileName: document.fileName }, token).then(tree => { | ||
| let ret: vscode.CodeLens[] = []; | ||
| tree.TopLevelTypeDefinitions.forEach(node => this._convertQuickFix(ret, document.fileName, node)); | ||
| tree.TopLevelTypeDefinitions.forEach(node => this._convertQuickFix(ret, document, node)); |
There was a problem hiding this comment.
Not your fault, but can we make this method async (and thus not having to do the then chaining)?
|
I love this change, but AFAIR the idea was to not diverge the VS Code experience from full VS. Is that not the case anymore? |
|
@filipw From talking to other Roslyn folks at lunch, it sounds like people are generally in favor of the change. I'm not aware of any requirement that the VS Code experience exactly match the VS one. Thoughts @DustinCampbell ? I would argue that if this is a better experience (and I think it is) that we should do it. |
|
This requires a new OmniSharp build, correct? |
|
@rchande : The experience for C# in VS vs. VS Code doesn't need to be precisely the same, but that is our starting point for designing the experience. If we decide that the experience should be different, we need to be able to articulate why, and also understand we aren't changing VS to match. I would not split references from the test code lenses here. That looks pretty bad to me. IMO, all of the code lenses should be above the attributes to match VS unless we've got a reason not to change VS. |
|
@DustinCampbell It looks like VS currently puts the codelens adornment underneath attributes. I'll sync with the IDE folks to see what they think and determine if they want to make the same change in VS.
|
|
@rchande : Yes, that was a conscious decision, and I think we should keep it there. When we implemented CodeLens in VS, I was the one who made the case for keeping the adornment directly above the method declaration and not allowing it to drift above the attributes. Here's an example of why we made that decision from the Roslyn code base:
It's an extreme case, but it's a real concern. |
|
cc @jinujoseph and @kuhlenh for consistency between VS and VS Code. |
|
I'm also curious to know if any thought has been given to XML doc comments, which are defined grammatically to appear above attributes. Does this still look good when the code lens is between XML doc comments and attributes? |
|
Also cc'ing @Pilchie in case he remembers any other reasons why we chose the position of code lens in VS that we did. |
|
@DustinCampbell's comments match my recollection. |
|
This is marked as "Blocked on OmniSharp". |
|
it was "Blocked on OmniSharp" because it required a corresponding change in the Omnisharp language server. |
|
That's very disappointing to hear |
Fixes: #429
To display the # of references before the set of attributes, the provideCodeLens must return a range for the attributes of the node. So changed the omnisharpLens object to contain two ranges - one the
actualRangeof the node ( to be consumed by the resolveCodeLens method) and the otherattributeRangethat will be consumed to determine the position to display the references.Also added
attributeSpantoRangehelper to convert span to a range in the document.