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.

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

Merged
merged 6 commits into from
May 21, 2016

Conversation

mavasani
Copy link
Contributor

… to turn on/off full solution analysis

We are moving the "Enable full solution analysis" option from Tools Option to the error list for improved discoverability, while also making it language agnostic. This change ensures that we set the Roslyn full solution analysis options when this button is toggled in the error list. Additionally, when we identify that the error list toggle button is available (Dev14 U3 or later), we will hide the tools option checkbox for "Enable full solution analysis" and show a note about it being moved to the error list. Note that we still continue with language specific defaults (C# off, VB on), but once the user has hit the error list toggle, the option setting is consistent across all languages.

Addresses workitem #11392

… to turn on/off full solution analysis

We are moving the "Enable full solution analysis" option from Tools Option to the error list for improved discoverability, while also making it language agnostic. This change ensures that we set the Roslyn full solution analysis options when this button is toggled in the error list. Additionally, when we identify that the error list toggle button is available (Dev14 U3 or later), we will hide the tools option checkbox for "Enable full solution analysis" and show a note about it being moved to the error list. Note that we still continue with language specific defaults (C# off, VB on), but once the user has hit the error list toggle, the option setting is consistent across all languages.

Addresses workitem dotnet#11392
@mavasani
Copy link
Contributor Author

Tagging @heejaechang @srivatsn @dotnet/roslyn-analysis for review.

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

@heejaechang
Copy link
Contributor

except localization and interface stuff, 👍

@mavasani
Copy link
Contributor Author

@heejaechang fixed localization. will remove interfaces once we have NuGet package for new shell binary

@srivatsn
Copy link
Contributor

@jasonmalinowski @Pilchie @DustinCampbell to review changes to options. Also tagging @paulvanbrenk since this will impact TypeScript.

@paulvanbrenk
Copy link
Contributor

paulvanbrenk commented May 20, 2016

@srivatsn where does this impact TypeScript? We don't have an option.... or do we now get one 'for free'

@natidea
Copy link
Contributor

natidea commented May 20, 2016

If someone is working on a TypeScript project, will they see this button in the Error list (even though it does not do anything since TypeScript always does FSD)?

@srivatsn
Copy link
Contributor

@paulvanbrenk If you don't have an option then there's no impact. @natidea Yes they'll still see the button. It's not just typescript. Even if I have nothing open, turning on\off the option just means try harder and some provider will and some won't.

@mavasani mavasani force-pushed the Issue11392 branch 2 times, most recently from f5ae74b to 8d11ab8 Compare May 20, 2016 23:30
@mavasani
Copy link
Contributor Author

@srivatsn @heejaechang - I have updated the PR as per today's discussion. Only pending part is moving to new shell nupkg, after which I'll merge this change.

if (fullAnalysisState)
{
var langauges = e.NewSolution.Projects.Select(p => p.Language).Distinct();
foreach (var language in langauges)
Copy link
Member

Choose a reason for hiding this comment

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

"langauges"

@mavasani
Copy link
Contributor Author

retest vsi please

…od marked with [MethodImpl(MethodImplOptions.NoInlining)]
@mavasani
Copy link
Contributor Author

retest vsi please

@mavasani
Copy link
Contributor Author

All tests passed and verified manually too.

@mavasani mavasani merged commit d55123f into dotnet:master May 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants