Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command is too tightly coupled with KeyBindings #3778

Closed
tig opened this issue Oct 7, 2024 · 1 comment · Fixed by #3880
Closed

Command is too tightly coupled with KeyBindings #3778

tig opened this issue Oct 7, 2024 · 1 comment · Fixed by #3880
Assignees
Labels
bug design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...)
Milestone

Comments

@tig
Copy link
Collaborator

tig commented Oct 7, 2024

In

I found View.KeyBindings, View.AddCommand, and View.InvokeCommand, etc... to work really well. However, I also realized the implementation is way too coupled with KeyBindings`. Specifically, there are many cases where commands need to be invoked from mouse events.

In fact, I now believe the correct fix for

is to build the mouse equvalent of View.KeyBindings: View.MouseBindings so devs could do:

AddCommand (Command.Context, ShowContextMenu);

KeyBindings.Add (Application.ContextKey, Command.Context);

MouseBindings.Add (MouseFlags.Button3Clicked, Command.Context); // btn3 is right

To address this I'd like to refactor KeyBindings to be generic (KeyBindings<Key>, KeyBindings<MouseFlags>,?) and make the Command API not have any KeyBinding specific knowledge. For example,:

public bool? InvokeCommand (Command command, Key? key = null, KeyBinding? keyBinding = null)...

would become

```cs
public bool? InvokeCommand<BindingType> (Command command, BindingType binding)

In #3677 I tweaked CommandContext to be able to hold arbitrary context data:

public record struct CommandContext
{
    /// <summary>
    ///     Initializes a new instance of <see cref="CommandContext"/> with the specified <see cref="Command"/>,
    /// </summary>
    /// <param name="command"></param>
    /// <param name="key"></param>
    /// <param name="keyBinding"></param>
    /// <param name="data"></param>
    public CommandContext (Command command, Key? key, KeyBinding? keyBinding = null, object? data = null)
    {
        Command = command;
        Key = key;
        KeyBinding = keyBinding;
        Data = data;
    }

    /// <summary>
    ///     The <see cref="Command"/> that is being invoked.
    /// </summary>
    public Command Command { get; set; }

    /// <summary>
    ///     The <see cref="Key"/> that is being invoked. This is the key that was pressed to invoke the <see cref="Command"/>.
    /// </summary>
    public Key? Key { get; set; }

    /// <summary>
    /// The KeyBinding that was used to invoke the <see cref="Command"/>, if any.
    /// </summary>
    public KeyBinding? KeyBinding { get; set; }

    /// <summary>
    ///     Arbitrary data.
    /// </summary>
    public object? Data { get; set; }
}

This struct needs to be like this instead:

/// <summary>
///     Provides context for a <see cref="Command"/> that is being invoked.
/// </summary>
/// <remarks>
///     <para>
///         To define a <see cref="Command"/> that is invoked with context,
///         use <see cref="View.AddCommand(Command,Func{CommandContext,System.Nullable{bool}})"/>.
///     </para>
/// </remarks>
#pragma warning restore CS1574 // XML comment has cref attribute that could not be resolved
public record struct CommandContext <BindingType>
{
    /// <summary>
    ///     Initializes a new instance of <see cref="CommandContext"/> with the specified <see cref="Command"/>,
    /// </summary>
    /// <param name="command"></param>
    /// <param name="key"></param>
    /// <param name="binding"></param>
    /// <param name="data"></param>
    public CommandContext (Command command, BindingType? binding, object? data = null)
    {
        Command = command;
        Binding = binding;
        Data = data;
    }

    /// <summary>
    ///     The <see cref="Command"/> that is being invoked.
    /// </summary>
    public Command Command { get; set; }

    /// <summary>
    /// The keyboard or mouse minding that was used to invoke the <see cref="Command"/>, if any.
    /// </summary>
    public BindingType? Binding { get; set; }

    /// <summary>
    ///     Arbitrary data.
    /// </summary>
    public object? Data { get; set; }
}
@tznind
Copy link
Collaborator

tznind commented Nov 25, 2024

MouseBindings.Add (MouseFlags.Button3Clicked, Command.Context); // btn3 is right

I really like this idea! simplifies a lot and builds on the solid foundation of bindings.

In its initial implementation Command was completely seperate from Keybindings. But agree that they have now bled together a lot with context etc.

@tig tig self-assigned this Dec 5, 2024
@tig tig closed this as completed in #3880 Dec 10, 2024
tig added a commit that referenced this issue Dec 10, 2024
Fixes #3778: Decouples `Command` from `KeyBindings`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...)
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants