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.

Sign up for GitHub

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

Jump to bottom

Roslyn side changes for the new toggle button added to the Error list… #11439

Merged
merged 6 commits into from
May 21, 2016
Merged

Roslyn side changes for the new toggle button added to the Error list… #11439

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
deff632
Roslyn side changes for the new toggle button added to the Error list…
mavasani May 18, 2016
7a6f4b5
Address PR feedback from Heejae - localization of strings.
mavasani May 20, 2016
ca0edce
Address PR feedback from Heejae
mavasani May 20, 2016
05594cc
Address PR feedback
mavasani May 21, 2016
18b4d37
Move to newer MS.VS.Shell.14.0 dll to consume the new error list chan…
mavasani May 21, 2016
990a60b
Move the code accessing the new IErrorList2 type into a separate meth…
mavasani May 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/VisualStudio/CSharp/Impl/Options/AdvancedOptionPageControl.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
Content="{x:Static local:AdvancedOptionPageStrings.Option_ClosedFileDiagnostics}" />
<CheckBox x:Name="RenameTrackingPreview"
Content="{x:Static local:AdvancedOptionPageStrings.Option_RenameTrackingPreview}" />
<Label x:Name="FullSolutionAnalysisMovedToErrorListLabel" Content="Note: Option &quot;Enable full solution analysis&quot; has been moved to the Error List." Visibility="Collapsed"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we give a more descriptive message here about the positioning or kind of the button (toggle button)?

Copy link
Contributor

Choose a reason for hiding this comment

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

should this be localized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, most certainly. Will do.

</StackPanel>
</GroupBox>
<GroupBox x:Uid="ExtractMethodGroupBox"
Expand Down
2 changes: 1 addition & 1 deletion src/VisualStudio/CSharp/Impl/Options/AdvancedOptionPageControl.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public AdvancedOptionPageControl(IServiceProvider serviceProvider) : base(servic
BindToOption(RenameTrackingPreview, FeatureOnOffOptions.RenameTrackingPreview, LanguageNames.CSharp);
BindToOption(DontPutOutOrRefOnStruct, ExtractMethodOptions.DontPutOutOrRefOnStruct, LanguageNames.CSharp);
BindToOption(AllowMovingDeclaration, ExtractMethodOptions.AllowMovingDeclaration, LanguageNames.CSharp);
BindToFullSolutionAnalysisOption(ClosedFileDiagnostics, LanguageNames.CSharp);
BindToFullSolutionAnalysisOption(ClosedFileDiagnostics, FullSolutionAnalysisMovedToErrorListLabel, LanguageNames.CSharp);
}
}
}
79 changes: 78 additions & 1 deletion src/VisualStudio/Core/Def/Implementation/TableDataSource/VisualStudioDiagnosticListTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
// 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.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
{
Expand All @@ -21,6 +25,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(
Expand All @@ -43,6 +50,17 @@ 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)
{
errorList2.AnalysisToggleStateChanged += OnErrorListFullSolutionAnalysisToggled;
_optionService.OptionChanged += OnOptionChanged;
}
}
}

private ITableDataSource GetCurrentDataSource()
Expand Down Expand Up @@ -130,5 +148,64 @@ private void OnErrorListPropertyChanged(object sender, PropertyChangedEventArgs
AddTableSourceIfNecessary(this.Workspace.CurrentSolution);
}
}

private void OnErrorListFullSolutionAnalysisToggled(object sender, AnalysisToggleStateChangedEventArgs e)
{
var newOptions = _optionService.GetOptions()
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
This method is handling a single option changed event and if disabled telling the error list to be turned off. We don't do anything for the reverse (option being turned on) as their might be other error list providers (say TS) that still have FSD off.

Copy link
Member

Choose a reason for hiding this comment

The 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;
}
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@jasonmalinowski jasonmalinowski May 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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, 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just use dynamic and not refer to ierrorlist2 at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the new types in error list, I'll remove them once we move to newer version of Microsoft.VisualStudio.Shell.14.0.dll

Copy link
Contributor

Choose a reason for hiding this comment

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

since this is not COM interface, is this going to work? we will not be able to cast error list to our own IErrorList2 interface.

Copy link
Contributor Author

@mavasani mavasani May 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

These types were added just so project builds. This won't work until we actually compile against newer shell binary, at which point we only need to delete these types and it should work (just minimizing potential ask mode changes)

Copy link
Contributor

Choose a reason for hiding this comment

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

This should now be deleted right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they have been deleted, this is an outdated diff.

{
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;
}
}
}
}
16 changes: 14 additions & 2 deletions src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
using System.Windows.Data;
using Microsoft.CodeAnalysis.Options;
using Microsoft.VisualStudio.ComponentModelHost;
using Roslyn.Utilities;
using Microsoft.VisualStudio.LanguageServices.Implementation.TableDataSource;

namespace Microsoft.VisualStudio.LanguageServices.Implementation.Options
{
Expand Down Expand Up @@ -98,8 +98,20 @@ protected void BindToOption(TextBox textBox, PerLanguageOption<int> optionKey, s
_bindingExpressions.Add(bindingExpression);
}

protected void BindToFullSolutionAnalysisOption(CheckBox checkbox, string languageName)
protected void BindToFullSolutionAnalysisOption(CheckBox checkbox, Label fullSolutionAnalysisMovedToErrorListLabel, string languageName)
{
// Full solution analysis option has been moved to error list from Dev14 Update3.
// We only want to show the full solution analysis option in Tools Options, if we are running against prior VS bits.
if (VisualStudioDiagnosticListTable.ErrorListHasFullSolutionAnalysisButton())
{
checkbox.Visibility = Visibility.Collapsed;
fullSolutionAnalysisMovedToErrorListLabel.Visibility = Visibility.Visible;
return;
}

checkbox.Visibility = Visibility.Visible;
fullSolutionAnalysisMovedToErrorListLabel.Visibility = Visibility.Collapsed;

Binding binding = new Binding()
{
Source = new FullSolutionAnalysisOptionBinding(OptionService, languageName),
Expand Down
1 change: 1 addition & 0 deletions src/VisualStudio/VisualBasic/Impl/Options/AdvancedOptionPageControl.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
Content="{x:Static local:AdvancedOptionPageStrings.Option_ClosedFileDiagnostics}" />
<CheckBox x:Name="RenameTrackingPreview"
Content="{x:Static local:AdvancedOptionPageStrings.Option_RenameTrackingPreview}" />
<Label x:Name="FullSolutionAnalysisMovedToErrorListLabel" Content="Note: Option &quot;Enable full solution analysis&quot; has been moved to the Error List." Visibility="Collapsed"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

same here localization?

</StackPanel>
</GroupBox>
<GroupBox x:Uid="GoToDefinitionGroupBox"
Expand Down
2 changes: 1 addition & 1 deletion src/VisualStudio/VisualBasic/Impl/Options/AdvancedOptionPageControl.xaml.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Namespace Microsoft.VisualStudio.LanguageServices.VisualBasic.Options
BindToOption(NavigateToObjectBrowser, VisualStudioNavigationOptions.NavigateToObjectBrowser, LanguageNames.VisualBasic)
BindToOption(DontPutOutOrRefOnStruct, ExtractMethodOptions.DontPutOutOrRefOnStruct, LanguageNames.VisualBasic)
BindToOption(AllowMovingDeclaration, ExtractMethodOptions.AllowMovingDeclaration, LanguageNames.VisualBasic)
BindToFullSolutionAnalysisOption(ClosedFileDiagnostics, LanguageNames.VisualBasic)
BindToFullSolutionAnalysisOption(ClosedFileDiagnostics, FullSolutionAnalysisMovedToErrorListLabel, LanguageNames.VisualBasic)
End Sub
End Class
End Namespace