patterncsharpMinor
Step by Step, ooh HotKey
Viewed 0 times
stephotkeyooh
Problem
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
The
Then I extended
I also extracted a
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
Purely for context, here's how the
```
private async void hooks_MessageReceived(object sender, HookEventArgs e)
{
if (sender is LowLevelKeyboardHook)
{
if (_skipKeyUp)
{
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 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 Hooks { get; }
void AddHook(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)
{
Solution
In
Because this :
And shouldn't you check for
In
In
I know we should use brackets all the time :
But, well... Maybe, just maybe, using this :
Finally, maybe it's just because I have very minimal knowledge of the winapi but this :
lacks comments. I have almost no idea what is happening in that method.
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 :pIn
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 :
///
/// Gets the integer portion of a word
///
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.Code Snippets
if (!IsAttached)
{
return;
}/// <summary>
/// Gets the integer portion of a word
/// </summary>
private static int LoWord(int dw)
{
return (dw & 0x8000) != 0
? 0x8000 | (dw & 0x7FFF)
: dw & 0xFFFF;
}Context
StackExchange Code Review Q#113498, answer score: 7
Revisions (0)
No revisions yet.