madskristensen / Community.VisualStudio.Toolkit

A community toolkit for writing Visual Studio extensions
Other
24 stars 3 forks source link

Make VSCT menu IDs more discoverable #21

Closed reduckted closed 3 years ago

reduckted commented 3 years ago

I don't know about anyone else, but names like IDG_VS_WNDO_OTRWNDWS1 aren't exactly clear about what they refer to. 🤣

It would be great if this package could some how define aliases for these IDs so that they were more discoverable.

For example, IDG_VS_WNDO_OTRWNDWS1 could be Group_View_OtherWindows.

madskristensen commented 3 years ago

Yes, I've had similar thoughts. I'm not sure how to do it, there are several ways we could. One is to use a different file extension that then generates a .vsct file on save

yannduran commented 3 years ago

What about a one-time conversion to a .vsct file that can come with the template, and be referenced with an Include?

Like <Include href="KnownGroupIds.vsct" /> (or some better name).

Generating a vsct on save means you'd still have to know the original archaic names.

madskristensen commented 3 years ago

@yannduran dynamically generating a .vsct from another XML file that is identical, with the exception that it gives intellisense for easy-to-understand names fixes the issue with archaic names. In fact, we could come up with a whole new scheme for VSCT and express it in JSON or something

madskristensen commented 3 years ago

@yannduran now I understand what you mean with a translation we can <include>. That will add some weight to the .vsix file, but it would be the simplest solution and one that doesn't require any dependency on other tooling than the template pack. My only concern is that it compiles the whole included .vsct file in and I wonder what that will do to the command table at runtime.

madskristensen commented 3 years ago

I've implemented the concept of aliasing the global symbols with friendly names in the test project. See the VSGlobals.vsct and how it is consumed from VSCommandTable.vsct.

It doesn't seem to increase the size of the .vsix, so it's not compiling the duplicate aliases into the .dll. Awesome! When we have the full list, we can add it to the template pack and add Intellisense for it in the VSCT Intellisense extension.

This is super promising and if we get this right, we will solve one of the biggest and oldest issues for extension authors. This is huge!!

yannduran commented 3 years ago

@madskristensen that looks great! I noticed however that you've missed the entire set of View.OtherWindows values, just FYI. And that's one of the most mysterious and hard to use ID's.

Is there any way to enforce/suggest that the Extensibility Essentials extension be installed, from from the Toolkit NuGet package?

If so, it would be great for making sure that new developers have it installed. If not, without at least VSCT Intellisense installed, our non-archaic names won't be visible.

yannduran commented 3 years ago

The path

C:\Program Files (x86)\Microsoft Visual Studio\**2019**\**Preview**\VSSDK\VisualStudioIntegration\Common\Inc\vsshlids.h

has both 2109 and Preview in it. It could be very frustrating for a developer whose reading the comment to not be able to find it if they don't have VS Preview installed, or the version isn't 2019 anymore.

madskristensen commented 3 years ago

I've updated the VSCT Intellisense extension to support any .vsct files included. It now looks like this when parenting a button to View.OtherWindows:

image

Get the CI build of VSCT Intellisense to try out for yourself.

There's still missing menu elements that I need to add, but most of them are there.

yannduran commented 3 years ago

@madskristensen why do you have View.DevWindows.OtherWindows instead of just View.OtherWindows? Surely, the DevWindows part isn't necessary as it's not very intuitive.

madskristensen commented 3 years ago

Because you may want to add you button to the group called View.DevWindows which contains the Other Windows menu. Or perhaps you want to add your own group to View.DevWindows.OtherWindows instead of using one of the predefined groups 0-6.

It's convention based. It always starts with a <Menu> then a dot and then a <Group> then a dot and then a <Menu> etc. The biggest thing missing today is the hierarchical information of what groups are in which menus etc. This convention solves that

yannduran commented 3 years ago

Maybe the convention needs to be <SomeMenu>.<SomeGroup>.<SomeMenu>, with suffixes, as in ViewMenu.DevWindowsGroup.OtherWindowsMenu so the convention is more obvious.

madskristensen commented 3 years ago

Yes, having suffixes will be more clear. It just looks a little messy to me.

How about showing a tooltip for each entry to show the actual location? Btw, this is actual implementation I'm working on and not a photoshopped image.

image

StevenRasmussen commented 3 years ago

@madskristensen... um... that looks AWESOME! That would be extremely helpful in trying to figure out the correct location!

madskristensen commented 3 years ago

Here are a few more

image

image

yannduran commented 3 years ago

I think it's a good concept, however, you're going to have names like MainMenu. and CodeWindow. in the intellisense names, but you're displaying names like VSMain and VSEditor instead (at least in your first image). The rest of the images make more sense.

I do think that the font size needs to come down a bit though.

Oh and DevWindows makes more sense now that I can see where it fits!

yannduran commented 3 years ago

Yes, the suffixes do look a bit messy, but I think the clarity that they bring makes them worth considering.

That's why the original horrendous prefixes were in the names in the first place; to know which where groups and which were menus. Unfortunately they were condensed so much as to make them often illegible to developers. I'm sure they made perfect sense to the C++ programmers at the time that they added them though.

Maybe your images will counter the loss of knowing which ones are menu names and which ones are group names. My concern was that by losing that distinction it was relying on convention a little too much. But then again maybe intellisense will make that point moot.

I know I'm going back and forth, but I'm just expressing my thought process.

bluetarpmedia commented 3 years ago

I'd vote for suffixes, even if only for the group part.

File.Print, Edit.GoTo and View.Code all look very command-like to me because they directly correspond to commands with those exact names inside their menus, whereas the group-only suffix helps:

File.PrintGroup Edit.GoToGroup View.CodeGroup

madskristensen commented 3 years ago

Just so we're clear, it means that

Edit.LanguageInfo.Advanced.Commands

becomes

Edit.LanguageInfoGroup.Advanced.CommandsGroup

Does that look not too long and easier to understand to everybody?

bluetarpmedia commented 3 years ago

Personally I think the little bit of extra verbosity is worth the trade off for the clarity.

yannduran commented 3 years ago

@madskristensen

Normally I'd prefer the simpler option, but in this case I agree with @bluetarpmedia that it's ambiguous whether the names actually refer to groups/menus etc. Good code should always make it clear what names mean (within reason of course).

With intellisense, it's not like you're going to be typing in the whole name anyway. It'd be especially helpful for developers who are new to extensions. Even with Mad's idea to have an accompanying image, you should still be able to see at a glance what something.something.something actually means.

Unless maybe a tooltip would be an acceptable alternative?