-
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
Roslyn side changes for the new toggle button added to the Error list… #11439
Changes from 3 commits
deff632
7a6f4b5
ca0edce
05594cc
18b4d37
990a60b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,20 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.ComponentModel; | ||
using System.ComponentModel.Composition; | ||
using System.ComponentModel.Design; | ||
using System.Linq; | ||
using System.Reflection; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.Options; | ||
using Microsoft.CodeAnalysis.Shared.Options; | ||
using Microsoft.Internal.VisualStudio.Shell; | ||
using Microsoft.VisualStudio.LanguageServices.Implementation.TaskList; | ||
using Microsoft.VisualStudio.Shell; | ||
using Microsoft.VisualStudio.Shell.Interop; | ||
using Microsoft.VisualStudio.Shell.TableManager; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.VisualStudio.LanguageServices.Implementation.TableDataSource | ||
{ | ||
|
@@ -21,6 +26,9 @@ internal partial class VisualStudioDiagnosticListTable : VisualStudioBaseDiagnos | |
private readonly IErrorList _errorList; | ||
private readonly LiveTableDataSource _liveTableSource; | ||
private readonly BuildTableDataSource _buildTableSource; | ||
private readonly IOptionService _optionService; | ||
|
||
private const string TypeScriptLanguageName = "TypeScript"; | ||
|
||
[ImportingConstructor] | ||
public VisualStudioDiagnosticListTable( | ||
|
@@ -43,6 +51,18 @@ public VisualStudioDiagnosticListTable( | |
_errorList.PropertyChanged += OnErrorListPropertyChanged; | ||
AddInitialTableSource(workspace.CurrentSolution, GetCurrentDataSource()); | ||
SuppressionStateColumnDefinition.SetDefaultFilter(_errorList.TableControl); | ||
|
||
_optionService = workspace.Services.GetService<IOptionService>(); | ||
if (ErrorListHasFullSolutionAnalysisButton()) | ||
{ | ||
var errorList2 = _errorList as IErrorList2; | ||
if (errorList2 != null) | ||
{ | ||
workspace.WorkspaceChanged += OnWorkspaceChanged; | ||
errorList2.AnalysisToggleStateChanged += OnErrorListFullSolutionAnalysisToggled; | ||
_optionService.OptionChanged += OnOptionChanged; | ||
} | ||
} | ||
} | ||
|
||
private ITableDataSource GetCurrentDataSource() | ||
|
@@ -130,5 +150,98 @@ private void OnErrorListPropertyChanged(object sender, PropertyChangedEventArgs | |
AddTableSourceIfNecessary(this.Workspace.CurrentSolution); | ||
} | ||
} | ||
|
||
private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs e) | ||
{ | ||
Contract.ThrowIfFalse(_errorList is IErrorList2); | ||
|
||
switch (e.Kind) | ||
{ | ||
case WorkspaceChangeKind.SolutionAdded: | ||
case WorkspaceChangeKind.SolutionChanged: | ||
case WorkspaceChangeKind.SolutionCleared: | ||
case WorkspaceChangeKind.SolutionReloaded: | ||
case WorkspaceChangeKind.SolutionRemoved: | ||
case WorkspaceChangeKind.ProjectAdded: | ||
case WorkspaceChangeKind.ProjectChanged: | ||
case WorkspaceChangeKind.ProjectRemoved: | ||
// Set error list toggle state based on current analysis state for all languages for projects in current solution. | ||
var fullAnalysisState = _optionService.GetOption(RuntimeOptions.FullSolutionAnalysis); | ||
if (fullAnalysisState) | ||
{ | ||
var langauges = e.NewSolution.Projects.Select(p => p.Language).Distinct(); | ||
foreach (var language in langauges) | ||
{ | ||
if (!ServiceFeatureOnOffOptions.IsClosedFileDiagnosticsEnabled(_optionService, language)) | ||
{ | ||
fullAnalysisState = false; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
((IErrorList2)_errorList).AnalysisToggleState = fullAnalysisState; | ||
return; | ||
} | ||
} | ||
|
||
private void OnErrorListFullSolutionAnalysisToggled(object sender, AnalysisToggleStateChangedEventArgs e) | ||
{ | ||
var newOptions = _optionService.GetOptions() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just write _workspace.Options = workspace.Options.WithChangedOption(....). I've been trying to move people off from the internal APIs to public ones. |
||
.WithChangedOption(RuntimeOptions.FullSolutionAnalysis, e.NewState) | ||
.WithChangedOption(ServiceFeatureOnOffOptions.ClosedFileDiagnostic, LanguageNames.CSharp, e.NewState) | ||
.WithChangedOption(ServiceFeatureOnOffOptions.ClosedFileDiagnostic, LanguageNames.VisualBasic, e.NewState) | ||
.WithChangedOption(ServiceFeatureOnOffOptions.ClosedFileDiagnostic, TypeScriptLanguageName, e.NewState); | ||
|
||
_optionService.SetOptions(newOptions); | ||
} | ||
|
||
private void OnOptionChanged(object sender, OptionChangedEventArgs e) | ||
{ | ||
Contract.ThrowIfFalse(_errorList is IErrorList2); | ||
|
||
if (e.Option == RuntimeOptions.FullSolutionAnalysis || e.Option == ServiceFeatureOnOffOptions.ClosedFileDiagnostic) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why isn't this logic the same as the OnWorkspaceChanged code above? or even better, why not a shared method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OnWorkspaceChanged is handling the scenario where current set of project languages in solution can change - we are attempting to find if there is at least one project in solution with FSD disabled, in which case error list button will be FSD off. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but in either case, you can respond to the event with the same logic. "If one project is loaded that has the option off, it's off". Right? |
||
{ | ||
var analysisDisabled = !(bool)e.Value; | ||
if (analysisDisabled) | ||
{ | ||
((IErrorList2)_errorList).AnalysisToggleState = false; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the else case if setting sync causes this to change live? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FullSolutionAnalysis option can only be turned off by Low VM situation. we are hiding the language specific tools option from the end user (ClosedFileDiagnostic), but still ensuring that if someone turns it off via the registry key, then we change the error list button state to off. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but imagine I have an Update 2 machine syncing settings with an Update 3 machine, and that sync turns the setting back on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I suspect my concern goes away if you just unify the code with the other one.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get your point, just invoking the code within OnWorkspaceChanged will handle both cases. Let me do so. |
||
} | ||
} | ||
|
||
internal static bool ErrorListHasFullSolutionAnalysisButton() | ||
{ | ||
try | ||
{ | ||
// Full solution analysis option has been moved to the error list from Dev14 Update3. | ||
// Use reflection to check if the new interface "IErrorList2" exists in Microsoft.VisualStudio.Shell.XX.0.dll. | ||
return Assembly.GetAssembly(typeof(ErrorHandler)).GetType("Microsoft.Internal.VisualStudio.Shell.IErrorList2") != null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using reflection to ensure this light ups against new bits, but retains old behavior against prior VS bits. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this going to work? Given you call this in the constructor, the binder would have already required that type exists. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IErrorList2 is accessed in the constructor only if this method returns true, i.e. type exists. I tested it after removing the below temporary IErrorList2 interface and using the new private MS.VS.Shell.14.0 nupkg which has this interface. F5 in Roslyn dev hive shows the old 'Enable full solution analysis' options in Tools Options, and installing the Roslyn VSIX on a machine with the newer MS.VS.Shell.14.0 in GAC causes the new functionality to light up and options are hidden in Tools Options. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding (that could be very wrong) was that the JIT tried to compile your entire method as soon as you entered it, so it would demand IErrorList2 must exist before your code ever gets to the point where it's checking. Generally the pattern has been separate methods get involved, sometimes marked NoInline. But as long as you've tested, 👍. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right - seems it works for debug bits installed into devhive but not for release bits, where it tries to load the type eagerly.. need to find a different approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you just use dynamic and not refer to ierrorlist2 at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moving the code accessing IErrorList2 to a separate method and marking it with [MethodImpl(MethodImplOptions.NoInlining)] resolved this.. luckily the VSI test caught the original issue, so I'll see if the tests pass with latest commit |
||
} | ||
catch (Exception) | ||
{ | ||
// Ignore exceptions. | ||
return false; | ||
} | ||
} | ||
|
||
// TODO: Remove the below temporary types 'IErrorList2' and 'AnalysisToggleButtonStateChangedEventArgs' when we move to new version of Microsoft.VisualStudio.Shell.XX.0.dll that has these types. | ||
private interface IErrorList2 : IErrorList | ||
{ | ||
bool AnalysisToggleState { get; set; } | ||
event EventHandler<AnalysisToggleStateChangedEventArgs> AnalysisToggleStateChanged; | ||
} | ||
|
||
private class AnalysisToggleStateChangedEventArgs : EventArgs | ||
{ | ||
public readonly bool OldState; | ||
public readonly bool NewState; | ||
|
||
public AnalysisToggleStateChangedEventArgs(bool oldState, bool newState) | ||
{ | ||
OldState = oldState; | ||
NewState = newState; | ||
} | ||
} | ||
} | ||
} |
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.
"langauges"