Hooked on Windows

Question

The code that was just added to Rubberduck will allow us to set up hotkeys for our features, so that next release, Ctrl+Shift+R brings up the refactor/rename dialog, for example.

The requirements were roughly:

• Let client code know when a keypress is captured in a code pane
• Enable registration and handling of hotkeys

So the interface looks like this:

public interface IKeyHook
{
    /// <summary>
    /// Raised when system keyhook captures a keypress in the VBE.
    /// </summary>
    event EventHandler<KeyHookEventArgs> KeyPressed;

    /// <summary>
    /// Registers specified delegate for specified key combination.
    /// </summary>
    /// <param name="key">The key combination string, including modifiers ('+': Shift, '%': Alt, '^': Control).</param>
    /// <param name="action">Any <c>void</c>, parameterless method that handles the hotkey.</param>
    void OnHotKey(string key, Action action = null);

    /// <summary>
    /// Removes all hotkey hooks and detaches low-level keyboard hook.
    /// </summary>
    void UnHookAll();
}

I already had a KeyHook class that handled the low-level key hook for the parser, so I decided to move all the private static extern declarations to their dedicated User32 and Kernel32 static classes, and then it felt natural (but is it?) to implement the hotkey hook in a class named KeyHook, so here it is:

public class KeyHook : IKeyHook, IDisposable
{
    private readonly VBE _vbe;

    private readonly IDictionary<IntPtr,HookInfo> _hookedKeys = new Dictionary<IntPtr,HookInfo>();

    private const int GWL_WNDPROC = -4;
    private const int WA_INACTIVE = 0;
    private const int WA_ACTIVE = 1;

    private readonly User32.TimerProc _timerProc;
    private readonly User32.WndProc _oldWndProc;
    private readonly IntPtr _oldWndPointer;
    private readonly User32.WndProc _newWndProc;
    private readonly IntPtr _hWndVbe;

    private bool _isRegistered = false;

    private readonly User32.HookProc _proc;
    private static IntPtr HookId = IntPtr.Zero;


    private static IntPtr SetHook(User32.HookProc proc)
    {
        using (var curProcess = Process.GetCurrentProcess())
        using (var curModule = curProcess.MainModule)
        {
            return User32.SetWindowsHookEx(WindowsHook.KEYBOARD_LL, proc, Kernel32.GetModuleHandle(curModule.ModuleName), 0);
        }
    }

    private static readonly Keys[] IgnoredKeys = 
    {
        Keys.Down,
        Keys.Up,
        Keys.Left,
        Keys.Right,
        Keys.PageDown,
        Keys.PageUp,
        Keys.CapsLock,
        Keys.Escape,
        Keys.Home,
        Keys.End,
        Keys.Shift,
        Keys.ShiftKey,
        Keys.LShiftKey,
        Keys.RShiftKey,
        Keys.Control,
        Keys.ControlKey,
        Keys.LControlKey,
        Keys.RControlKey,
    };

    private IntPtr HookCallback(int nCode, IntPtr wParam, IntPtr lParam)
    {
        if (_vbe.ActiveWindow == null || _vbe.ActiveWindow.Type != vbext_WindowType.vbext_wt_CodeWindow)
        {
            // don't do anything if not in a code window
            return User32.CallNextHookEx(HookId, nCode, wParam, lParam);
        }

        var vkCode = Marshal.ReadInt32(lParam);
        var key = (Keys)vkCode;

        var windowHandle = User32.GetForegroundWindow();
        var codePane = _vbe.ActiveCodePane;

        Task.Run(() =>
        {
            if (windowHandle != _hWndVbe
                || wParam != (IntPtr) WM.KEYUP 
                || nCode < 0 
                || codePane == null
                || IgnoredKeys.Contains(key))
            {
                return;
            }

            var component = codePane.CodeModule.Parent;
            var args = new KeyHookEventArgs(key, component);
            OnKeyPressed(args);
        });

        return User32.CallNextHookEx(HookId, nCode, wParam, lParam);
    }

    public KeyHook(VBE vbe)
    {
        _vbe = vbe;
        _hWndVbe = (IntPtr)_vbe.MainWindow.HWnd;
        _newWndProc = WindowProc;
        _oldWndPointer = User32.SetWindowLong(_hWndVbe, (int)WindowLongFlags.GWL_WNDPROC, _newWndProc);
        _oldWndProc = (User32.WndProc)Marshal.GetDelegateForFunctionPointer(_oldWndPointer, typeof(User32.WndProc));

        _timerProc = TimerCallback;
        _proc = HookCallback;
    }

    private void Attach()
    {
        HookId = SetHook(_proc);
    }

    private void Detach()
    {
        User32.UnhookWindowsHookEx(HookId);
    }

    public event EventHandler<KeyHookEventArgs> KeyPressed;

    private void OnKeyPressed(KeyHookEventArgs e)
    {
        var handler = KeyPressed;
        if (handler != null)
        {
            handler.Invoke(this, e);
        }
    }

    public void Dispose()
    {
        Detach();
    }

    public void OnHotKey(string key, Action action = null)
    {
        var hotKey = key;
        var lShift = GetModifierValue(ref hotKey);
        var lKey = GetKey(hotKey);

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

        if (action == null)
        {
            UnHookKey((uint)lKey, lShift);
        }
        else
        {
            HookKey((uint)lKey, lShift, action);
        }
    }

    /// <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 lShift = 0;
        for (var i = 0; i < 3; i++)
        {
            var firstChar = key.Substring(0, 1);
            if (firstChar == "+")
            {
                lShift |= (uint)KeyModifier.SHIFT;
            }
            else if (firstChar == "%")
            {
                lShift |= (uint)KeyModifier.ALT;
            }
            else if (firstChar == "^")
            {
                lShift |= (uint)KeyModifier.CONTROL;
            }
            else
            {
                break;
            }

            key = key.Substring(1);
        }
        return lShift;
    }

    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 },
    };

    private IntPtr _timerId;

    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 void HookKey(uint keyCode, uint shift, Action action)
    {
        UnHookKey(keyCode, shift);

        if (!_hookedKeys.Any())
        {
            HookWindow();
        }

        var hookId = (IntPtr)Kernel32.GlobalAddAtom(Guid.NewGuid().ToString());
        var success = User32.RegisterHotKey(_hWndVbe, hookId, shift, keyCode);
        Debug.Print("RegisterHotKey(hWnd:{0},id:{1},modifiers:{2},vk:{3}) returned {4}", _hWndVbe, hookId, shift, keyCode, success);

        if (success)
        {
            _hookedKeys.Add(hookId,new HookInfo(hookId, keyCode, shift, action));
            Debug.Print("Added hook id {0} for keycode {1}", hookId, keyCode);
            _isRegistered = true;
        }
    }

    private void UnHookKey(uint keyCode, uint shift)
    {
        var hooks = _hookedKeys.Where(hook => hook.Value.KeyCode == keyCode && hook.Value.Shift == shift).ToList();
        foreach (var hook in hooks.Select(h => h.Value))
        {
            User32.UnregisterHotKey(_hWndVbe, hook.HookId);
            Kernel32.GlobalDeleteAtom((ushort)hook.HookId);
            Debug.Print("Removing hook id {0} (key code {1})", hook.HookId, keyCode);
            _hookedKeys.Remove(hook.HookId);

            if (!_hookedKeys.Any())
            {
                UnHookWindow();
                break;
            }
        }
    }

    /// <summary>
    /// Called when hook form goes out of scope, to remove all hooks.
    /// </summary>
    public void UnHookAll()
    {
        Debug.Print("Unhook all...");
        foreach (var hook in _hookedKeys.Values)
        {
            User32.UnregisterHotKey(_hWndVbe, hook.HookId);
            Kernel32.GlobalDeleteAtom((ushort)hook.HookId);
        }

        UnHookWindow();
        Detach();
    }

    private void HookWindow()
    {
        Debug.Print("HookWindow...");
        try
        {
            var timerId = (IntPtr)Kernel32.GlobalAddAtom(Guid.NewGuid().ToString());
            User32.SetTimer(_hWndVbe, timerId, 500, _timerProc);
        }
        catch (Exception exception)
        {
            Console.WriteLine(exception);
        }
    }

    private void UnHookWindow()
    {
        Debug.Print("UnHookWindow...");
        try
        {
            User32.SetWindowLong(_hWndVbe, (int)WindowLongFlags.GWL_WNDPROC, _oldWndProc);
            _isRegistered = false;

            User32.KillTimer(_hWndVbe, _timerId);
            Kernel32.GlobalDeleteAtom((ushort)_timerId);

            _timerId = IntPtr.Zero;
        }
        catch (Exception exception)
        {
            Console.WriteLine(exception);
        }
    }

    private IntPtr WindowProc(IntPtr hWnd, int uMsg, int wParam, int lParam)
    {
        Debug.Print("WindowProc message hook received WM{0}", (WM)uMsg);
        try
        {
            var processed = false;
            if (hWnd == _hWndVbe)
            {
                switch ((WM)uMsg)
                {
                    case WM.HOTKEY:
                        Debug.Print("WindowProc message hook handles WM{0} message (hWnd {1})", (WM)uMsg, hWnd);
                        if (GetWindowThread(User32.GetForegroundWindow()) == GetWindowThread(_hWndVbe))
                        {

                            var key = _hookedKeys.FirstOrDefault(k => k.Key == (IntPtr)wParam).Value;
                            if (key.Action != null)
                            {
                                key.Action.Invoke();
                                processed = true;
                            }
                        }
                        else
                        {
                            Debug.Print("Mismatching WindowThreadId between foreground window and VBE mainwindow.");
                        }
                        break;

                    case WM.ACTIVATEAPP:
                        Debug.Print("WindowProc message hook handles WM{0} message (hWnd {1})", (WM)uMsg, hWnd);
                        switch (LoWord(wParam))
                        {
                            case WA_ACTIVE:
                                Debug.Print("handling WA_ACTIVE...");
                                foreach (var key in _hookedKeys.Values)
                                {
                                    var result = User32.RegisterHotKey(_hWndVbe, key.HookId, (uint)KeyModifier.CONTROL, key.KeyCode);
                                    Debug.Print("RegisterHotKey({0},{1},{2},{3}) returned {4}", hWnd, key.HookId, (uint)KeyModifier.CONTROL, key.KeyCode, result);
                                }
                                _isRegistered = true;
                                break;

                            case WA_INACTIVE:
                                Debug.Print("handling WA_INACTIVE...");
                                foreach (var key in _hookedKeys.Values)
                                {
                                    var result = User32.UnregisterHotKey(_hWndVbe, key.HookId);
                                    Debug.Print("Unregistered hotkey {0} (hWnd {1}) returned {2}", key.HookId, hWnd, result);
                                }
                                _isRegistered = false;
                                break;

                            default:
                                Debug.Print("WMACTIVATEAPP wParam {0} not processed", wParam);
                                break;
                        }

                        break;
                }
            }

            if (!processed)
            {
                Debug.Print("WindowProc message hook ignored WM{0} message (hWnd {1})", (WM)uMsg, hWnd);
                return User32.CallWindowProc(_oldWndProc, hWnd, uMsg, wParam, lParam);
            }
        }
        catch (Exception exception)
        {
            Console.WriteLine(exception);
        }

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

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

        return (IntPtr)hThread;
    }

    /// <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 void TimerCallback(IntPtr hWnd, WindowLongFlags msg, IntPtr timerId, uint time)
    {
        // check if the VBE is still in the foreground
        if (User32.GetForegroundWindow() == _hWndVbe && !_isRegistered)
        {
            // app got focus, re-register hotkeys and re-attach key hook
            foreach (var key in _hookedKeys.Values)
            {
                User32.RegisterHotKey(_hWndVbe, key.HookId, key.Shift, key.KeyCode);
            }
            _isRegistered = true;
            Attach();
        }
        else
        {
            // app lost focus, unregister hotkeys and detach key hook
            foreach (var key in _hookedKeys.Values)
            {
                User32.UnregisterHotKey(_hWndVbe, key.HookId);
            }
            _isRegistered = false;
            Detach();
        }
    }
}

Brutal, I know. The HookCallback is the low-level hook handler that ultimately fires up a parser task whenever a code module gets modified [by typing]. WindowProc is the handler that receives all the Windows messages sent to the VBE's main window; whenever the VBE loses focus, hooks detach - when the VBE gets reactivated, hooks are re-attached.

Quite a bit of this (everything related to hotkeys) is C# code I wrote, derived from my understanding of the Smart Indenter VB6 source code that was graciously offered to the Rubberduck project, but also possibly suffers from my understanding of p/invoke. I'm just happy to finally have something that works.

I think need to refactor this; I don't like all the casting that's going on, and I find the class quite massive. I'm thinking of taking the hotkey stuff out of there, but then the hotkey stuff alone is quite massive - how would I best rearrange this code?

_{(view it on GitHub)}

Answer 1

/// <summary>
/// Registers specified delegate for specified key combination.
/// </summary>
/// <param name="key">The key combination string, including modifiers ('+': Shift, '%': Alt, '^': Control).</param>
/// <param name="action">Any <c>void</c>, parameterless method that handles the hotkey.</param>
void OnHotKey(string key, Action action = null);  

Having some OnXXX method always remind me about OnEvent hence it should be used for events only. The summary of that method states Registers specified delegate for specified key combination. which isn't what the implementation of this interface method is doing. The implementation is doing too much because it is, based on the value of the action, either register or unregister a hook for a key.

What I would do here is having two methods, Register(string key, Action action) and UnRegister(string key) to cleary distinguish between the two actions.

---

Inside the GetModifierValue() method you are using too many strings. You could consider to use char instead like so

private static uint GetModifierValue(ref string key)
{
    int i;
    uint lShift = 0;
    for (i = 0; i < 3; i++)
    {
        var firstChar = key[i];
        if (firstChar == '+')
        {
            lShift |= (uint)KeyModifier.SHIFT;
        }
        else if (firstChar == '%')
        {
            lShift |= (uint)KeyModifier.ALT;
        }
        else if (firstChar == '^')
        {
            lShift |= (uint)KeyModifier.CONTROL;
        }
        else
        {
            break;
        }

    }
    key = key.Substring(i + 1);
    return lShift;
}  

in this way the variables name firstChar won't lie anymore about its type.

However it is more dangerous that you don't ever evaluate the passed key to have at least 4 characters. You should do this in the (hopefully former) OnHotKey() method.

---

I ain't be that much into com and interop, but you are using a lot of IntPtr which IMO should be freed properly. In addition you should use a proper IDisposable pattern to do this without the possibility of exceptions. I don't know if it is needed to "unhook" the key hooks before calling Detach() method inside the Dispose() method.

---

Because _hookedKeys is a IDictionary<TKey, TValue> you should use the property Count instead of the extensionmethod Any() to check if it contains anything like so

private void HookKey(uint keyCode, uint shift, Action action)
{
    UnHookKey(keyCode, shift);

    if (_hookedKeys.Count == 0)
    {
        HookWindow();
    }  

---

Its better(faster) to check in an if condition a bool value at first. So this

private void TimerCallback(IntPtr hWnd, WindowLongFlags msg, IntPtr timerId, uint time)
{
    // check if the VBE is still in the foreground
    if (User32.GetForegroundWindow() == _hWndVbe && !_isRegistered)
    {  

should become this

private void TimerCallback(IntPtr hWnd, WindowLongFlags msg, IntPtr timerId, uint time)
{
    // check if the VBE is still in the foreground
    if (!_isRegistered && User32.GetForegroundWindow() == _hWndVbe)
    {  

Answer 2

1. I think you should probably group public members at the top of your class (constructor being first) and private members at the bottom. It is quite hard to navigate classes where public and private methods are mixed together.

2. Your class clearly has more then one responsibility. You should at least extract parsing logic (GetKey, GetModifierKey, etc.) to separate entity. You might also want to extract hooking logic:

   //this interface should encapsualte your hooking logic
   //and fire an event whenever key is pressed
   interface IKeySource
   {
       event EventHandler<KeyPressedArgs> KeyPressed;
   }
   
   //this interface should manage defined hotkeys
   //and subscribe to IKeySource.KeyPressed event
   interface IHotkeyManager
   {
       void Register(...);
       void Unregister(...);
   }
   

3. Why would you register your hooks as global system hotkeys and then write lots of code just to check if your application is active? What's the point in having global hotkeys then? It does not make sense. Shouldn't you use local application hooks instead? Here is a hook I wrote a while back when I needed a bunch of tricky hotkeys:

   public class LocalKeyboardHook : IDisposable
   {
       public LocalKeyboardHook()
       {
           _hookCallback = new HooksApi.HookProc(OnMessageReceived);
           _hook = HooksApi.SetWindowsHookEx(HooksApi.HookType.WH_KEYBOARD, _hookCallback, IntPtr.Zero, AppDomain.GetCurrentThreadId());
       }
   
       public event Action<KeyboardHookEventArgs> Triggered;
   
       public void Dispose()
       {
           HooksApi.UnhookWindowsHookEx(_hook);
       }
   
       private readonly HooksApi.HookProc _hookCallback;
       private readonly IntPtr _hook;
   
       private IntPtr OnMessageReceived(int nCode, int wParam, IntPtr lParam)
       {
           if ((HookCode)nCode == HookCode.HC_ACTION)
           {
               var args = new KeyboardHookEventArgs((VirtualKeys) wParam, lParam.ToInt64());
               Invoke.Action(Triggered, args);
               if (args.Handled)
               {
                   return (IntPtr)1;
               }
           }
           return HooksApi.CallNextHookEx(_hook, nCode, wParam, lParam);
       }
   }
   

   Hooking to application message pump should be easy (as long as you are willing to spend days digging through windows api documentation and interoping, ofc). Your code is super complicated though! To the point, where I am surprised it works at all. Maybe I am missing some crucial context, but I do not understand why solution needs to be so complex.