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

[SourceLink] Go To Def tab improvements #57995

Closed

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Nov 26, 2021

Part of #55834

Includes #57978 so just review 2b6c31a (#57995)
Tabs look like this now:
image

@davidwengier davidwengier requested review from a team as code owners November 26, 2021 02:59
@davidwengier davidwengier changed the title Source link tab improvements [SourceLink] Go To Def tab improvements Nov 26, 2021
var documentName = string.Format(
"{0} [{1}]",
navigateDocument!.Name,
FeaturesResources.from_metadata);
sourceFileInfos[0]!.SourceDescription);
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean for sourceFileInfos to be an array here? If the first is significant/special, can we extract that out to a local (that's also been null-suppressed once) rather than duplicating it everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately for now, the first is not special, but whilst GoToDef supports multiple locations, MetadataAsSoruce doesn't, so there is some refactoring of things that needs to happen I haven't started to look at yet. A local is probably a good idea

@@ -2948,4 +2948,12 @@ Zero-width positive lookbehind assertions are typically used at the beginning of
<data name="Silent" xml:space="preserve">
<value>Silent</value>
</data>
<data name="embedded" xml:space="preserve">
<value>embedded</value>
<comment>Embedded is a technical term for "Embedded source", where souce files are embedded into the PDB</comment>
Copy link
Member

Choose a reason for hiding this comment

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

On behalf of the translation teams, thank you for adding comments like this. 😄

Comment on lines +2956 to +2957
<value>external</value>
<comment>External means "external source", meaning source files that are not part of the current solution</comment>
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to tag this specially, if it's opening a file on your machine? Like does this get strange if you then want to edit that file like a local file, or add that missing project to your solution?

Copy link
Member

Choose a reason for hiding this comment

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

Or are we taking the on-disk file and copying it to a temp folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you were right, this is a file on your machine.. but we open it readonly, and if you were to edit it, then we wouldn't find it because the checksum wouldn't match. If we didn't tag it specifically, what would we say? I don't think it should look like a normal file, because its not in the solution, so lots of things don't work like normal (and as mentioned, its readonly). I don't think the tab should say "from metadata", because its the real source. So "external" is the best I could come up with. "local" I guess could work?

@davidwengier
Copy link
Contributor Author

Closing this. Going to combine with the output window logging, and add some telemetry, in a follow up since this missed Preview 2 anyway.

@davidwengier davidwengier deleted the SourceLinkTabImprovements branch January 21, 2022 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants