Hooked on Windows

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
{
    /// 
    /// Raised when system keyhook captures a keypress in the VBE.
    /// 
    event EventHandler KeyPressed;

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

    /// 
    /// Removes all hotkey hooks and detaches low-level keyboard hook.
    /// 
    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 _hookedKeys = new Dictionary();

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

/// 
/// Registers specified delegate for specified key combination.
/// 
/// The key combination string, including modifiers ('+': Shift, '%': Alt, '^': Control).
/// Any void, parameterless method that handles the hotkey.
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 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)
    {

StackExchange Code Review Q#113245, answer score: 18