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

Set the correct toggled state as soon as the solution is fully loaded #15

Merged
merged 3 commits into from
Aug 21, 2018
Merged

Set the correct toggled state as soon as the solution is fully loaded #15

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b601f41
Convert to background-loading AsyncPackage
jnm2 Jul 25, 2018
55542be
Set the correct toggled state as soon as the solution is fully loaded
jnm2 Jul 25, 2018
94715bb
Updates per review
jnm2 Aug 21, 2018
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
25 changes: 10 additions & 15 deletions Tvl.VisualStudio.JustMyCodeToggle/JustMyCodeTogglePackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,17 @@ namespace Tvl.VisualStudio.JustMyCodeToggle
using System;
using System.ComponentModel.Design;
using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.VisualStudio;
using Microsoft.VisualStudio.Shell;
using ErrorHandler = Microsoft.VisualStudio.ErrorHandler;
using IMenuCommandService = System.ComponentModel.Design.IMenuCommandService;
using Task = System.Threading.Tasks.Task;

[Guid(JustMyCodeToggleConstants.guidJustMyCodeTogglePackageString)]
[PackageRegistration(UseManagedResourcesOnly = true)]
[PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)]
[ProvideMenuResource(1000, 1)]
internal class JustMyCodeTogglePackage : Package
[ProvideAutoLoad(VSConstants.UICONTEXT.SolutionExistsAndFullyLoaded_string, PackageAutoLoadFlags.BackgroundLoad)]
Copy link
Member

Choose a reason for hiding this comment

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

💭 This seems like a reasonable context for this

internal class JustMyCodeTogglePackage : AsyncPackage
{
private readonly OleMenuCommand _command;

Expand All @@ -26,27 +29,19 @@ public JustMyCodeTogglePackage()
_command = new OleMenuCommand(invokeHandler, changeHandler, beforeQueryStatus, id);
}

public SVsServiceProvider ServiceProvider
{
get
{
return new VsServiceProviderWrapper(this);
}
}

public EnvDTE.DTE ApplicationObject
{
get
{
return ServiceProvider.GetService(typeof(EnvDTE._DTE)) as EnvDTE.DTE;
return GetService(typeof(EnvDTE._DTE)) as EnvDTE.DTE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's either GetService, or JTF.Run on GetServiceAsync in BeforeQueryStatus and InvokeHandler.

Copy link

Choose a reason for hiding this comment

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

I believe it will be called on the UI thread, so GetService is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And if somehow it wasn't called on the UI thread, we'd still need to switch to the UI thread because DTE isn't free-threaded, correct? So pretty much no matter what, this needs to be GetService?

Copy link

Choose a reason for hiding this comment

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

I've been using DTE on non-UI threads for years and it generally doesn't care. If you're being pedantic you should assume it isn't free-threaded. 😉

}
}

protected override void Initialize()
protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
{
base.Initialize();
Copy link
Member

Choose a reason for hiding this comment

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

💡 It appears code typically calls the base implementation, but it's not sure if this is required:

await base.InitializeAsync(cancellationToken, progress);

// Rest of initialization ...

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 had checked that it was returning CompletedTask, but the very fact that I had to check makes me suspicious of looking in the first place. Funny enough, this was the last PR before I started leaving base calls in my overrides. :D

await JoinableTaskFactory.SwitchToMainThreadAsync();
Copy link
Member

Choose a reason for hiding this comment

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

❗️ Propagate cancellationToken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good, thanks. ReSharper usually tells me about this. :'(


var mcs = (IMenuCommandService)GetService(typeof(IMenuCommandService));
var mcs = (IMenuCommandService)await GetServiceAsync(typeof(IMenuCommandService));
Copy link
Member

@sharwell sharwell Aug 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

❗️ Use Assumes.Present after GetServiceAsync:

var mcs = (IMenuCommandService)await GetServiceAsync(typeof(IMenuCommandService));
Assumes.Present(mcs);

mcs.AddCommand(_command);
}

Expand Down