Skip to content

Implement extension side of destinations #1630

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

Closed
Tracked by #803
pokey opened this issue Jul 13, 2023 · 0 comments · Fixed by #1605
Closed
Tracked by #803

Implement extension side of destinations #1630

pokey opened this issue Jul 13, 2023 · 0 comments · Fixed by #1605

Comments

@pokey
Copy link
Member

pokey commented Jul 13, 2023

Extension side

  • Add getDestination(insertionMode: InsertionMode) to Target, and have default implementation in BaseTarget
  • Handle destination list by processing targets for each destination, then wrapping each output in a destination with proper insertion mode
  • Change type signature of run for actions that expect a destination to use Destination
  • In the case of "swap", it expects two actual targets (not destinations), but internally converts them to destinations when using them as such.
  • Change PositionTarget to be Destination and stop implementing Target for this type, removing all unnecessary attributes
  • Remove edit from Target
  • Might be worth tackling Switch "change" implementation to use editNew #773 while here

On the api surface, we'd modify ActionCommand something like the following:

interface PartialSimpleActionDescriptor {
  name: SimpleActionName;
  target: PartialTargetDescriptor;
}

interface PartialBringMoveActionDescriptor {
  name: "bring" | "move";
  source: PartialTargetDescriptor;
  destination: PartialDestinationDescriptor;
}

interface PartialSwapActionDescriptor {
  name: "swap";
  target1: PartialTargetDescriptor;
  target2: PartialTargetDescriptor;
}

interface PartialWrapWithPairedDelimiterActionDescriptor {
  name: "wrapWithPairedDelimiter";
  left: string;
  right: string;
  target: PartialTargetDescriptor;
}

type PartialActionDescriptor =
  | PartialSimpleActionDescriptor
  | PartialBringMoveActionDescriptor
  | PartialSwapActionDescriptor
  | PartialWrapWithPairedDelimiterActionDescriptor;

We probably don't need to create corresponding full action descriptor types, because we can probably just use local variables in the process of handling the action. Eg

function runAction(partialActionDescriptor: PartialActionDescriptor) {
  switch (partialActionDescriptor.name) {
    case "bring":
    case "move":
      const [sourceDescriptor, destinationTargetDescriptor] = inferFullTargets([
        partialActionDescriptor.source,
        partialActionDescriptor.destination.target,
      ]);

      // Note: we don't need to worry about final stages here because bring and move don't use them!
      const source = pipelineRunner.run(sourceDescriptor);
      const destination = pipelineRunner
        .run(destinationTargetDescriptor)
        .getDestination(partialActionDescriptor.destination.insertionMode);

      return action.run(source, destination);
    case "wrapWithPairedDelimiter":
      // Some duplication with default case, but not tooo bad
      const [targetDescriptor] = inferFullTargets([
        partialActionDescriptor.target,
      ]);
      const target = pipelineRunner.run(targetDescriptor);

      return action.run(
        target,
        partialActionDescriptor.left,
        partialActionDescriptor.right
      );
    default:
      // TODO: Could prob expose `inferFullTarget` (singular) that takes single descriptor with no previousTargets
      // eg const targetDescriptor = inferFullTarget(partialActionDescriptor.target)
      const [targetDescriptor] = inferFullTargets([
        partialActionDescriptor.target,
      ]);

      // TODO: Do we want to handle final stages in the generic case?  Most
      // actions don't have them anyway so we might want special cases for them
      const target = pipelineRunner.run(targetDescriptor);

      return action.run(target);
  }
}

Fwiw if we don't need post-inference action descriptor types, then maybe we should remove Partial from these names, eg just BringMoveActionDescriptor

Note that it's also tempting to just inline the whole ActionDescriptor into the Command object itself. Not sure what the nested object is buying us here

github-merge-queue bot pushed a commit that referenced this issue Jul 13, 2023
- Fixes #1630
- Partially #1351

## Checklist

- [ ] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [ ] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [ ] I have not broken the cheatsheet

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Co-authored-by: Pokey Rule <[email protected]>
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 a pull request may close this issue.

1 participant