Skip to content

Destinations as first-class objects #803

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.

Sign up for GitHub

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

Jump to bottom
Closed
2 tasks done
pokey opened this issue Jun 29, 2022 · 0 comments
Closed
2 tasks done

Destinations as first-class objects #803

pokey opened this issue Jun 29, 2022 · 0 comments
Assignees
@AndreasArvidsson
Labels
code quality Improvements to code quality enhancement New feature or request
Milestone
Short list

Comments

@pokey
Copy link
Member

pokey commented Jun 29, 2022 •
edited
Loading

The problem

Today, our handling of positions is inconsistent. This problem manifests itself in multiple ways:

  • We allow "end of" to appear in places that expect a position target, which means that "paste end of air" is a valid command, making it nearly impossible to get Talon to hear "take air past end of item bat". It instead hears two commands: "take air" then "paste end of item bat"
  • We need to have the edit action on regular targets, and position targets need all the methods from regular targets that they don't actually support
  • Targets have no strongly typed way of indicating that they expect a position
  • Position targets are hacked to set delimiter to "" when position is "end of" or "start of"

The solution

We introduce a new type called a Destination, both on the Talon and extension side. This type is distinct from a Target, and will be used everywhere we expect a PositionTarget today (eg "paste", second target of "bring", etc).

Destination will be have the following interface:

type InsertionMode = "before" | "after" | "to";

interface Destination {
  insertionMode: InsertionMode;
  target: Target;
}

It will exist in the following places:

  • As a capture Talon side
  • As a new descriptor type, similar to today's TargetDescriptor
  • As a new post-processTargets type, like today's Target

It will involve the following steps:
@pokey pokey added code quality Improvements to code quality enhancement New feature or request labels Jun 29, 2022
@pokey pokey mentioned this issue Jun 29, 2022
Open
@pokey pokey mentioned this issue Mar 24, 2023
Merged
3 tasks
pokey added a commit that referenced this issue Mar 27, 2023
@pokey
Support inline snippets; add Talon snippet api (#1329)
94bab18 
- Fixes #1324
- Fixes #1325 

Note that I created a Talon-side insertion api that supports the full
set of options, including substitutions, targets, and scopes, but
decided to leave it out for now because I'm not sure exactly how it
should look and we don't need it for the mathfly support we're planning
to use it for. In particular, we should probably figure out #803 before
we implement the more complex api because snippets really want a
destination, not a target. I did use the complex Talon-side api to
record some tests of the extension api, though

Here is the complex Talon snippet insertion api in case useful at some
point

<details><summary>Complex talon api</summary>

```diff
From e39e03e3a06a6db4f1edb245a5225037d0ea08d3 Mon Sep 17 00:00:00 2001
From: Pokey Rule <[email protected]>
Date: Fri, 24 Mar 2023 14:22:40 +0000
Subject: [PATCH] Complex insert snippet Talon api

---
 cursorless-talon/src/snippets.py | 35 ++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/cursorless-talon/src/snippets.py b/cursorless-talon/src/snippets.py
index a9f100f..409512011 100644
--- a/cursorless-talon/src/snippets.py
+++ b/cursorless-talon/src/snippets.py
@@ -91,15 +91,34 @@ class Actions:
             },
         )
 
-    def cursorless_insert_snippet(body: str):
+    def cursorless_insert_snippet(
+        body: str,
+        target: Optional[dict] = None,
+        scope: Optional[str] = None,
+        snippet_variable: Optional[str] = None,
+        text: Optional[str] = None,
+    ):
         """Inserts a custom snippet"""
-        actions.user.cursorless_implicit_target_command(
-            "insertSnippet",
-            {
-                "type": "custom",
-                "body": body,
-            },
-        )
+        snippet_arg: dict[str, Any] = {
+            "type": "custom",
+            "body": body,
+        }
+        if scope:
+            snippet_arg["scopeType"] = {"type": scope}
+        if snippet_variable:
+            snippet_arg["substitutions"] = {snippet_variable: text}
+
+        if target:
+            actions.user.cursorless_single_target_command_with_arg_list(
+                "insertSnippet",
+                target,
+                [snippet_arg],
+            )
+        else:
+            actions.user.cursorless_implicit_target_command(
+                "insertSnippet",
+                snippet_arg,
+            )
 
     def cursorless_wrap_with_snippet_by_name(
         name: str, variable_name: str, target: dict
-- 
2.39.2

```

</details>

## Checklist

- [x] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [x] 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)
- [x] I have not broken the cheatsheet
cursorless-bot pushed a commit that referenced this issue Mar 27, 2023
@pokey
Support inline snippets; add Talon snippet api (#1329)
70a4ff9 
- Fixes #1324
- Fixes #1325 

Note that I created a Talon-side insertion api that supports the full
set of options, including substitutions, targets, and scopes, but
decided to leave it out for now because I'm not sure exactly how it
should look and we don't need it for the mathfly support we're planning
to use it for. In particular, we should probably figure out #803 before
we implement the more complex api because snippets really want a
destination, not a target. I did use the complex Talon-side api to
record some tests of the extension api, though

Here is the complex Talon snippet insertion api in case useful at some
point

<details><summary>Complex talon api</summary>

```diff
From e39e03e3a06a6db4f1edb245a5225037d0ea08d3 Mon Sep 17 00:00:00 2001
From: Pokey Rule <[email protected]>
Date: Fri, 24 Mar 2023 14:22:40 +0000
Subject: [PATCH] Complex insert snippet Talon api

---
 cursorless-talon/src/snippets.py | 35 ++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/cursorless-talon/src/snippets.py b/cursorless-talon/src/snippets.py
index a9f100f..409512011 100644
--- a/cursorless-talon/src/snippets.py
+++ b/cursorless-talon/src/snippets.py
@@ -91,15 +91,34 @@ class Actions:
             },
         )
 
-    def cursorless_insert_snippet(body: str):
+    def cursorless_insert_snippet(
+        body: str,
+        target: Optional[dict] = None,
+        scope: Optional[str] = None,
+        snippet_variable: Optional[str] = None,
+        text: Optional[str] = None,
+    ):
         """Inserts a custom snippet"""
-        actions.user.cursorless_implicit_target_command(
-            "insertSnippet",
-            {
-                "type": "custom",
-                "body": body,
-            },
-        )
+        snippet_arg: dict[str, Any] = {
+            "type": "custom",
+            "body": body,
+        }
+        if scope:
+            snippet_arg["scopeType"] = {"type": scope}
+        if snippet_variable:
+            snippet_arg["substitutions"] = {snippet_variable: text}
+
+        if target:
+            actions.user.cursorless_single_target_command_with_arg_list(
+                "insertSnippet",
+                target,
+                [snippet_arg],
+            )
+        else:
+            actions.user.cursorless_implicit_target_command(
+                "insertSnippet",
+                snippet_arg,
+            )
 
     def cursorless_wrap_with_snippet_by_name(
         name: str, variable_name: str, target: dict
-- 
2.39.2

```

</details>

## Checklist

- [x] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [x] 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)
- [x] I have not broken the cheatsheet
@pokey pokey mentioned this issue Apr 25, 2023
Closed
6 tasks
This was referenced Jun 9, 2023
Closed
Open
Merged
@pokey pokey mentioned this issue Jun 18, 2023
Closed
@AndreasArvidsson AndreasArvidsson self-assigned this Jul 5, 2023
@pokey pokey added this to the Short list milestone Jul 6, 2023
@AndreasArvidsson AndreasArvidsson mentioned this issue Jul 7, 2023
Closed
@pokey pokey mentioned this issue Jul 7, 2023
Merged
3 tasks
This was referenced Jul 10, 2023
Merged
Closed
@AndreasArvidsson AndreasArvidsson mentioned this issue Jul 30, 2023
Merged
11 tasks
@AndreasArvidsson AndreasArvidsson linked a pull request Jul 30, 2023 that will close this issue
Talon side spoken form tests #1637
Merged
11 tasks
@pokey pokey removed a link to a pull request Jul 31, 2023
Talon side spoken form tests #1637
Merged
11 tasks
@AndreasArvidsson AndreasArvidsson linked a pull request Aug 1, 2023 that will close this issue
Talon side spoken form tests #1637
Merged
11 tasks
@AndreasArvidsson AndreasArvidsson removed a link to a pull request Aug 1, 2023
Talon side spoken form tests #1637
Merged
11 tasks
thetomcraig-aya pushed a commit to thetomcraig/cursorless that referenced this issue Mar 27, 2024
@pokey @cursorless-bot
Support inline snippets; add Talon snippet api (cursorless-dev#1329)
24347dc 
- Fixes cursorless-dev#1324
- Fixes cursorless-dev#1325 

Note that I created a Talon-side insertion api that supports the full
set of options, including substitutions, targets, and scopes, but
decided to leave it out for now because I'm not sure exactly how it
should look and we don't need it for the mathfly support we're planning
to use it for. In particular, we should probably figure out cursorless-dev#803 before
we implement the more complex api because snippets really want a
destination, not a target. I did use the complex Talon-side api to
record some tests of the extension api, though

Here is the complex Talon snippet insertion api in case useful at some
point

<details><summary>Complex talon api</summary>

```diff
From e39e03e3a06a6db4f1edb245a5225037d0ea08d3 Mon Sep 17 00:00:00 2001
From: Pokey Rule <[email protected]>
Date: Fri, 24 Mar 2023 14:22:40 +0000
Subject: [PATCH] Complex insert snippet Talon api

---
 cursorless-talon/src/snippets.py | 35 ++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/cursorless-talon/src/snippets.py b/cursorless-talon/src/snippets.py
index a9f100f..409512011 100644
--- a/cursorless-talon/src/snippets.py
+++ b/cursorless-talon/src/snippets.py
@@ -91,15 +91,34 @@ class Actions:
             },
         )
 
-    def cursorless_insert_snippet(body: str):
+    def cursorless_insert_snippet(
+        body: str,
+        target: Optional[dict] = None,
+        scope: Optional[str] = None,
+        snippet_variable: Optional[str] = None,
+        text: Optional[str] = None,
+    ):
         """Inserts a custom snippet"""
-        actions.user.cursorless_implicit_target_command(
-            "insertSnippet",
-            {
-                "type": "custom",
-                "body": body,
-            },
-        )
+        snippet_arg: dict[str, Any] = {
+            "type": "custom",
+            "body": body,
+        }
+        if scope:
+            snippet_arg["scopeType"] = {"type": scope}
+        if snippet_variable:
+            snippet_arg["substitutions"] = {snippet_variable: text}
+
+        if target:
+            actions.user.cursorless_single_target_command_with_arg_list(
+                "insertSnippet",
+                target,
+                [snippet_arg],
+            )
+        else:
+            actions.user.cursorless_implicit_target_command(
+                "insertSnippet",
+                snippet_arg,
+            )
 
     def cursorless_wrap_with_snippet_by_name(
         name: str, variable_name: str, target: dict
-- 
2.39.2

```

</details>

## Checklist

- [x] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [x] 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)
- [x] I have not broken the cheatsheet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AndreasArvidsson AndreasArvidsson

Labels
code quality Improvements to code quality enhancement New feature or request
Projects
None yet
Milestone
Short list
Development

Successfully merging a pull request may close this issue.

Update Talon side for new destination types cursorless-dev/cursorless
2 participants
@pokey @AndreasArvidsson