Fix razor completion #35225
Fix razor completion #35225
Conversation
| internal sealed class InteractiveSupportsFeatureService | ||
| { | ||
| [ExportWorkspaceService(typeof(ITextBufferSupportsFeatureService), WorkspaceKind.Interactive), Shared] | ||
| internal class InteractiveTextBufferSupportsFeatureService : ITextBufferSupportsFeatureService |
There was a problem hiding this comment.
you can have 1 type with 2 interfaces with 2 ExportWorkspaceServices?
you can even share instance by exporting ExportWorkspaceServiceFactory with same service instance?
There was a problem hiding this comment.
No, you can't. Each export is a nested type itself.
|
|
||
| return CompletionChange.Create(change); | ||
| // Certain types of workspace don't support document change, e.g. DebuggerIntellisense | ||
| if (!workspace.CanApplyChange(ApplyChangesKind.ChangeDocument)) |
There was a problem hiding this comment.
I think you can use feature service from debugger workspace to disable it rather than using CanApplyChange just for debugger workspace?
There was a problem hiding this comment.
Nope, doesn't work. workspace.Services.GetService<IDocumentSupportsFeatureService>().SupportsRefactorings(document) returns true for DebuggerIntellisenseWorkspace
There was a problem hiding this comment.
sure, you need to export one for DebuggerIntellisenseWorkspace that return false? IDocumentSupportsFeatureService is per workpsace service which supposed to return different value based on workspace? like we have one for interactive and etc?
| } | ||
|
|
||
| // During an EnC session, adding import is not supported. | ||
| var encService = workspace.Services.GetService<IDebuggingWorkspaceService>()?.EditAndContinueServiceOpt; |
There was a problem hiding this comment.
I think this should move to FeatureService ?
There was a problem hiding this comment.
Should we return "CodeFix and Refactorings not available" in the ITextBufferSupportsFeatureService during EnC?
| } | ||
|
|
||
| // Certain documents, e.g. Razor document, don't support adding imports | ||
| var documentSupportsFeatureService = workspace.Services.GetService<IDocumentSupportsFeatureService>(); |
There was a problem hiding this comment.
hmmm.. why not add one specific for type import?
There was a problem hiding this comment.
I think the existing SupportRefactoring already does what I need, so I just used it. Do you prefer adding one specific to adding import? That feels too narrow to me.
| [ExportWorkspaceService(typeof(IDocumentSupportsFeatureService), ServiceLayer.Default), Shared] | ||
| internal class DefaultDocumentSupportsFeatureService : IDocumentSupportsFeatureService | ||
| { | ||
| public bool SupportsCodeFixes(Document document) |
There was a problem hiding this comment.
do we actually need one that accept Document for these? I think @dibarbet actually removed one that accept document and explicitly changed to text buffer for these APIs?
There was a problem hiding this comment.
I don't need those for my purpose, they were included just because I wanted to expose the same APIs in Feature layer. @dibarbet Any particular reason that those shouldn't be available for Document?
There was a problem hiding this comment.
I don't see any reason for them not to be available - it was changed from document -> text buffer because we had the text buffer first in almost all the cases the interface was being used, so we were often just getting the document for no reason.
| internal sealed class VisualStudioSupportsFeatureService | ||
| { | ||
| [ExportWorkspaceService(typeof(ITextBufferSupportsFeatureService), ServiceLayer.Host), Shared] | ||
| private class VisualStudioTextBufferSupportsFeatureService : ITextBufferSupportsFeatureService |
There was a problem hiding this comment.
these feels like could be shared as well?
There was a problem hiding this comment.
Not sure what do you mean. Each type can only ExportWorkspaceService once.
|
@heejaechang @rynowak this commit contains a change which I believe would fix this customer feedback . Could you please take a look? Also @rynowak I couldn't figure out a repro for the crash, could you give me some help here? How can I generate a .g.cs Razor file that reference a type from source code? |
The issue from the customer feedback is a case that needs to have a friendlier error message. You can repro this by creating an ASP.NET Core application, and then doing a refactoring that tries to change code inside a |
|
@rynowak Hmm, still couldn't repro the crash. The refactoring attempts to change the namespace, which affects all the references to enclosed types. While there's some issue with cshtml, it doesn't crash. How can I have a .g.cs file that references the type
|
|
Try the same thing with |
|
@rynowak With |
|
@heejaechang Could you please take a look at this again? |
|
Discussed with @heejaechang offline, I will merge this as is, and address his comment for 16.2 |
|
@ManishJayaswal for approval |
Fix #35172.
This also include a small change as an attempt to fix the feedback issue below. I still can't repro the crash so not able to verify the fix yet.
https://developercommunity.visualstudio.com/content/problem/514087/problem-in-namespace-refactor.html?childToView=546711#comment-546711
FYI @heejaechang
@jinujoseph @vatsalyaagrawal this needs approval.
Customer and scenario info
Who is impacted by this bug?
Razor users who turned on import completion.
What is the customer scenario and impact of the bug?
Use commit a import completion item
Expected
The type name has been committed
Actual
the document will be corrupted
What is the workaround?
User can't use import completion in Razor document, manually add imports first.
How was the bug found?
user feedback
If this fix is for a regression - what had regressed, when was the regression introduced, and why was the regression originally missed?
No. import completion is a new feature added in 16.1