vsixcommunity / community.visualstudio.toolkit Goto Github PK

The new Commands class that lets you execute a command by name and the overloads for VSConstants.VSStd97CmdID and so on are fantastic, but knowing which enum the command you want to execute is defined in makes it a bit painful to use.

I think a KnownCommands class that defines the CommandID objects for all commands that Visual Studio defines would be great. I've created a proof-of-concept here: master...reduckted:feature/known-commands

The good thing is that the CommandIDs for all 800+ commands that Visual Studio provides can be generated quite easily (see the PowerShell script).

Here's what you see in intellisense:

There's still a few things to improve, but I thought I'd get some opinions before I go any further.

I'm looking to see whether the Solution Explorer contains any items. See here:

https://github.com/reduckted/ProjectFilter/blob/b1e2681b41086fb3fba0c65774202bf593deb576/source/ProjectFilter/Services/SolutionLoadObserver.cs#L44-L55

What I'm doing might be a bit of a hack, but I'm executing a bunch of commands that operate on the Solution Explorer tool window to hide unloaded projects. If Solution Explorer hasn't been populated, then I wait a bit and try again.

I also need to activate the Solution Explorer window (see here) to allow those commands to work, and get the selection (see here) so that I can restore the selected items afterwards.

I see there's an IVsSolutionUIHierarchyWindow, but it doesn't seem to provide access to the hierarchy in the window, and I can't find a corresponding SVsSolutionUIHierarchyWindow type.

@bluetarpmedia do you have an idea on how to do that?

Please rename ForgetAndLogOnFailure() back to FireAndForget() as it was the simplest and most elegant name for the method.

For a toolkit that's aiming to simplify and make writing extensions easier, a name like ForgetAndLogOnFailureviolates violates those base tenets IMO. The original name FireAndForget should have been kept, with a logOnFailure parameter added as @reducted suggested, as this makes the most sense and caters to developers who are new to writing extensions as well as users with more advanced logging requirements.

Can we PLEASE just go back to FireAndForget? It's in a bunch of YouTube videos, and ForgetAndLogOnFailure () will only confuse/annoy people.

Mads: It was @AArnott that suggested renaming it to more accurately reflect what the method was doing. It's not just "forgetting" a task, it handles the errors and logs to the output window. I like the idea of adding an optional parameter that allows you to disable the output window logging. I'm personally OK with the name FireAndForget(), but there are various opinions on this. Perhaps we can come up with a list of names to choose from

Yann: With all due respect to Andrew, just because he suggested renaming it doesn't mean it has to be renamed. Andrew works with very technical details, so yes, he would think that way. It's great that Andrew's on board to do code reviews, to point out any technical flaws in our code, or any bad practices, but overly complicating names just to cover everything a method does is not what I thought this toolkit was about.

We're creating a Community Toolkit to make writing extensions easier. Nobody really needs to know the technical details behind that particular method. Users just need to know that they can use it to fire an async method and forget about it (and the technical details are handled for them under the hood). That's what we've been trying to do, abstract the technical/difficult-to-know-about details under a simplified facade. The analyzers can direct people to use this method instead of the others, and can give a small explanation about the logging option at the same time. It's simple and elegant. Your first instinct was a good one!

For more advanced users, the addition of the parameter allows control of the logging, if required - without confusing users who don't care about logging any errors.

I am also (obviously) fine with using FireAndForget(). And as you may have possibly guessed, I feel VERY strongly about this._

Is the "Extensibility" project type ({82B43B9B-A64C-4715-B499-D71E9CA2BD60}) missing from the list of types in ProjectTypes, or is that GUID considered a sub-type and belongs somewhere else?

I came across the following while reviewing WindowGuids. The "Find and Replace" window (shown below) appears to have a GUID {6324226F-61B6-4F28-92EE-18D4B5FE1E48}. There are currently a few GUIDs related to Find, but I'm not sure what window they represent. I also could not find a reference to this GUID in the docs.

Should this Find and Replace be added to WindowGuids and if so, what should the property name be? FindReplace2?

The .editorconfig contains this:

dotnet_diagnostic.VSTHRD003.severity=silent
dotnet_diagnostic.VSTHRD104.severity=silent
dotnet_diagnostic.VSTHRD111.severity = none

I think that should all be removed and any underlying issues in the code that produce these warnings should be fixed, or the warning should only be suppressed around the specific code/method with a justification.

I removed them and did a rebuild and the only vs-threading analyzer warning was VSTHRD003 in the public static void FireAndForget(this System.Threading.Tasks.Task task) extension method, so at the very least I think the 104 and 111 analyzers can be re-enabled.

I'm unsure whether the VSTHRD003 warning is a problem in this case. The task has already started before the extension method is called, and it has a different ExecutionContext to the FireAndForget extension method, and that appears to be exactly what VSTHRD003 is warning about.

See: https://github.com/microsoft/vs-threading/blob/main/doc/analyzers/VSTHRD003.md

(Where it talks about foreign tasks from another context I believe it's referring to ExecutionContext -- i.e. the task started on a different stack frame.)

@AArnott Any chance you could comment on this please?

public static void FireAndForget(this System.Threading.Tasks.Task task)
{
    ThreadHelper.JoinableTaskFactory.RunAsync(async () =>
    {
        try
        {
            await task.ConfigureAwait(false);  <---- VSTHRD003 here
        }
        catch (Exception ex)
        {
            await ex.LogAsync();
        }
    });
}

The interface to implement is IVsTrackProjectDocuments2

I have a package that inherits from AsyncPackage
with
[PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)]

Which UI context do I need to set to make sure that my packages gets loaded before the fist solution is loaded so I can register the OnBeforeOpenSolution event in time to see the event for the first solution that is opened.
I am registering the event from a method that gets called from InitializeAsync() like this:
VS.Events.SolutionEvents.OnBeforeOpenSolution += SolutionEvents_OnBeforeOpenSolution;
I tried all kind of contexts such as

• VSConstants.UICONTEXT.SolutionExists_string
• VSConstants.UICONTEXT.EmptySolution_string
• VSConstants.UICONTEXT.ShellInitialized_string

but nothing seems to work .
I have 2 packages

1. A project system for a custom language
2. A language service for a custom language

The project system seems to be initialized first (which makes sense because the project file is opened before the source files) but when the first project of our custom language is opened then the solution is already open and the OnBeforeOpenSolution() does not get fired.

I noticed the project templates create the older project system style that targets .NET 4.7.2. Are there plans to move this to .NET (Core) 5+? Also, is there any news on moving the VS SDK to .NET (Core) 5+?

For instance, I noticed Microsoft.VisualStudio.GraphModel doesn't appear to have a .NET Standard/Core offering.

Although it's not too difficult to use, creating a WritableSettingsStore is a bit cumbersome, and certainly not easy to discover. Here's my proposed API for a wrapper class. It borrows some techniques from the CommandBase class.

First, an attribute that defines the "collection path" to use.

[AttributeUsage(AttributeTargets.Class)]
public sealed class SettingsCollectionAttribute : Attribute {
    public SettingsCollectionAttribute(string collectionPath);
}

And then the wrapper base class:

public abstract class UserSettings<T> where T : class, new()
{
    public async static Task<T> GetAsync();

    protected bool GetBoolean(bool defaultValue = false, [CallerMemberName] string? propertyName = null);
    protected void SetBoolean(bool value, [CallerMemberName] string? propertyName = null);

    protected int GetInt32(int defaultValue = 0, [CallerMemberName] string? propertyName = null);
    protected void SetInt32(int value, [CallerMemberName] string? propertyName = null);

    protected long GetInt64(long defaultValue = 0, [CallerMemberName] string? propertyName = null);
    protected void SetInt64(long value, [CallerMemberName] string? propertyName = null);

    protected string GetString(string defaultValue = "", [CallerMemberName] string? propertyName = null);
    protected void SetString(string value, [CallerMemberName] string? propertyName = null);

    protected void ClearValue([CallerMemberName] string? propertyName = null);
}

The idea being that you inherit from UserSettings and provide properties that call the Get... and Set... methods. For example:

[SettingsCollection("ProjectFilter_ed6f0249-446a-4ddf-a8e8-b545113ba58f")]
public class ExtensionSettings : UserSettings<ExtensionSettings> {
    public bool LoadProjectDependencies {
        get { return GetBoolean(defaultValue: true); }
        set { SetBoolean(value); }
    }
}

And getting an instance is as simple as:

var settings = await ExtensionSettings.GetAsync();
settings.LoadProjectDependencies = true;

Thoughts? If people are happy with the design, then I'll submit a PR.

Understanding this toolkit is an early concept, and the fact that WinUI 3 has not RTMed, are there thoughts around adding support for WinUI views/controls to this toolkit?

This looks quadratic to me because List.Contains is O(n) and the results List grows in size to equal (or get close to) the size of items.

List<IVsHierarchyItem> results = new();

...

VSITEMSELECTION[] items = new VSITEMSELECTION[itemCount];
multiSelect.GetSelectedItems(0, itemCount, items);

foreach (VSITEMSELECTION item in items)
{
    IVsHierarchyItem? hierItem = await item.pHier.ToHierarcyItemAsync(item.itemid);
    if (hierItem != null && !results.Contains(hierItem))
    {
        results.Add(hierItem);
    }
}

Is the results.Contains necessary? Is there a scenario where GetSelectedItems returns duplicates?

If so, perhaps the method can add to results unconditionally and after the foreach completes, remove duplicates at the end.

Using the following code:

var events = Community.VisualStudio.Toolkit.VS.Events;
events.DocumentEvents.DocumentSaved += DocumentEvents_DocumentSaved;

Generates this exception when run in VS2022:

Exception thrown: 'System.MissingMethodException' in Test.dll
Method not found: 'Void EnvDTE._dispDocumentEvents_DocumentSavedEventHandler..ctor(System.Object, UIntPtr)'.

This happens for all the events.

The scenario I have is that It seems that I do not need to split my extension, the only Issue I have is that I was using the DTE to get the project events.

The solution I thought was that I will use the 16 version of the toolkit for those events and my extension will be compatible with 16 and 17.

However while it is the case that the Build events work fine, I am seeing it fail in VS17 because of the Events.WindowEvents being Initalized when I access the Events object to get a handle on the BuildEvents.

If the various branches of the hierarchy initalized lazily, i.e. only when they were used, then I would not need to split my extension unless I strayed into the areas that forced a split.

Infobar.cs doesn't match the format used in related methods. For instance, CreateInfoBar.

Any thought around including DGML, how to simplify, and different use-cases?

For instance (and with limited knowledge if there are other alternatives), would it be appropriate to use DGML to create different UI like a TreeView of custom elements (similar to Solution Explorer)?

Came across this repo that attempts to simplify.
https://github.com/merijndejonge/DgmlBuilder

References

https://docs.microsoft.com/en-us/visualstudio/modeling/directed-graph-markup-language-dgml-reference?view=vs-2019

The call to IMenuCommandService.AddCommand requires the main/UI thread so if the client is not already on the main thread when calling BaseCommand.InitializeAsync then AddCommand will hang.

I can't find the guid in the regular lists

Mads,

I like the idea of the Community Toolkit a lot, but I am hesitating to use it for our projects for the following reason:

we had a dependency in our extension to Mono.Cecil and everything worked great until one user used it on a VS installation that also had Xamarin installed. It appeared that the Xamarin team had used an older version of Mono.Cecil, and even though we had clearly indicated using a ProvideCodeBase attribute that we depended on a specific version of Mono.Cecil and we had included that version in our extension, Vs still decided to load the older version of Mono.Cecil which broke our code.
We resolved that now by adding "our own version" of mono.cecil with another assembly name.

How do we prevent a similar problem with the Community toolkit?
I monitored some of the commits from the last month and I see several commits that make breaking changes.
If we had a dependency on the toolkit and another extension has a dependency to a different version then there would be same problems again.
I see only 2 solutions:

• The Toolkit code never makes breaking changes
• We include the toolkit in our code and create our own assembly from it.

How do you look at this ?

Robert

When saving a property that is a bool, the BaseOptionModel should store the value as a boolean using WritableSettingsStore.SetBool() instead of using a binary serializer and storing it as a string.

The same should occur to int, uint, long, ulong and string.

Status bar is usually two words the majority of time and references should follow the camel-casing standard. It is also used in this format under the StatusBar API.

The update would include renaming Statusbar.cs file (to StatusBar.cs) along with related methods like GetStatusbarTextAsync (to GetStatusBarTextAsync).

I'm using the Blue theme in Visual Studio. If I add a TextBlock containing text to ThemeWindowDemo.xaml as follows

            <TextBlock Margin="{StaticResource Margin}">
                <TextBlock Text="Click here: " />
                <Hyperlink>Interesting link</Hyperlink>
            </TextBlock>

The TextBlock is barely legible. I suspect it is rendering as almost-white text on white background (sample image below). Would it be appropriate for TextBlocks to be handled by Themes.UseVsTheme?

DTE.Version, but as a proper .NET Version class

It would be great if the build done event passed along whether the build succeeded or failed.

i.e. the int fSuccess or is there another easy way to determine this from what is returned?

DTE.ToolWindows

Build events are never raised.
Looks like IVsUpdateSolutionEvents.UpdateSolution_Begin is called by VS, but only IVsUpdateSolutionEvents2.UpdateSolution_Begin forwards the event.

Bear with me as this is asking to change a few things...

I was trying to review/use the Selection and Solutions code to become familiar with what's under the hood. There are some core things that don't appear to work, but could probably be something I'm overlooking (as usual 😄).

For example, VS.Solutions.GetActiveProjectAsync() will return null even when I have a project added/opened/selected. I believe the reason is because it is really a SolutionItem and the result comes from using the as Project when returning.

I've also found it a little confusing when using Selection and Solutions as they both are related to getting items from the solution (for the most part). Plus, there is some overlap. For example, VS.Selection.GetSelectedItemsAsync() and VS.Solutions.GetActiveSolutionItemAsync() both call VS.Selection.GetSelectedHierarchiesAsync() under the hood.

In addition, the wording for Selection seems too open-ended as you could eventually have selections for other things other than the solution.

I've not had a chance to learn more about the Events (Selection and Solution), but they also appear to have some common pieces (like SelectionChangedEventArgs which use SolutionItem).

With that, I recommend we consolidate with the initial changes below. This wouldn't change logic functionality at first, but help simplify/prepare to address some of the broken areas (like GetActiveProjectAsync() mentioned above).

• Rename (current) Solution.cs to SolutionRoot.cs as it is a SolutionItem
• Rename Solutions.cs to Solution.cs
• Move Selection.cs code to Solution.cs
• Deprecate Selection.cs file
• Consolidate SelectionEvents.cs to SolutionEvents.cs or at least move SelectionEvents.cs under Solution folder for now.
• Other?

I started a possible PR, but since this is asking for fundamental changes, I wanted to get feedback before attempting. Hopefully this make sense. Thoughts?

The VS.Services.GetCommandServiceAsync() method does not seem to ever work because the service is never found.

I don't know why, but it seems that the IMenuCommandService can only be retrieved from a Package and not from the global service provider.

IMenuCommandService fromGlobal = await VS.GetServiceAsync<IMenuCommandService, IMenuCommandService>();
IMenuCommandService fromPackage = await this.GetServiceAsync<IMenuCommandService, IMenuCommandService>();
// `this` is a Package object.

await VS.MessageBox.ShowAsync(
    "From Global: " + ((fromGlobal is null) ? "null" : "not null") + "\r\n" +
    "From Package: " + ((fromPackage is null) ? "null" : "not null")
);

Do we really need to send cookie = 0 in each invocation of statusBar.Progress? If I understand status bar API correcty, I think we need to reuse same value of the cookie between our progress change invocation.

So we need to grab an our new cookie:

uint _cookie = 0; //zero matters!
_statusBar.Progress(ref _cookie, 1, "", 0, 0);

and then reuse it until our progress has finished. I really don't know what way is 100% correct, can anyone shed light on it?

if I right about reusing cookie value, I would suggest the following API:

//in VS.Notification we put:
        public async Task<StatusBarProgress> StartStatusBarAnimationAsync()
        {
            await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
            IVsStatusbar? statusBar = await GetStatusBarAsync();

            return new StatusBarProgress(statusBar);
        }

        public class StatusBarProgress : IDisposable
        {
            private readonly IVsStatusbar _statusBar;
            private uint _cookie;

            public StatusBarProgress(IVsStatusbar statusBar)
            {
                ThreadHelper.ThrowIfNotOnUIThread();

                _statusBar = statusBar ?? throw new ArgumentNullException(nameof(statusBar));

                _statusBar.FreezeOutput(0);
                _statusBar.Progress(ref _cookie, 1, "", 0, 0); //grab our cookie
            }

            public void SetProgress(string text, int currentStep, int numberOfSteps)
            {
                ThreadHelper.ThrowIfNotOnUIThread();
                _statusBar.Progress(ref _cookie, 1, text, (uint)currentStep, (uint)numberOfSteps);
            }

            public void Dispose()
            {
                ThreadHelper.ThrowIfNotOnUIThread();
                _statusBar.Progress(ref _cookie, 0, "", 0, 0); //please make note that fInProgress = 0; it works because _cookie is not zero
            }
        }

//using it:
            using (var statusBarAnimation = await VS.Notifications.StartStatusBarAnimationAsync())
            {
                statusBarAnimation.SetProgress("1", 1, 3); //show first progress step
                await Task.Delay(1000);
                statusBarAnimation.SetProgress("0", 0, 3); //hide our progress for a second
                await Task.Delay(1000);
                statusBarAnimation.SetProgress("2", 2, 3); //reshow it
                await Task.Delay(1000);
            }

In this design we allowed to use currentStep = 0 to temporarily hide progress, because animation stops in the Dispose method. Also, both examples in the last episode demonstrates same behaviour with cookie.

any thoughs?
Thanks guys!

This is used in the SolutionItem class and I have no idea how to do it without the DTE

The following seems like a very unlikely scenario but should there be protection for this?

BaseToolWindow<T>.Initialize calls ToolkitPackage.AddToolWindow which lazily creates the _toolWindowProviders list and then adds the provider to it.

If the extension author calls Initialize for different tool windows from different threads, then the threads will race to create the list and write to it.

Since it is a partial class, would it make it clearer if 'Dialog' (or 'MessageBox') was added to the end of the Show methods? For example...

VS.Notifications.ShowConfirmDialog("title", "message");
VS.Notifications.ShowErrorDialog("title", "message");
VS.Notifications.ShowMessageDialog("title", "message");
VS.Notifications.ShowWarningDialog("title", "message");

The idea is that you should be able to get to the selected folder view items much in the same way you get the selected solution items.

The project type guid constants are a great start. I think some of them might be slightly off on their meaning (for example, the guid associated with the WAP constant is really just a C# project) and there are a ton more that can be added that I saw here.

I can open a PR if it's deemed useful to add them.

This requires investigation since I'm not sure how to do this. Also, can't easily find how to from GitHub code searches

I could be misinterpreting how this should work, but seems like the solution item would also return when selecting along with other items. Use image below as example.

✔️ : It returns the solution item and a count of 1 if I only select the solution item.
✔️ : It returns the folder item and a count of 1 if I only select the folder item.
✔️ : It returns both folder items (2nd folder not shown above) and a count of 2 if I select both folder items.
❌ : It returns the folder item and a count of 1 if I select the solution item and the folder item. Seems like it would also return the solution item and a count of 2.

I've added other items (folders, projects, etc.) and get same results. I believe the error occurs within this if statement as the hierItem is returned null for the solution item and doesn't get added to results.

Each event class should be located under their respective folders. The SolutionEvents.cs class should be located in the Solution folder etc.

@reduckted, @madskristensen

Scenario:

I wanted to convert some of my projects to use the Community Toolkit instead of my Luminous Code NuGet package. I want to use ToolkitPackage to benefit from the added tool window features.

My current commanding system relies on inheritance to achieve a single responsibility for each inherited class. It also has a far more intuitive way of handling BeforeQueryStatus, but I'll be doing a pull request to suggest using something similar.

In my extensions, I have an abstract base class (because it never get instantiated) which I call PackageCommand, which inherits from AsyncCommandBase. It has some logic in it that I want all of my commands to have available, as it checks in the package's options to see if all of the extension's commands should be enabled or not.

Note: the AsyncCommandBase is not currently really async, it was a work in progress

I then have additional base command classes for each group of commands that I want to enable/disable that inherit from that PackageCommand class. Let's say that I have two groups of commands, so GroupOneCommand and GroupTwoCommand. Each of those classes will will add some logic to look up the package's options to see if all of the commands in the group should be enabled or not.

Problems:

1. You can't currently inherit from BaseComamnd<T> (I don't see any way to do so due it being generic, or at the very least not the way I need to).
2. With the current implementation of BaseComamnd<T> I'd end up repeating the same code over and over (the whole reason that I wrote Luminous Code was to reduce repeated code).

We need to ensure that an assembly and file version is added to the assembly (probably in an AssemblyInfo.cs class) in each nuget package, such that multiple versions can be loaded by VS.

We need it to avoid this madskristensen/Tweakster#76 (comment)

@CZEMacLeod any idea how to do that best?