Make Virtual document tracking apply to specific scheme. #2562

NTaylorMullen · 2018-09-29T00:14:44Z

Also fixed issue where we'd attempt to initialize virtual documents before the project world was ready. This is an issue because virtual documents don't exist as part of the MSBuild piece of the project and therefore would more-often than not get put into the miscellaneous project on the server; resulting in bad IntelliSense.
Fixed the diagnostic provider to also obey the new virtualCSharp-* scheme requirement
Changed the default change forwarder to ignore virtual documents to ensure that it doesn't try and trigger any changes for them until the server is ready.

Fixes #2538
Fixes aspnet/Razor.VSCode#105

@akshita31 I ported your changes to this PR and made the few PR requested changes. Also, I as well couldn't add a negative test for the virtual document tracking for reasons here.

NTaylorMullen · 2018-09-29T00:17:03Z

src/features/virtualDocumentTracker.ts

+
+        // We wait until project state on the server has been initialized to add any virtual documents.
+
+        server.waitForEmptyEventQueue().then(async () => {


@rchande @akshita31 this is ugly but essentially we need to wait until all of the projects have been initialized so we wait for the project initialization process to start and then in the background (we're not awaiting this) queue up a virtual document tracker initialization step. All of this is to ensure that we don't try and initialize a virtual document before its corresponding project is ready.

I thought about trying to assume the project in this virtual document tracker but figured that'd break functionality for anyone who truly just wanted a miscellaneous project only virtual document.

If there's a better way to do this i'm all ears.

I don't think this is doing what you want. waitForEmptyEventQueue() waits for all the pending extension -> omnisharp messages to be send and responded to over the stdio communication channel. It doesn't have any knowledge of when OmniSharp is done loading projects (and unfortunately, there's no such message at the moment).

@NTaylorMullen Why are we trying to wait for all projects to be initialized?

Ya I figured I was using it incorrectly, but once the project resolution flow started it seemed to only return once they'd all been loaded.

If we try and add a virtual document before it's corresponding project has been loaded it gets added to the Misc workspace project. Because of that it can't see any of the projects types.

What would you suggest as an alternative?

I believe it should get "promoted" to being a non-misc file if a project loads it after it's added as a misc file. If that's not the case, we should make it so. Have you actually seen it fail to promote the file?

I believe it should get "promoted" to being a non-misc file if a project loads it after it's added as a misc file. If that's not the case, we should make it so. Have you actually seen it fail to promote the file?

Seems that way, or the promotion takes a super long time. From what I looked at it seemed that only MSBuild included file are promoted once a project has been loaded. Probably missing something though. Want to sit down and person and debug?

codecov · 2018-09-29T00:21:52Z

Codecov Report

Merging #2562 into master will decrease coverage by 0.52%.
The diff coverage is 56.09%.

@@            Coverage Diff             @@
##           master    #2562      +/-   ##
==========================================
- Coverage   65.08%   64.56%   -0.53%     
==========================================
  Files          98       98              
  Lines        4248     4273      +25     
  Branches      613      620       +7     
==========================================
- Hits         2765     2759       -6     
- Misses       1304     1330      +26     
- Partials      179      184       +5

Flag	Coverage Δ
#integration	`52.92% <56.09%> (-0.22%)`	⬇️
#unit	`84.73% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
src/features/changeForwarding.ts	`62.96% <0%> (ø)`	⬆️
src/features/virtualDocumentTracker.ts	`38.09% <42.3%> (-22.78%)`	⬇️
src/features/diagnosticsProvider.ts	`78.08% <85.71%> (+2.71%)`	⬆️
src/omnisharp/requestQueue.ts	`72.72% <0%> (-16.67%)`	⬇️
src/omnisharp/utils.ts	`71.92% <0%> (-1.76%)`	⬇️
src/omnisharp/server.ts	`70.68% <0%> (-0.69%)`	⬇️
src/features/codeLensProvider.ts	`48.54% <0%> (+1.94%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37c0fa1...b128d3d. Read the comment docs.

rchande

As discussed, misc file -> proj associated needs to get implemented in omnisharp. If you remove lines 150-152 in virtualDocumentTracker this lgtm.

src/features/virtualDocumentTracker.ts

- Also fixed issue where we'd attempt to initialize virtual documents before the project world was ready. This is an issue because virtual documents don't exist as part of the MSBuild piece of the project and therefore would more-often than not get put into the miscellaneous project on the server; resulting in bad IntelliSense. - Fixed the diagnostic provider to also obey the new `virtualCSharp-*` scheme requirement - Changed the default change forwarder to ignore virtual documents to ensure that it doesn't try and trigger any changes for them until the server is ready. OmniSharp/omnisharp-vscode#dotnet#2538 aspnet/Razor.VSCode#105

NTaylorMullen · 2018-10-01T23:43:25Z

@rchande @akshita31 the failure that's happening on travis in this PR is happening in master on my Mac as well. Looks like an unrelated issue, that known?

rchande · 2018-10-02T16:11:58Z

@NTaylorMullen Is this ready to merge?

NTaylorMullen · 2018-10-02T17:02:03Z

Yup go for it.

NTaylorMullen commented Sep 29, 2018

View reviewed changes

NTaylorMullen mentioned this pull request Sep 29, 2018

Add vitual documents with a definite scheme #2552

Closed

rchande suggested changes Oct 1, 2018

View reviewed changes

NTaylorMullen commented Oct 1, 2018

View reviewed changes

src/features/virtualDocumentTracker.ts Show resolved Hide resolved

rchande approved these changes Oct 1, 2018

View reviewed changes

NTaylorMullen force-pushed the nimullen/fixvirtualdoctracker branch from b334b64 to fcdad6b Compare October 1, 2018 23:10

Merge branch 'master' into nimullen/fixvirtualdoctracker

81e7ffb

Merge branch 'master' into nimullen/fixvirtualdoctracker

b128d3d

rchande merged commit 8641dd4 into dotnet:master Oct 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Virtual document tracking apply to specific scheme. #2562

Make Virtual document tracking apply to specific scheme. #2562

NTaylorMullen commented Sep 29, 2018

NTaylorMullen Sep 29, 2018

rchande Oct 1, 2018

rchande Oct 1, 2018

NTaylorMullen Oct 1, 2018

rchande Oct 1, 2018

NTaylorMullen Oct 1, 2018

codecov bot commented Sep 29, 2018 •

edited

Loading

rchande left a comment

NTaylorMullen commented Oct 1, 2018

rchande commented Oct 2, 2018

NTaylorMullen commented Oct 2, 2018


		// We wait until project state on the server has been initialized to add any virtual documents.

		server.waitForEmptyEventQueue().then(async () => {

Make Virtual document tracking apply to specific scheme. #2562

Make Virtual document tracking apply to specific scheme. #2562

Conversation

NTaylorMullen commented Sep 29, 2018

NTaylorMullen Sep 29, 2018

Choose a reason for hiding this comment

rchande Oct 1, 2018

Choose a reason for hiding this comment

rchande Oct 1, 2018

Choose a reason for hiding this comment

NTaylorMullen Oct 1, 2018

Choose a reason for hiding this comment

rchande Oct 1, 2018

Choose a reason for hiding this comment

NTaylorMullen Oct 1, 2018

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2018 • edited Loading

Codecov Report

rchande left a comment

Choose a reason for hiding this comment

NTaylorMullen commented Oct 1, 2018

rchande commented Oct 2, 2018

NTaylorMullen commented Oct 2, 2018

codecov bot commented Sep 29, 2018 •

edited

Loading