-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Disable document outline #65688
Disable document outline #65688
Conversation
src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsCodeWindowManager.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsCodeWindowManager.cs
Outdated
Show resolved
Hide resolved
This feels weird to me. Say someone was using the feature. So they have the outlining window open. Now VS will launch and this will just be empty. Won't they think something is broken here? How would they know they need to enable this feature to get things working again? |
#65641 is expected to be the final resolution to this. this is a stop gap to unblock users who have the document outline window opened for other reasons (such as XAML) and who currently have no other recourse. |
I guess my point is: can we make this less confusing for users who run into this? For example, we could put a message (or button) in the ui explaining that this is not working for C#/VB until the option is explicitly enabled. |
Not a bad idea, I'll ask what the rules are for getting a new translation in for that text. Question would be what we do if the translation team couldn't turn it around in time for the servicing release. Would we want to wait for the next servicing release? I don't think we are allowed to release un-translated strings in a servicing patch. |
@dotnet/roslyn-infrastructure it appears that integration tests in the release/17.4 branch are failing consistently I assume those failures should not block merge. |
At this point we can't add new strings in a servicing update. I am open to options here but I think if we want to insert this now we should do so as is. |
missed window for this to be relevant |
@arkalyanms I believe integration tests are broken in the release/dev17.4 branch so I assume this is good to go once its reviewed |
@jasonmalinowski We've decided we'd like to take this change for servicing. Can you review? |
private const string FeatureName = "DocumentOutlineOptions"; | ||
|
||
public static readonly Option2<bool> EnableDocumentOutline = new(FeatureName, nameof(EnableDocumentOutline), defaultValue: false, | ||
storageLocation: new FeatureFlagStorageLocation("Roslyn.DocumentOutline")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we're disabling the feature entirely? there's no UI to get it back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
workaround while we improve performance in #65641
CC: @allisonchou this a fix we want to get into the next servicing release.