Handling keyboard shortcuts in C# software

Question

Edit: I have added the revised code in an answer to this question.

---

Currently, I handle the keyboard shortcuts of my applications in a single huge method that looks like this:

    protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
    {
        // Next issue (validate the fix)
        if (keyData == (Keys.Control | Keys.Enter) || 
            keyData == (Keys.Shift | Keys.Enter))
        {
            mark_as_fixed();
            return true;
        }
        // Skip to next issue without validating or changing anything
        if (keyData == (Keys.Alt | Keys.Down))
        {
            next_issue();
        }
        // Previous issue
        if (keyData == (Keys.Alt | Keys.Up))
        {
            previous_issue();
            return true;
        }
        ... [130 lines of this]

It works perfectly, it isn't particularly hard to read or maintain, but having a method 130 lines long and constantly growing just feels wrong.

Also I may want to implement shortcut customization in the future, and there is no easy way to find out what keys are mapped to which method so it will have to be rewritten.

Since all non trivial applications have a gazillion shortcuts, I assume it's a solved problem, but I couldn't find a good explanation on how applications handle their shortcuts.

Answer 1

Reusing Mat's Mug's idea of a base Command class: what about using a Dictionary for the mapping?

This way you separate the ideas of command and shortcut (probably losing the potential help of reflection for programmatically instantiate your commands), but you can easily let your users map commands as they need.

Answer 2

Also I may want to implement shortcut customization in the future, and there is no easy way to find out what keys are mapped to which method so it will have to be rewritten.

Stop right here! Extract a class for each command (e.g. instead of a mark_as_fixed method, you'll have a MarkAsFixedCommand class). If you're using WPF you can implement the ICommand interface and have nevermind, you're using WinForms. Still, extract the command classes - WPF's ICommand is a really simple interface that merely exposes bool CanExcute(object) and void Execute(object) methods (you don't have to implement the object parameter if you don't need it) - by extracting your logic into command classes, you can prepare the ground for the day you redo your UI with WPF (which you should!).

there is no easy way to find out what keys are mapped to which method

Make one! Have your command classes derive from an abstract CommandBase class that exposes a method for it:

public abstract bool IsShortcutKey(Keys keys);

Now every derived command class has to implement this IsShortcutKey method. The implementation for MarkAsFixedCommand could look like this:

public override bool IsShortcutKey(Keys keys)
{ 
    return keys == (Keys.Control | Keys.Enter)
        || keys == (Keys.Shift | Keys.Enter); 
}

A cleaner way would have been to expose some Keys ShortcutKey { get; } property getter, but that wouldn't play nice with multiple shortcuts for the same command.

Anyway now that each command implementation is responsible for is shortcut key logic, you can easily fetch the configuration and make that method return true when keys matches whatever Keys value you have in your config.

Then, whoever runs this ProcessCmdKey method simply needs to know about all available commands - take them as an IEnumerable<CommandBase> constructor parameter and let whoever is calling this constructor deal with providing it with the command instances (which you will acquire with some reflection code).

Now ProcessCmdKey can be as simple as this:

// assume only 1 command returns true for specified keyData value:
var command = _commands.SingleOrDefault(cmd => cmd.IsShortcutKey(keyData));
if (command != null && command.CanExecute())
{
    command.Execute();
    return true;
}

return base.ProcessCmdKey(ref msg, keyData);

---

PS - Method names should be PascalCase, not lower_snake_case.

Answer 3

I use your method also except I would like to suggest using a case statement instead for readability.

I'm still a rookie programmer with less than a year of experience, so there could be better methods than this.

protected override bool ProcessCmdKey(ref Message msg, Keys keyData) 
    {
        switch (keyData)
        {
            case Keys.F1: 
                //Stuff
                return true;
            case Keys.F2:
                //Stuff
                return true;
            case Keys.Alt | Keys.Down:
                //Stuff
                return true;
        }
        return base.ProcessCmdKey(ref msg, keyData);
    }

Also don't forget your return base.ProcessCmdKey(ref msg, keyData); So you aren't disabling all other keys not specified.

Answer 4

I'd rather use a more flexible design where you or the user can configure the shortcuts.

In this design each operation would be an ICommand with two members:

public interface ICommand
{
    Keys Keys { get; }
    void Execute();
}

An abstract CommandBase class would implement the Keys property:

public abstract class CommandBase : ICommand
{
    protected CommandBase(Keys keys) { Keys = keys; }
    public Keys Keys { get; }
    public abstract void Execute();
}

and concrete types the Execute method:

public class LoadReportCommand : CommandBase
{
    public LoadReportCommand(Keys keys) : base(keys) { }
    public override void Execute() { }
}
public class MarkAsFixedCommand : CommandBase
{
    public MarkAsFixedCommand(Keys keys) : base(keys) { }
    public override void Execute() { }
}

I would then make the shortcuts configurable via the app.config or any other configuration. The key would be the command name and the the value the keys:

private static IEnumerable<ICommand> _commands;

void Main()
{
    // this would come from a configuration
    var commandShortCuts = new Dictionary<string, string>
    {
        ["LoadReport"] = "Control+O",
        ["MarkAsFixed"] = "Shift+Enter",
    };

    // get all commands in this assembly
    var commandTypes =
        Assembly.GetExecutingAssembly()
        .GetTypes()
        .Where(t => t.BaseType == typeof(CommandBase))
        .ToList();

    // initialize commands and their shortcuts
    _commands = commandShortCuts.Select(x =>
    {
        var keys = x.Value.Split('+').Aggregate(
            Keys.None, 
            (result, next) => 
                result |= (Keys)Enum.Parse(typeof(Keys), next));
        var commandType = commandTypes.SingleOrDefault(t => t.Name.Equals(x.Key + "Command"));
        return (ICommand)Activator.CreateInstance(commandType, keys);
    })
    .ToList();
}

Then you could execute a command like this:

protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
{
    var command = _commands.SingleOrDefault(c => c.Keys == keyData);
    command?.Execute();
}

Answer 5

This is what I have now, based on Mat's and Bel8z' answers:

Dictionary<Keys, Func<Keys, bool>> dic_shortcut_mngr = new Dictionary<Keys, Func<Keys, bool>>();
private void init_shortcuts()
{
    //Files
    dic_shortcut_mngr.Add((Keys.Control | Keys.O), open);
    dic_shortcut_mngr.Add((Keys.Control | Keys.L), load_report);
    // Operations
    dic_shortcut_mngr.Add((Keys.Control | Keys.Enter), mark_as_fixed);
    dic_shortcut_mngr.Add((Keys.Shift | Keys.Enter), mark_as_fixed);
    dic_shortcut_mngr.Add((Keys.Alt | Keys.Down), next_issue);
    dic_shortcut_mngr.Add((Keys.Alt | Keys.Up), previous_issue);
    dic_shortcut_mngr.Add((Keys.Insert), copy);
    //Copy items
    dic_shortcut_mngr.Add((Keys.Control | Keys.NumPad0), copy_item);
    dic_shortcut_mngr.Add((Keys.Control | Keys.NumPad1), copy_item);
    dic_shortcut_mngr.Add((Keys.Control | Keys.NumPad2), copy_item);
    ...
}

protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
{
    if (dic_shortcut_mngr.ContainsKey(keyData))
    {
        return dic_shortcut_mngr[keyData].Invoke(keyData);
    }
    return base.ProcessCmdKey(ref msg, keyData);
}
private bool copy_item(Keys key)
{
    //NumKeys start at index 96. Removing 96 gets the numkey's value 
    int i = (int)key - 96;
    if (dgv.Rows.Count > i && dgv.Rows[i].Cells["item"] != null)
    {
        string item = dgv.Rows[i].Cells["item"].Value.ToString();
        Clipboard.SetText(item);
        rtb_fix_target.Paste();
    }
    return true;
}

I decided to use Func<Keys, bool> instead of Action to be able to return "true" and pass on the keydata to the methods (which is useful handling the numpad shortcuts as they define the item to be copied)