Set the correct toggled state as soon as the solution is fully loaded #15
Conversation
| public EnvDTE.DTE ApplicationObject | ||
| { | ||
| get | ||
| { | ||
| return ServiceProvider.GetService(typeof(EnvDTE._DTE)) as EnvDTE.DTE; | ||
| return GetService(typeof(EnvDTE._DTE)) as EnvDTE.DTE; |
There was a problem hiding this comment.
It's either GetService, or JTF.Run on GetServiceAsync in BeforeQueryStatus and InvokeHandler.
There was a problem hiding this comment.
I believe it will be called on the UI thread, so GetService is fine.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😉
| using IMenuCommandService = System.ComponentModel.Design.IMenuCommandService; | ||
| using Task = System.Threading.Tasks.Task; | ||
|
|
||
| [Guid(JustMyCodeToggleConstants.guidJustMyCodeTogglePackageString)] | ||
| [PackageRegistration(UseManagedResourcesOnly = true)] |
There was a problem hiding this comment.
I think this should be:
[PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)]AllowsBackgroundLoading = true is like a master switch that allows the package to load on a b/g thread. PackageAutoLoadFlags.BackgroundLoad is just a hint for that particular ProvideAutoLoad.
| public EnvDTE.DTE ApplicationObject | ||
| { | ||
| get | ||
| { | ||
| return ServiceProvider.GetService(typeof(EnvDTE._DTE)) as EnvDTE.DTE; | ||
| return GetService(typeof(EnvDTE._DTE)) as EnvDTE.DTE; |
There was a problem hiding this comment.
I believe it will be called on the UI thread, so GetService is fine.
| { | ||
| base.Initialize(); | ||
| await JoinableTaskFactory.SwitchToMainThreadAsync(); |
There was a problem hiding this comment.
❗️ Propagate cancellationToken
There was a problem hiding this comment.
Good, thanks. ReSharper usually tells me about this. :'(
|
|
||
| var mcs = (IMenuCommandService)GetService(typeof(IMenuCommandService)); | ||
| var mcs = (IMenuCommandService)await GetServiceAsync(typeof(IMenuCommandService)); |
There was a problem hiding this comment.
❗️ Use Assumes.Present after GetServiceAsync:
var mcs = (IMenuCommandService)await GetServiceAsync(typeof(IMenuCommandService));
Assumes.Present(mcs);| [ProvideMenuResource(1000, 1)] | ||
| internal class JustMyCodeTogglePackage : Package | ||
| [ProvideAutoLoad(VSConstants.UICONTEXT.SolutionExistsAndFullyLoaded_string, PackageAutoLoadFlags.BackgroundLoad)] |
There was a problem hiding this comment.
💭 This seems like a reasonable context for this
| { | ||
| base.Initialize(); |
There was a problem hiding this comment.
💡 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 ...There was a problem hiding this comment.
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
|
@sharwell Updated. |
| @@ -1,19 +1,24 @@ | |||
| // Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. | |||
| // Licensed under the MIT License. See LICENSE.txt in the project root for license information. | |||
|
|
|||
| using Microsoft; | |||
There was a problem hiding this comment.
📝 This should have been inside the namespace, but it also should have been detected by the build. I'll fix it in a follow-up pull request where I enable StyleCop Analyzers.
Fixes #13.
/cc @sharwell, @jcansdale