Skip to content

Conversation

@teslae1
Copy link

@teslae1 teslae1 commented Dec 18, 2025

What this PR does / why we need it:
Now only constructs the action on match. The logic for this is now moved to a small wrapper called "ActionMatcher".

Which issue(s) this PR fixes
This pr fixes the TODO:

// TODO: Constructing up to several hundred Actions every time we hit a key is moronic

…ow only constructing new ones on action match - which we need since state gets mutated upon action match construction
@teslae1
Copy link
Author

teslae1 commented Dec 18, 2025

Hey @J-Fields first of all thanks a lot for your contributions to this extension - like many other developers I rely heavily upon this code everyday.

In this pull request I did my best to make a small change that resolves your TODO comment. I was wondering how I would go about getting this reviewed?

I ask this because I'm also awaiting a review on #9872

@MGessinger
Copy link

I am not the package maintainer, but if I may propose an alternative solution:

The "TODO" itself mentions making the functions doesActionApply and couldActionApply static. Doing that literally doesn't work because it requires static overrides (which doesn't exist), but the methods don't have to be class methods anyway. All the info needed to make the decision whether an action does or could apply is essentially type information. So instead, I would propose the following:

Turn the decorator RegisterAction into a "mixin" (more or less) as follows:
First, define an interface

interface IActionDescription {
    keys: readonly string[] | readonly string[][];
    modes: readonly Mode[];
    actionType: readonly ActionType;
}

Declare the argument of RegisterAction as IActionDescription. Now, when the decorator is placed on a class, the static properties keys, modes and actionType are required on this class (note: static!).
Next, remove doesActionApply and couldActionApply from BaseAction, and turn them into local functions in base.ts or perhaps a helper class.
Now RegisterAction can be rewritten to operate on the type descriptors rather than needing an instance.

Lastly, the issue of inheritance has to be solved, since some actions override the couldActionApply method. To tackle this, the best idea I've had yet is to allow an optional property to the IActionDesceiption interface:

couldActionApply?: (
    vimState: VimState,
    keysPressed: string[],
    base: (vimState: VimState, keysPressed: string[]) => boolean
);

An action can then decide to implement this method (with enforced typing!) and it may or may not call the base implementation. This essentially mimics inheritance but through composition instead.

Although this requires a substantially larger refactoring than the one in this PR, I think it reduces the memory footprint of checking actions down to zero.

@teslae1
Copy link
Author

teslae1 commented Jan 4, 2026

I am not the package maintainer, but if I may propose an alternative solution:

The "TODO" itself mentions making the functions doesActionApply and couldActionApply static. Doing that literally doesn't work because it requires static overrides (which doesn't exist), but the methods don't have to be class methods anyway. All the info needed to make the decision whether an action does or could apply is essentially type information. So instead, I would propose the following:

Turn the decorator RegisterAction into a "mixin" (more or less) as follows: First, define an interface

interface IActionDescription {
    keys: readonly string[] | readonly string[][];
    modes: readonly Mode[];
    actionType: readonly ActionType;
}

Declare the argument of RegisterAction as IActionDescription. Now, when the decorator is placed on a class, the static properties keys, modes and actionType are required on this class (note: static!). Next, remove doesActionApply and couldActionApply from BaseAction, and turn them into local functions in base.ts or perhaps a helper class. Now RegisterAction can be rewritten to operate on the type descriptors rather than needing an instance.

Lastly, the issue of inheritance has to be solved, since some actions override the couldActionApply method. To tackle this, the best idea I've had yet is to allow an optional property to the IActionDesceiption interface:

couldActionApply?: (
    vimState: VimState,
    keysPressed: string[],
    base: (vimState: VimState, keysPressed: string[]) => boolean
);

An action can then decide to implement this method (with enforced typing!) and it may or may not call the base implementation. This essentially mimics inheritance but through composition instead.

Although this requires a substantially larger refactoring than the one in this PR, I think it reduces the memory footprint of checking actions down to zero.

@MGessinger Thank you for your proposal - I appreciate the time you took.

  1. I am curious as to wether your statement "whether an action does or could apply is essentially type information" is actually true. An example of code that might disprove this is the "ChangeLine" action (from actions.ts) which uses the current configuration when applying the methods "doesActionApply" and "couldActionApply":
@RegisterAction
class ChangeLine extends BaseCommand {
  modes = [Mode.Normal];
  keys = ['S'];
  override runsOnceForEachCountPrefix = false;

  public override async exec(position: Position, vimState: VimState): Promise<void> {
    await new operator.ChangeOperator(this.multicursorIndex).runRepeat(
      vimState,
      position,
      vimState.recordedState.count || 1,
    );
  }

  // Don't clash with sneak
  public override doesActionApply(vimState: VimState, keysPressed: string[]): boolean {
    return super.doesActionApply(vimState, keysPressed) && !configuration.sneak;
  }

  public override couldActionApply(vimState: VimState, keysPressed: string[]): boolean {
    return super.couldActionApply(vimState, keysPressed) && !configuration.sneak;
  }
}
  1. I think its important to focus on what we are trying to achieve here - in my opinion this code touches a "hot path" in this extension since it is being called once every keypress so we need to solve for performance + not introducing new bugs.
    So while your suggestion (if viable) lowers the memory usage further I don't know if we need to put that much work and code into saving more memory - however I was not the creator of the TODO comment so I might be wrong

  2. In my own experience as a noob in this codebase the current design of inheritance where the actual "doesActionApply" function is defined immediately within the class that is also the action is simple and easy to understand - so we should have a very good reason to remove this design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants