CommandBars, Buttons and Commands: Cleanup is on the menu

Question

One of the biggest pain point in the entire rubberduck code base, is the way commandbar menus and menu items are created and wired up - something needs to be done to straighten that up, and while I was refactoring the App class and got to the point where I needed to address the dependencies of RubberduckMenu, I had an idea: instead of creating menu buttons and wiring up event handlers, I could be creating commands and associating them with as many buttons as needed!

For example, I could have a RefactorRenameCommand that could be called from the Rubberduck/Refactorings menu, or from the code pane context menu, or from the form designer's context menu, or whatever.

So I created a Rubberduck.UI.Commands namespace, and wrote an abstract class to encapsulate the concept:

namespace Rubberduck.UI.Commands
{
    /// <summary>
    /// Base class to derive all menu commands from.
    /// </summary>
    public abstract class RubberduckCommandBase
    {
        private readonly IRubberduckMenuCommand _command;

        protected RubberduckCommandBase(IRubberduckMenuCommand command)
        {
            _command = command;
            _command.RequestExecute += command_RequestExecute;
        }

        private void command_RequestExecute(object sender, EventArgs e)
        {
            ExecuteAction();
        }

        protected IRubberduckMenuCommand Command { get { return _command; } }

        /// <summary>
        /// A method that uses the <see cref="Command"/> helper to wire up as many UI controls as needed.
        /// </summary>
        public abstract void Initialize();

        /// <summary>
        /// The method that is executed when either wired-up UI control is clicked.
        /// </summary>
        public abstract void ExecuteAction();

        public void Release()
        {
            _command.Release();
        }
    }
}

The IRubberduckMenuCommand interface defines everything I need to be able to associate "buttons" with a command, to destroy the created COM objects on demand (changing the app's language from the Settings dialog causes a reload of the entire UI), and of course a mechanism to communicate that the command needs to be executed:

namespace Rubberduck.UI.Commands
{
    /// <summary>
    /// An object that encapsulates the logic to wire up a number of CommandBarControl instances to a specific command.
    /// </summary>
    public interface IRubberduckMenuCommand
    {
        /// <summary>
        /// Associates a new <see cref="CommandBarButton"/> to the command.
        /// </summary>
        /// <param name="parent">The parent control collection to add the button to.</param>
        /// <param name="caption">The localized caption for the command.</param>
        /// <param name="beginGroup">Optionally specifies that the UI element begins a command group.</param>
        /// <param name="beforeIndex">Optionally specifies the index of the UI element in the parent collection.</param>
        /// <param name="image">An optional icon to represent the command.</param>
        /// <param name="mask">A transparency mask for the command's icon. Required if <see cref="image"/> is not null.</param>
        void AddCommandBarButton(CommandBarControls parent, string caption, bool beginGroup = false, int beforeIndex = -1, Image image = null, Image mask = null);

        /// <summary>
        /// Destroys all UI elements associated to the command.
        /// </summary>
        void Release();

        /// <summary>
        /// Requests execution of the command.
        /// </summary>
        event EventHandler RequestExecute;
    }
}

At first I had a plain Execute method there and it looked nice on paper, but the simple AboutCommand implementation was getting pretty complicated, so I changed it a bit - this is how the interface ended up implemented:

namespace Rubberduck.UI.Commands
{
    public class RubberduckMenuCommand : IRubberduckMenuCommand
    {
        private readonly IList<CommandBarButton> _buttons;

        /// <summary>
        /// Creates a new menu command.
        /// </summary>
        public RubberduckMenuCommand()
        {
            _buttons = new List<CommandBarButton>();
        }

        public void AddCommandBarButton(CommandBarControls parent, string caption, bool beginGroup = false, int beforeIndex = -1, Image image = null, Image mask = null)
        {
            if (image != null && mask == null)
            {
                throw new ArgumentNullException("'image' cannot be null if 'mask' is non-null.");
            }
            if (image == null && mask != null)
            {
                throw new ArgumentNullException("'mask' cannot be null if 'image' is non-null.");
            }

            var button = (CommandBarButton) (beforeIndex == -1
                ? parent.Add(MsoControlType.msoControlButton, Temporary: true)
                : parent.Add(MsoControlType.msoControlButton, Before: beforeIndex, Temporary: true));

            button.BeginGroup = beginGroup;
            button.Caption = caption;

            if (image != null)
            {
                SetButtonImage(button, image, mask);
            }

            button.Click += button_Click;
            _buttons.Add(button);
        }

        public void Release()
        {
            foreach (var button in _buttons)
            {
                button.Click -= button_Click;
                button.Delete();
                Marshal.ReleaseComObject(button);
            }

            _buttons.Clear();
        }

        private void button_Click(CommandBarButton Ctrl, ref bool CancelDefault)
        {
            OnRequestExecute();
        }

        public event EventHandler RequestExecute;
        public void OnRequestExecute()
        {
            var handler = RequestExecute;
            if (handler != null)
            {
                handler(this, EventArgs.Empty);
            }
        }

        private void SetButtonImage(CommandBarButton button, Image image, Image mask)
        {
            button.FaceId = 0;
            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);
            }
        }
    }
}

I only need this RubberduckMenuCommand implementation. In fact, I'm not even sure I need the abstraction in the first place: would it hurt to couple RubberduckCommandBase with RubberduckMenuCommand?

Here's the "prototype" command:

namespace Rubberduck.UI.Commands
{
    /// <summary>
    /// A command that displays the "About" dialog.
    /// </summary>
    public class AboutCommand : RubberduckCommandBase
    {
        private readonly VBE _vbe;

        public AboutCommand(IRubberduckMenuCommand command, VBE vbe)
            : base(command)
        {
            _vbe = vbe;
        }

        public override void Initialize()
        {
            var parent = _vbe.CommandBars[1].Controls.OfType<CommandBarPopup>()
                .SingleOrDefault(control => control.Caption == RubberduckUI.RubberduckMenu);

            if (parent == null)
            {
                throw new InvalidOperationException("Parent menu not found. Cannot create child menu item.");
            }

            Command.AddCommandBarButton(parent.Controls, RubberduckUI.RubberduckMenu_About, true);
        }

        public override void ExecuteAction()
        {
            using (var window = new _AboutWindow())
            {
                window.ShowDialog();
            }
        }
    }
}

As you can see, all the class is doing is calling Command.AddCommandBarButton in the Initialize method, and doing its thing in the ExecuteAction method.

If I implement all menu commands like this, I'll have a class for each one and the RubberduckMenu class will no longer need to care about handling & dispatching any of the clicks; in fact, all it will be responsible for, will be to create the CommandBarPopup objects that each command will use as parents. And I can constructor-inject some IEnumerable<RubberduckCommandBase> and simply iterate all commands and call their Initialize method to create the menu.

This looks too good to be true. Do you see anything that's coming to bite me?

Answer 1

I think you have the right idea, but your implementation can be improved. I'll give it a try, although I have no clue what rubberduck is, so I might be missing something here. :)

1. The way you described it, the RubberduckCommand should represent the action to be performed. And it should not matter, whether you invoke this action via menu button, toolbar button, hotkey combination or whatever. If that is the case, then it should not depend on IRubberduckMenuCommand interface. You should reverse the dependency.
2. The button adding itself to menu looks weird. I think you need an abstrction for menus and toolbars with AddMenuItem method, which would take your button as argument. Again, reverse the dependency. AddCommandBarButton method looks pretty generic to me, should be easy enough.
3. I don't like AddCommandBarButton parameters. It feels like half of those should be properties in IRubberduckMenuCommand interface.
4. Class name starting with underscore is something those eyes have never seen before.:)

That being said, here is how I would refactor it:

//This is abstrcation for command.
//It deos not "know" anything about menu structure or menu API.
//Pretty similar to Input.ICommand.
interface IRubberCommand
{
    void Execute();
    bool CanExecute { get; }
}

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

    public bool CanExecute
    {
        get { return true; }
    }
}

//This is abstrcation for menu item. It holds the reference to associated command
//and all the necessary information to construct this item in UI.
//But it does not construct the item itself, this is a job for higher level of abstraction
interface IRubberButtonMenuItem
{
    IRubberCommand Command { get; }

    Image Image { get; }
    Image Mask { get; }
    string Caption { get; }
}

class ShowAboutMenuItem : IRubberButtonMenuItem
{
    //inject command from DI container
    public ShowAboutMenuItem(ShowAboutCommand command)
    {
        Command = command;
    }

    public IRubberCommand Command { get; private set; }

    public Image Image
    {
        get { return null; }
    }

    public Image Mask
    {
        get { return null; }
    }

    public string Caption
    {
        get { return "About"; }
    }
}

//This is abstrcation for menu itself.
//This is where you create actual UI objects
//for IRubberButtonMenuItem's, this is where you release them
//and this is where you hold the list of added MenuItems.
interface IRubberMenu
{
    //create UI object and add it to menu
    void AddItem(IRubberButtonMenuItem menuItem, ...);
    //remove and dispose UI object
    void RemoveItem(IRubberButtonMenuItem menuItem);
    //remove all items
    void Clear();
    //get all added items
    IEnumerable<IRubberButtonMenuItem> GetAll();
}

class Toolbar : IRubberMenu
{
    ...
}

class ContextMenu : IRubberMenu
{
    ...
}

Answer 2

This looks good! I've got a couple of comments to make in a vague, hand wavy way so I'll start off with a comment about the code as written:

public SomeMethod(int beforeIndex = -1)
{
    if (beforeIndex == -1) 
    {

No! Bad Mug. No presents for you this Christmas. Use a Nullable<int> and be more idiomatic:

public SomeMethod(int? beforeIndex = null)
{
    if (beforeIndex.HasValue) 
    {

There's also some magic numbery going on here:

_vbe.CommandBars[1].Controls.OfType<CommandBarPopup>()
            .SingleOrDefault(control => control.Caption == RubberduckUI.RubberduckMenu);

You could do something like:

_vbe.CommandBars.SelectMany(commandBar => commandBar.Controls)
                .OfType<CommandBarPopup>()
                .SingleOrDefault(control => control.Caption == RubberduckUI.RubberduckMenu);

Or name your constant. E.g. const int RubberDuckMenuBarIndex = 1.

Anyway, the code looks good - nice names, nice spacing, great formatting in general. I'm assuming IPictureDisp isn't one of your own interfaces?

So, on to some dangerous hand waving.

I feel like you need to make the seperation between displying the buttons and wiring up the command a bit more obvious. I think the way you can achieve that is with the mediator pattern.

You basically have a central hub that sends messages to the right things to do the right thing. All of your buttons could then do the same thing - send a message to the mediator.

var result = Mediator.Send(new AboutClickedCommand());

Then you have some sort of iterfaces like:

public interface ICommand<TResult> { }

public interface ICommandHandler<T, TResult> where T : ICommand<TResult>
{
    TResult Handle(T command);
}

public class Mediator
{
    public TResult Send<TResult> (ICommand<TResult> command)
    {
        var handler = LookUpHandlerForCommand(command);
        if (handler != null)
        {
            return handler.Handle(command);
        }
        throw new HandlerMissingException();
    }
}

So your mediator simply looks up a handler and passes the message on and returns the result back. I haven't used it, but Jimmy Bogard has a library called Mediatr which is supposedly very good.

Your buttons would simply encapsulate the rules about displaying themselves, your ICommandHandlers would have all of the stuff about what they should do. You also allow the handler to report back to the thing that called it - quite a useful thing to have. You can also add properties to your command objects to pass in additional information/context.

Hopefully that makes sense and I haven't messed up the generics in my sample code...