CommandBars, Buttons and Commands: Take 2

Question

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?

Answer 1

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());
    }
}

Answer 2

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.