Step by Step, ooh HotKey

Question

Still hooked on Windows, I refactored the hook into a number of interfaces, and I wanted to support 2-step hotkeys, so I started with defining an IHook that can be attached, detached, exposes a property that tells whether a hook is attached or not, and raises an event when the hook receives a message:

public interface IHook
{
    event EventHandler<HookEventArgs> MessageReceived;
    void OnMessageReceived();

    bool IsAttached { get; }

    void Attach();
    void Detach();
}

The HookEventArgs simply carry a Keys enum value:

public class HookEventArgs : EventArgs
{
    private readonly Keys _key;

    public HookEventArgs(Keys key)
    {
        _key = key;
    }

    public Keys Key { get { return _key; } }

    public new static HookEventArgs Empty {get { return new HookEventArgs(Keys.None); }}
}

Then I extended IHook into a IHotKeyHook, which exposes a HookInfo value and a flag that indicates whether that hotkey is a 2-step hotkey, like Visual Studio's Ctrl+R,M for refactor/extract method:

public interface IHotKeyHook : IHook
{
    HookInfo HookInfo { get; }
    bool IsTwoStepHotKey { get; }
}

I also extracted a TimerHook, and gave it its own interface:

public interface ITimerHook
{
    event EventHandler Tick;
    void Attach();
    void Detach();
}

Lastly, I added a level of abstraction for an object that would be responsible for managing all these hooks and passing the events to the Rubberduck App class:

public interface IRubberduckHooks : IHook, IDisposable
{
    IEnumerable<IHook> Hooks { get; }
    void AddHook<THook>(THook hook) where THook : IHook;
}

---

App

Purely for context, here's how the App class consumes IRubberduckHooks - I have yet to actually handle the hotkeys and map them to actual commands, but the backbone is here:

    private async void hooks_MessageReceived(object sender, HookEventArgs e)
    {
        if (sender is LowLevelKeyboardHook)
        {
            if (_skipKeyUp)
            {
                _skipKeyUp = false;
                return;
            }

            if (_isAwaitingTwoStepKey)
            {
                // todo: use _firstStepHotKey and e.Key to run 2-step hotkey action

                AwaitNextKey(false);
                return;
            }

            var component = _vbe.ActiveCodePane.CodeModule.Parent;
            await ParseComponentAsync(component);

            AwaitNextKey(false);
            return;
        }

        var hotKey = sender as IHotKeyHook;
        if (hotKey == null)
        {
            AwaitNextKey(false);
            return;
        }

        if (hotKey.IsTwoStepHotKey)
        {
            _firstStepHotKey = hotKey.HookInfo.Key;
            AwaitNextKey(true, _firstStepHotKey);
        }
        else
        {
            // todo: use e.Key to run 1-step hotkey action

            _firstStepHotKey = Keys.None;
            AwaitNextKey(false);
        }
    }

    private void AwaitNextKey(bool eatNextKey = true, Keys key = default(Keys))
    {
        _isAwaitingTwoStepKey = eatNextKey;
        foreach (var hook in _hooks.Hooks.OfType<ILowLevelKeyboardHook>())
        {
            hook.EatNextKey = eatNextKey;
        }

        // todo: include modifier keys
        _stateBar.SetStatusText(eatNextKey ? "(hotkey " + key + ") was pressed. Waiting for second key..." : "Ready.");
        _skipKeyUp = eatNextKey;
    }

---

RubberduckHooks

This class is responsible for "coordinating" the hooks, and implements the main Windows message hook that picks up, notably HOTKEY messages. It's this class that the App class calls into, to register hotkeys; it's also this class' events that the App class is listening for.

I'm having a problem with the parameterless public void OnMessageReceived() method that the IRubberduckHooks interface inherits from IHook, since the App class isn't supposed to be able to call that method - I "solved" it by throwing a NotImplementedException, but IMO there's a design smell here.. have a look:

public class RubberduckHooks : IRubberduckHooks
{
    private readonly IntPtr _mainWindowHandle;

    private readonly IntPtr _oldWndPointer;
    private readonly User32.WndProc _oldWndProc;
    private User32.WndProc _newWndProc;

    private readonly ITimerHook _timerHook;

    private const int WA_INACTIVE = 0;
    private const int WA_ACTIVE = 1;

    public RubberduckHooks(IntPtr mainWindowHandle, ITimerHook timerHook)
    {
        _mainWindowHandle = mainWindowHandle;
        _oldWndProc = WindowProc;
        _newWndProc = WindowProc;
        _oldWndPointer = User32.SetWindowLong(_mainWindowHandle, (int)WindowLongFlags.GWL_WNDPROC, _newWndProc);
        _oldWndProc = (User32.WndProc)Marshal.GetDelegateForFunctionPointer(_oldWndPointer, typeof(User32.WndProc));

        _timerHook = timerHook;
        _timerHook.Tick += timerHook_Tick;
    }

    private readonly IList<IHook> _hooks = new List<IHook>(); 
    public IEnumerable<IHook> Hooks { get { return _hooks; } }

    public void AddHook<THook>(THook hook) where THook : IHook
    {
        _hooks.Add(hook);
    }

    public event EventHandler<HookEventArgs> MessageReceived;

    public void OnMessageReceived()
    {
        throw new NotImplementedException();
    }

    private void OnMessageReceived(object sender, HookEventArgs args)
    {
        var handler = MessageReceived;
        if (handler != null)
        {
            handler.Invoke(sender, args);
        }
    }
    
    public bool IsAttached { get; private set; }

    public void Attach()
    {
        if (IsAttached)
        {
            return;
        }

        foreach (var hook in Hooks)
        {
            hook.MessageReceived += hook_MessageReceived;
            hook.Attach();
        }

        IsAttached = true;
    }

    public void Detach()
    {
        if (!IsAttached)
        {
            return;
        }

        foreach (var hook in Hooks)
        {
            hook.MessageReceived -= hook_MessageReceived;
            hook.Detach();
        }

        IsAttached = false;
    }

    private void hook_MessageReceived(object sender, HookEventArgs e)
    {
        OnMessageReceived(sender, e);
    }

    private void timerHook_Tick(object sender, EventArgs e)
    {
        if (!IsAttached && User32.GetForegroundWindow() == _mainWindowHandle)
        {
            Attach();
        }
        else
        {
            Detach();
        }
    }

    public void Dispose()
    {
        _timerHook.Tick -= timerHook_Tick;
        _timerHook.Detach();

        Detach();
    }

    private IntPtr WindowProc(IntPtr hWnd, int uMsg, int wParam, int lParam)
    {
        try
        {
            var processed = false;
            if (hWnd == _mainWindowHandle)
            {
                switch ((WM)uMsg)
                {
                    case WM.HOTKEY:
                        if (GetWindowThread(User32.GetForegroundWindow()) == GetWindowThread(_mainWindowHandle))
                        {
                            var hook = Hooks.OfType<IHotKeyHook>().SingleOrDefault(k => k.HookInfo.HookId == (IntPtr)wParam);
                            if (hook != null)
                            {
                                hook.OnMessageReceived();
                                processed = true;
                            }
                        }
                        break;

                    case WM.ACTIVATEAPP:
                        switch (LoWord(wParam))
                        {
                            case WA_ACTIVE:
                                Attach();
                                break;

                            case WA_INACTIVE:
                                Detach();
                                break;
                        }

                        break;
                }
            }

            if (!processed)
            {
                return User32.CallWindowProc(_oldWndProc, hWnd, uMsg, wParam, lParam);
            }
        }
        catch (Exception exception)
        {
            Console.WriteLine(exception);
        }

        return User32.CallWindowProc(_oldWndProc, hWnd, uMsg, wParam, lParam);
    }

    /// <summary>
    /// Gets the integer portion of a word
    /// </summary>
    private static int LoWord(int dw)
    {
        return (dw & 0x8000) != 0
            ? 0x8000 | (dw & 0x7FFF)
            : dw & 0xFFFF;
    }

    private IntPtr GetWindowThread(IntPtr hWnd)
    {
        uint hThread;
        User32.GetWindowThreadProcessId(hWnd, out hThread);

        return (IntPtr)hThread;
    }
}

---

HotKeyHook

The HotKeyHook class is responsible for attaching and detaching a single hotkey, so that the RubberduckHooks instance can pick it up in the main Windows message hook as a WM_HOTKEY message.

I don't like the back-and-forth going on here:

• User presses a hotkey in the VBE.
• WM_HOTKEY message gets picked up in RubberduckHooks instance; the OnMessageReceived method is called on the appropriate HotKeyHook instance.
• RubberduckHooks handles the HotKeyHook instance's MessageReceived event by relaying it to its own OnMessageReceived overload.
• App class receives a MessageReceived event that conveys a HookEventArgs that tells Rubberduck which hotkey was pressed, and that's where the hotkey gets handled.

I'm sure this can be simplified quite a bit.

public class HotKeyHook : IHotKeyHook
{
    private readonly Action _action;
    private readonly IntPtr _hWndVbe;

    public HookInfo HookInfo { get; private set; }

    public HotKeyHook(IntPtr hWndVbe, string key, bool isTwoStepHotKey)
    {
        _hWndVbe = hWndVbe;
        _action = OnMessageReceived;

        IsTwoStepHotKey = isTwoStepHotKey;
        Key = key;
    }

    public bool IsTwoStepHotKey { get; private set; }
    public bool IsAttached { get; private set; }
    public string Key { get; private set; }

    public event EventHandler<HookEventArgs> MessageReceived;

    public void OnMessageReceived()
    {
        var handler = MessageReceived;
        if (handler != null)
        {
            var hotKey = Key;
            var shift = GetModifierValue(ref hotKey);
            var args = new HookEventArgs(GetKey(hotKey));
            handler.Invoke(this, args);
        }
    }

    public void Attach()
    {
        var hotKey = Key;
        var lShift = GetModifierValue(ref hotKey);
        var lKey = GetKey(hotKey);

        if (lKey == Keys.None)
        {
            throw new InvalidOperationException("Invalid key.");
        }

        HookKey(lKey, lShift, _action);
    }

    public void Detach()
    {
        if (!IsAttached)
        {
            throw new InvalidOperationException("Hook is already detached.");
        }

        User32.UnregisterHotKey(_hWndVbe, HookInfo.HookId);
        Kernel32.GlobalDeleteAtom(HookInfo.HookId);

        IsAttached = false;
    }

    private void HookKey(Keys key, uint shift, Action action)
    {
        if (IsAttached)
        {
            throw new InvalidOperationException("Hook is already attached.");
        }

        var hookId = (IntPtr)Kernel32.GlobalAddAtom(Guid.NewGuid().ToString());
        var success = User32.RegisterHotKey(_hWndVbe, hookId, shift, (uint)key);
        if (!success)
        {
            throw new Win32Exception("HotKey was not registered.");
        }

        HookInfo = new HookInfo(hookId, key, shift, action);
        IsAttached = true;
    }

    private static readonly IDictionary<char,uint> Modifiers = new Dictionary<char, uint>
    {
        { '+', (uint)KeyModifier.SHIFT },
        { '%', (uint)KeyModifier.ALT },
        { '^', (uint)KeyModifier.CONTROL },
    };

    /// <summary>
    /// Gets the <see cref="KeyModifier"/> values out of a key combination.
    /// </summary>
    /// <param name="key">The hotkey string, returned without the modifiers.</param>
    private static uint GetModifierValue(ref string key)
    {
        uint result = 0;

        for (var i = 0; i < 3; i++)
        {
            var firstChar = key[0];
            if (Modifiers.ContainsKey(firstChar))
            {
                result |= Modifiers[firstChar];
            }
            else
            {
                // first character isn't a modifier symbol:
                break;
            }

            // truncate first character for next iteration:
            key = key.Substring(1);
        }

        return result;
    }

    private Keys GetKey(string keyCode)
    {
        var result = Keys.None;
        switch (keyCode.Substring(0, 1))
        {
            case "{":
                _keys.TryGetValue(keyCode, out result);
                break;
            case "~":
                result = Keys.Return;
                break;
            default:
                if (!String.IsNullOrEmpty(keyCode))
                {
                    result = (Keys)Enum.Parse(typeof(Keys), keyCode);
                }
                break;
        }

        return result;
    }

    private static readonly IDictionary<string, Keys> _keys = new Dictionary<string, Keys>
    {
        { "{BACKSPACE}", Keys.Back },
        { "{BS}", Keys.Back },
        { "{BKSP}", Keys.Back },
        { "{CAPSLOCK}", Keys.CapsLock },
        { "{DELETE}", Keys.Delete },
        { "{DEL}", Keys.Delete },
        { "{DOWN}", Keys.Down },
        { "{END}", Keys.End },
        { "{ENTER}", Keys.Enter },
        { "{RETURN}", Keys.Enter },
        { "{ESC}", Keys.Escape },
        { "{HELP}", Keys.Help },
        { "{HOME}", Keys.Home },
        { "{INSERT}", Keys.Insert },
        { "{INS}", Keys.Insert },
        { "{LEFT}", Keys.Left },
        { "{NUMLOCK}", Keys.NumLock },
        { "{PGDN}", Keys.PageDown },
        { "{PGUP}", Keys.PageUp },
        { "{PRTSC}", Keys.PrintScreen },
        { "{RIGHT}", Keys.Right },
        { "{TAB}", Keys.Tab },
        { "{UP}", Keys.Up },
        { "{F1}", Keys.F1 },
        { "{F2}", Keys.F2 },
        { "{F3}", Keys.F3 },
        { "{F4}", Keys.F4 },
        { "{F5}", Keys.F5 },
        { "{F6}", Keys.F6 },
        { "{F7}", Keys.F7 },
        { "{F8}", Keys.F8 },
        { "{F9}", Keys.F9 },
        { "{F10}", Keys.F10 },
        { "{F11}", Keys.F11 },
        { "{F12}", Keys.F12 },
        { "{F13}", Keys.F13 },
        { "{F14}", Keys.F14 },
        { "{F15}", Keys.F15 },
        { "{F16}", Keys.F16 },
    };
}

---

By cheating a bit and hard-coding the "Ctrl+" part in the 2-step hotkey handler (I'll get the hotkey modifiers in there, that's a given), I got this screenshot:

..which shows that the mechanism works as intended:

• When a single-step hotkey is pressed, it gets handled immediately.
• When a two-step hotkey is pressed, the RubberduckHooks enters a "gimmeh teh key" mode that "eats" the next key (it won't show up in the code pane, and the low-level keyboard hook will ignore it), and it's up to the App class to determine which action to execute based on what the pressed key was.
• Typing code in a code pane in the VBE still triggers a parse task that processes the modified code module.

My next implementation step will be to tweak my CommandMenuItem objects to include either some hotkey string (e.g. "Ctrl+R,R" or "Ctrl+T"), or two Keys properties (e.g. HotKeyStep1 and HotKeyStep2, where step 1 would include the modifier key(s), and step 2 would be Keys.None for a single-step hotkey), so that the App class can cycle through its menus and determine which command needs to be executed, if any.

But before I do that, is there anything I need to know about this current implementation?

Answer 1

Your bit twiddling is a bit odd:

/// <summary>
/// Gets the integer portion of a word
/// </summary>
private static int LoWord(int dw)
{
    return (dw & 0x8000) != 0
        ? 0x8000 | (dw & 0x7FFF)
        : dw & 0xFFFF;
}

Is the same as just doing:

/// <summary>
/// Gets the integer portion of a word
/// </summary>
private static int LoWord(int dw)
{
    return dw & 0xFFFF;
}

Which both return the low 16 bit word.

According to msdn

The low-order word specifies whether the window is being activated or deactivated. [snipped]. The high-order word specifies the minimized state of the window being activated or deactivated. A nonzero value indicates the window is minimized.

So you might also be interested in getting the higher 16 bits and checking if you're active but minimized.

Update:

I thought I'd a some other thoughts...

public interface IRubberduckHooks : IHook, IDisposable
{
    IEnumerable<IHook> Hooks { get; }
    void AddHook<THook>(THook hook) where THook : IHook;
}

I don't see why you need the AddHook method to be generic... You're only adding the hook to an internal list of IHook. I think you'd minimise noise by removing the generics:

public interface IRubberduckHooks : IHook, IDisposable
{
    IEnumerable<IHook> Hooks { get; }
    void AddHook(IHook hook);
}

This is also a bit odd:

switch (keyCode.Substring(0, 1))

You could just switch on the char directly:

switch (keyCode[0])
{
    case '{':

The default case doesn't need to check String.IsNullOrEmpty(keyCode) either as an exception would have been thrown when you enter the switch if the keycode was null or an empty string.

The fact that GetModifierValue has a string parameter passed by reference is less than ideal... Get in a method name normally screams idempotent to me - mutating my parameter is pretty unexpected. I think you have a concept that needs wrapping up in a class here. You're returning something called a shift and stripping characters out of the parameter - I'm sure it makes sense but it's not obvious what that sense is to me.

Your Empty property on HookEventArgs would be better backed by a field. Don't forget that classes default to referential equality and so Empty != Empty if you return a new instance each time.

Answer 2

In HookEventArgs, do you have multiple keys or a single one?

Because this : public Keys Key { get { return _key; } } is confusing!

Keys is an enum. According to the naming guideline, your enum should be pluralized if it's a Flag enum. I don't know if that's your case, but if so the property name should be Keys, otherwise the enum should be Key.

And shouldn't you check for null? Mat, I remember writing this comment everytime I review your code. We should have a chat :p

In App, the cyclomatic complexity must be pretty high. Because there's the todo in there, I can't really propose a refactor. Plus, well maybe you won't be able to refactor since it seems there's a specific order in the execution of your code. But maybe there are methods you could extract!

In RubberduckHooks, const members should be on top, and I think readonly members should be separated from the others, but I'm not sure.

I know we should use brackets all the time :

if (!IsAttached)
{
    return;
}

But, well... Maybe, just maybe, using this : if (!IsAttached) return; would be good. It removes LoC, and well, considering it's on the same line you can't really introduce bugs. But that's just my thing, I dislike having brackets everywhere for returns.

Finally, maybe it's just because I have very minimal knowledge of the winapi but this :

/// <summary>
/// Gets the integer portion of a word
/// </summary>
private static int LoWord(int dw)
{
    return (dw & 0x8000) != 0
        ? 0x8000 | (dw & 0x7FFF)
        : dw & 0xFFFF;
}

lacks comments. I have almost no idea what is happening in that method.

In HotKeyHook, fields/properties/constructor are mixed up.

Answer 3

Quick hit, terribly incomplete review...

You have several interfaces that define

void Attach();
void Detach();

These should be stored in a common interface. Maybe IAttachable or something.

Answer 4

Just one quick thought browsing over:

public class HookEventArgs : EventArgs
{
    private readonly Keys _key;

    public HookEventArgs(Keys key)
    {
        _key = key;
    }

    public Keys Key { get { return _key; } }

    public new static HookEventArgs Empty {get { return new HookEventArgs(Keys.None); }}
}

With regards to that last property, why new up one of these every time (since it is immutable)? I'd do this:

public class HookEventArgs : EventArgs
{
    private static readonly Lazy<HookEventArgs> _empty = new Lazy<HookEventArgs>(() => new HookEventArgs(Keys.None));

    private readonly Keys _key;

    public HookEventArgs(Keys key)
    {
        _key = key;
    }

    public Keys Key { get { return _key; } }

    public new static HookEventArgs Empty {get { return _empty.Value; }}
}

Answer 5

Some things I noticed:

IHook has an event handler called MessageReceived that takes type HookEventArgs. That's 2 very generically-named types and one very specific property name. I can't infer usages from the code provided, but a preliminary smell test tells me IHook and HookEventArgs need to be named more specifically, or MessageReceived is too specific of a name for the event. I can't really put my finger on what exactly it is, but it's kind of hard to follow what is supposed to be going on because the only thing correlating a hook with messaging behavior is a property name.

A hook that implements IHook that has a collection of IHooks and a method taking in a THook constrained to type IHook. So maybe the interface can benefit from covariance/contravariance with a type parameter and an in or out keyword, but it could also be that something is using inheritance where it doesn't need to be, too.

The code appears to be inching towards "an interface for every class" territory in some spots. IRubberDuckHooks may not be providing much use outside of unit testing; it obviously depends on the context, but sometimes it's ideal to simply make a method virtual over creating a new interface.

You may remember a past conversation where I expressed my feelings about dictionaries providing a means for easy mapping. If not, you might be able to guess them :) I don't really think they're a huge problem in and of themselves, but they can lead to fragile code since things that might be known at compile time are being evaluated at runtime (leading to possible misses that could be caught via compilation errors). It's one of those things where I'd probably do a quick gut check if I found myself frequently relying on them. In this case, I don't think there's too much risk. However, it does lead back to an interesting question: Why are we dealing with a string representation of a key at all? It may be that at some point, the only thing you've gotten back from an API is a string, but there could also be some intermediary custom "serialization" going on, in which case I would generally say that there's probably a better way to solve the problem that's currently being solved by representing a strongly-typed object with a string.

Whether or not IDisposable should be exposed on an interface versus its implementing type is subject to debate--it can be argued that having an interface implement it creates a leaky abstraction since there's really nothing present around how and what resources are getting released. In my personal experience, I frequently find that it's serving the purpose of // DON'T FORGET TO DISPOSE THIS!! in the code when really something needs to be fixed or moved elsewhere.

(Take this with a grain of salt) There is a lot of code here, and while it helps to provide context, a lot of this peripheral code is actually raising more questions as I go through it which makes me want to see even more context. Although it can be time-consuming, sometimes posting less code and following a line of questioning can be quite productive.