6
\$\begingroup\$

Following-up on CommandBars, Buttons and Commands: Cleanup is on the menu, I decided to try a more ambitious approach, as suggested in Nikita's answer. It works, but there are a number of "gotchas" that I'm not sure how to clean up.

So I have an ICommand interface, and its generic counterpart:

public interface ICommand
{
    void Execute();
}

public interface ICommand<in T> : ICommand
{
    void Execute(T parameter);
}

Note that there's no CanExecute method, because I haven't yet figured out where the VBE API or the Office CommandBar API is giving me a decent hook to execute it.

I haven't implemented any parameterized commands yet, but I'm thinking the parameterless overload will call the parameterized one after figuring out the parameter to use... but I'll stick to the "About" menu command for this post.

public class AboutCommand : ICommand
{
    public void Execute()
    {
        using (var window = new _AboutWindow())
        {
            window.ShowDialog();
        }
    }
}

public class AboutCommandMenuItem : CommandMenuItemBase
{
    public AboutCommandMenuItem(ICommand command)
        : base(command)
    {
    }

    public override string Key { get { return "RubberduckMenu_About"; } }
}

The CommandMenuItemBase is an abstract class that immensely facilitates implementing menu commands - all that's left to do is to specify the resource key that contains the caption.

public abstract class CommandMenuItemBase : ICommandMenuItem
{
    private readonly ICommand _command;

    protected CommandMenuItemBase(ICommand command)
    {
        _command = command;
    }

    public abstract string Key { get; }

    public ICommand Command { get { return _command; } }
    public Func<string> Caption { get { return () => RubberduckUI.ResourceManager.GetString(Key, RubberduckUI.Culture); } }
    public bool IsParent { get { return false; } }
    public virtual Image Image { get { return null; } }
    public virtual Image Mask { get { return null; } }

}

The class implements ICommandMenuItem, which itself extends the IMenuItem interface:

public interface ICommandMenuItem : IMenuItem
{
    ICommand Command { get; }
}

public interface IMenuItem
{
    Func<string> Caption { get; }

    string Key { get; }

    bool IsParent { get; }
    Image Image { get; }
    Image Mask { get; }
}

With that done, I need a way to regroup and manage the menu items - enter ICommandBar:

public interface ICommandBar
{
    void Localize();
    void AddItem(IMenuItem item, bool? beginGroup = null, int? beforeIndex = null);
    bool RemoveItem(IMenuItem item);
    bool Remove();
    IEnumerable<IMenuItem> Items { get; }
}

And now I could wrap a CommandBarPopup object, by implementing both IMenuItem and ICommandBar:

/// <summary>
/// An objects that wraps a <see cref="CommandBarPopup"/> instance.
/// </summary>
public abstract class ParentMenu : IMenuItem, ICommandBar
{
    private readonly string _key;
    private readonly Func<string> _caption;
    private readonly CommandBarPopup _popup;
    private readonly IDictionary<IMenuItem, CommandBarControl> _items = new Dictionary<IMenuItem, CommandBarControl>();

    protected ParentMenu(CommandBarControls parent, string key, Func<string> caption, int? beforeIndex)
    {
        _key = key;
        _caption = caption;
        _popup = beforeIndex.HasValue
            ? (CommandBarPopup) parent.Add(MsoControlType.msoControlPopup, Temporary: true, Before: beforeIndex)
            : (CommandBarPopup) parent.Add(MsoControlType.msoControlPopup, Temporary: true);
        _popup.Tag = _key;

        Localize();
    }

    public abstract void Initialize();

    public string Key { get { return _key; } }
    public Func<string> Caption { get {return _caption; } }
    public bool IsParent { get { return true; } }
    public Image Image { get {return null; } }
    public Image Mask { get { return null; } }

    public void Localize()
    {
        _popup.Caption = _caption.Invoke();
        LocalizeChildren();
    }

    private void LocalizeChildren()
    {
        foreach (var kvp in _items)
        {
            var value = kvp.Key.Caption.Invoke();
            kvp.Value.Caption = value;
        }
    }

    public void AddItem(IMenuItem item, bool? beginGroup = null, int? beforeIndex = null)
    {
        var controlType = item.IsParent 
            ? MsoControlType.msoControlPopup 
            : MsoControlType.msoControlButton;
        var child = beforeIndex.HasValue
            ? _popup.Controls.Add(controlType, Temporary: true, Before: beforeIndex)
            : _popup.Controls.Add(controlType, Temporary: true);

        child.Caption = item.Caption.Invoke();
        child.BeginGroup = beginGroup ?? false;
        child.Tag = item.Key;

        if (!item.IsParent)
        {
            var button = (CommandBarButton)child;
            SetButtonImage(button, item.Image, item.Mask);

            var command = ((ICommandMenuItem)item).Command;
            button.Click += delegate { command.Execute(); };
        }

        _items.Add(item, child);
    }

    public bool RemoveItem(IMenuItem item)
    {
        try
        {
            var child = _items[item];
            child.Delete();
            Marshal.ReleaseComObject(child);
            _items.Remove(item);
            return true;
        }
        catch (COMException)
        {
            return false;
        }
    }

    public bool Remove()
    {
        foreach (var menuItem in _items)
        {
            RemoveItem(menuItem.Key); // note: should we care if this fails?
        }

        try
        {
            _popup.Delete();
            Marshal.ReleaseComObject(_popup);
            return true;
        }
        catch (COMException)
        {
            return false;
        }
    }

    public IEnumerable<IMenuItem> Items { get { return _items.Keys; } }

    private static void SetButtonImage(CommandBarButton button, Image image, Image mask)
    {
        button.FaceId = 0;
        if (image == null || mask == null)
        {
            return;
        }

        button.Picture = AxHostConverter.ImageToPictureDisp(image);
        button.Mask = AxHostConverter.ImageToPictureDisp(mask);
    }

    private class AxHostConverter : AxHost
    {
        private AxHostConverter() : base("") { }

        static public IPictureDisp ImageToPictureDisp(Image image)
        {
            return (IPictureDisp)GetIPictureDispFromPicture(image);
        }

        static public Image PictureDispToImage(IPictureDisp pictureDisp)
        {
            return GetPictureFromIPicture(pictureDisp);
        }
    }
}

And this class is implemented by the RubberduckParentMenu class, and will be implemented by a RefactorParentMenu class at one point, too.

public class RubberduckParentMenu : ParentMenu
{
    private readonly CodeExplorerCommandMenuItem _codeExplorer;
    private readonly OptionsCommandMenuItem _options;
    private readonly AboutCommandMenuItem _about;

    public RubberduckParentMenu(CommandBarControls parent, int beforeIndex,
        CodeExplorerCommandMenuItem codeExplorer,
        OptionsCommandMenuItem options,
        AboutCommandMenuItem about)
        : base(parent, "RubberduckMenu", () => RubberduckUI.RubberduckMenu, beforeIndex)
    {
        _codeExplorer = codeExplorer;
        _options = options;
        _about = about;
    }

    public override void Initialize()
    {
        AddItem(_codeExplorer);
        AddItem(_options, true);
        AddItem(_about, true);
    }
}

And there's my problem: I'm going to end up with a constructor parameter for each menu item I want in the [Rubberduck] menu, and it doesn't feel right that I'm injecting concrete types when IMenuItem would suffice... on the other hand, how else could I know in which order to add them, which needs to BeginGroup, and what image/icon to use for which, if I were iterating IMenuItem instances?

Another problem is the IoC configuration, which is utterly annoying because I couldn't figure out how to set up a convention to do the ICommand bindings, so I'll need one for each type... and that's not very maintenance-friendly:

    private void BindRubberduckMenu()
    {
        const int windowMenuId = 30009;
        var menuBarControls = _vbe.CommandBars[1].Controls;
        var beforeIndex = FindMenuInsertionIndex(menuBarControls, windowMenuId);

        _kernel.Bind(t => t.FromThisAssembly()
            .SelectAllClasses()
            .InNamespaceOf<ICommand>()
            .EndingWith("CommandMenuItem")
            .BindToSelf());

        _kernel.Bind<ICommand>().To<AboutCommand>().WhenInjectedExactlyInto<AboutCommandMenuItem>();
        _kernel.Bind<ICommand>().To<OptionsCommand>().WhenInjectedExactlyInto<OptionsCommandMenuItem>();
        _kernel.Bind<ICommand>().To<CodeExplorerCommand>().WhenInjectedExactlyInto<CodeExplorerCommandMenuItem>();

        _kernel.Bind<RubberduckParentMenu>().ToSelf()
            .WithConstructorArgument("parent", menuBarControls)
            .WithConstructorArgument("beforeIndex", beforeIndex);
    }

There has to be a better way. Anything else jumps at you?

\$\endgroup\$
  • \$\begingroup\$ i don't know the "framework" or library you're working with so maybe my comment won't make sense. Lots of what makes up a menu item seems to be structure and definition (tree, name, icon,.. you name it). Would it be an option to to extract definition into types which only contain the definition and then use a "generic" (not c# generics) command item which either gets the definition inject or is built up by the definition? That would mean you would not need inheritance (prefer aggregation over inheritance), there might be a solution for the command stuff in it, too. \$\endgroup\$ – BatteryBackupUnit Aug 5 '15 at 4:53
  • \$\begingroup\$ Also, since you're doing an open-source project, would you mind linking to the 1,2,3 parts in the code base which are relevant here? I'd like to take a look as soon as I get some time. \$\endgroup\$ – BatteryBackupUnit Aug 5 '15 at 4:55
  • \$\begingroup\$ @BatteryBackupUnit I'm struggling with this refactoring, I've shelved these changes in the ICommand_Shelved branch on my fork; the relevant namespace is Rubberduck.UI.Commands, and then there's the RubberduckModule class that holds the kernel bindings. \$\endgroup\$ – Mathieu Guindon Aug 5 '15 at 5:58
  • \$\begingroup\$ @BatteryBackupUnit FYI I ended up getting everything to work as intended, the "shelved" branch is now merged into the [next] branch of the main repository, and there's a bunch of GitHub issues left up-for-grabs =) \$\endgroup\$ – Mathieu Guindon Sep 6 '15 at 14:01
4
\$\begingroup\$

I think the code is definitely much better! I wouldn't worry too much about registering the ICommands in your IoC. After all, you're not going to have hundreds of menu items yet.

Anyway, two very minor comments:

= beginGroup ?? false;

is better as

= beginGroup.GetValueOrDefault();

And this:

private AxHostConverter() : base("") { }

Would be better as:

private AxHostConverter() : base(string.Empty) { }

Edit

With the caveat that I don't know ninject and that this isn't very performant here's a reflection based approach for registration as a complete console app:

internal class Program
{
    static void Main(string[] args)
    {
        IKernel kernel = new StandardKernel();

        var query = from command in Assembly.GetExecutingAssembly().GetTypes()
                    where command.GetInterfaces().Contains(typeof(ICommand)) && command.IsClass
                    select
                        new Tuple<Type, Type>(
                        command,
                        FindCorrespondingType(command));

        foreach (var item in query)
        {
            kernel.Bind<ICommand>().To(item.Item1).WhenInjectedExactlyInto(item.Item2);
        }

        kernel.Get<AboutCommandMenuItem>();
        kernel.Get<OptionsCommandMenuItem>();
        Console.ReadKey();
    }

    private static Type FindCorrespondingType(Type command)
    {
        return Assembly.GetExecutingAssembly().GetTypes().FirstOrDefault(t => t.Name == $"{command.Name}MenuItem");
    }
}

public interface ICommand
{ }

public class AboutCommand : ICommand
{
}

public class OptionsCommand : ICommand
{

}

public class AboutCommandMenuItem
{
    public AboutCommandMenuItem(ICommand comman)
    {
        Console.WriteLine(comman.GetType());
    } 
}

public class OptionsCommandMenuItem
{
    public OptionsCommandMenuItem(ICommand comman)
    {
        Console.WriteLine(comman.GetType());
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks again! Note, the reason for registering ICommand, is so their dependencies can be automagically resolved. AboutCommand doesn't do much, but consider some RefactorExtractMethodCommand, that has an IRefactoring dependency, which has its own assortment of dependencies... \$\endgroup\$ – Mathieu Guindon Aug 4 '15 at 13:56
  • \$\begingroup\$ @Mat'sMug - I've got you. That makes perfect sense, if it were StructureMap I'd probably be able to help a bit but apart from the code above I've never used ninject :( \$\endgroup\$ – RobH Aug 4 '15 at 14:29
2
\$\begingroup\$

I feel like implementing support for both ICommand and ICommand<T> is going to be really tiresome. Not to mention, that ICommand<T> implementation is going to have two methods: Execute and Execute(T). Which one should I call? Really confusing. If you already know for sure, that you are going to need a command which takes a parameter, you should probably go for weak-typed API instead of generics:

interface ICommand
{
    void Execute(object arg = null);
}

You are hooking ICommand.Execute method to Click event though. I don't quite see at which point you are going to pass the parameter to command and what it is going to be.

As for the way you create menus... conceptually it looks about right. At some point, you are going to have this place, where you explicitly add items to menu in desired order, I doubt there is a pretty way around it. There are different ways to approach this though. You could, for example, create a generic add method:

void AddItem<TMenuItem>(bool? beginGroup = null, int? beforeIndex = null) 
   where TMenuItem : IMenuItem

and manually fetch menu item inside this method from your DI container. This way you won't have to create one million fields in you RubberduckParentMenu as your menu grows.

You should also probably split AddItem method:

void AddItem(IMenuItem item, bool? beginGroup = null, int? beforeIndex = null);
void AddCommandBar(ICommandBar bar, ...);

and remove IsParent property. Then you simply use first method to add commands, and second method to add sub-menus.

\$\endgroup\$
  • \$\begingroup\$ I'm going to bind the commands InSingletonScope and inject them in places where they will be programmatically executed, e.g. a code inspection quick-fix could pass a Declaration parameter to some RefactorRenameCommand, or a QualifiedSelection parameter to some RefactorExtractMethodCommand... or perhaps I'm over-thinking this and these things should depend directly on IRefactoring... \$\endgroup\$ – Mathieu Guindon Aug 4 '15 at 16:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.