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

Extend snippet functionality #1707

Closed
AndreasArvidsson opened this issue Jan 27, 2025 · 5 comments · Fixed by #1718
Closed

Extend snippet functionality #1707

AndreasArvidsson opened this issue Jan 27, 2025 · 5 comments · Fixed by #1718
Assignees

Comments

@AndreasArvidsson
Copy link
Collaborator

AndreasArvidsson commented Jan 27, 2025

Once cursorless-dev/cursorless#2747 is merged we have a way of migrating Cursorless snippet to the community snippet format. Having one single easy to use format for all Talon users is a nice improvement, but the Cursorless snippet format did have two features that the community format is missing. My intent is to extend the community snippet format to support these.

The two Cursorless snippet features missing from community snippets are:

  1. Use snippets in a non active editor with a different programming language.
    eg you are in a markdown file and say "snip air after blue bat". blue bat is in another split and in python. The Cursorless snippet implementation would pick the language for the editor you are inserting the snippet into regardless of what the active/focused editor's language is.
  2. Different snippet bodies for the same snippet name/phrase and language depending on the parse tree.
    eg in javascript a function looks like function foo() { }, but a method in a class looks like foo() { }. With the Cursorless snippet format you can just say "snip funk" and Cursorless would figure out which one of the two snippet to use. To do this Cursorless snippets have two extra fields scopeTypes and excludeDescendantScopeTypes to make its match with.

Proposed solution

For both these features we need to send multiple snippets from Talon to Cursorless. Today community does its snippet insertion using the action def insert_snippet(body: str). What we could do is to add an action like def insert_snippet_variants(snippets: list[Snippet]). The default module implementation would just be to pick the first snippet corresponding to the active language and then call insert_snippet . Cursorless on the other hand could context override insert_snippet_variants to send all snippet variants to the Cursorless extension using the command client. After that Cursorless can pick the best snippet. Thanks @phillco for this idea.

We still need to figure out how to either implement or replace scopeTypes and excludeDescendantScopeTypes in the community format.
The reason why we need both of these fields is scopeTypes would specify class so Cursorless knows to use the method snippet body when you're inside a class and excludeDescendantScopeTypes would specify function so that Cursorless knows to NOT use a method snippet when inside a function.

Example snippets

name: functionDeclaration
phrase: funk
---

language: javascript
-
function $1 () {
    $0
}
---

language: javascript
scopeTypes: class
excludeDescendantScopeTypes: function
-
$1 () {
    $0
}

Problems

  1. Ideally we would find a way to express this functions/method behavior using only one field.
  2. How can we handle Cursorless context overriding an action for users with an old community version? What is the best way to check or catch the errors so users don't get a warning in the log about a missing module definition?
@pokey
Copy link
Collaborator

pokey commented Jan 28, 2025

This looks solid to me. And I think we can re-use the code we have floating around in Cursorless for selecting snippet based on context; it has some good tests

Re the problems, for 1) I don't have any better ideas; these two fields were the best I could come up with. For 2), that's a good question. Might be worth asking on slack

@AndreasArvidsson
Copy link
Collaborator Author

AndreasArvidsson commented Jan 28, 2025

Sounds good

Could we use something like matchScope: class | !function or matchScope: class | not function? Basically means that we use a method when we are in a class, but also not in a function. Implementation wise we divide them into a whitelist and a blacklist.

@AndreasArvidsson
Copy link
Collaborator Author

AndreasArvidsson commented Jan 31, 2025

I'm doing some testing

  1. Putting a action on a context when there is no module definition
2025-01-31 11:13:25.818 WARNING actions skipped because they have no matching declaration:
    Context(user.cursorless-talon.src.snippets): user.test_action
  1. Putting a match on the context. No messages in the log if the context isn't active. ie we could conditionally activate a tag that enables the new context actions

Using a on ready callback to check the registry if the action exists. If it does we can activate a new tag that enables the new context and our action overrides. This sounds like a good way forward.

@AndreasArvidsson
Copy link
Collaborator Author

AndreasArvidsson commented Jan 31, 2025

After some more digging through the code I don't think we need to context override any new actions.
There already is an action insert_snippet_by_name(name: str, substitutions: Optional[dict[str, str]] = None) we can use. This is the same action community calls whenever you say "snip if" so seems fitting that Cursorless would just use the same one.

The only thing we need to introduce is a getter actions so that Cursorless Talon can actually get multiple snippets. eg get_snippets(name: str) -> list[Snippet]. Since this is a call to an action we can just wrap that in try catch and fall back to the old implementation if needed. No need for new tags, on ready events or anything like that.

@AndreasArvidsson
Copy link
Collaborator Author

AndreasArvidsson commented Feb 1, 2025

Here is my suggestion
#1718

Here is the Cursorless changes. In essence Cursorless puts its own version off insert_snippet_by_name on a context and then utilizes get_insertion_snippets and get_wrapper_snippets to get multiple snippets. Backwards compatibility is achieved with a try/catch.

cursorless-dev/cursorless#2804

nriley added a commit that referenced this issue Feb 2, 2025
* Added actions to get multiple snippets for the same name. Can be used
by extensions like Cursorless to make better decisions about which
snippet to use.
* Simplified the snippet code and added stronger types
* Only update Talon lists if they have actually changed. 

Fixes #1707
Fixes #1373

---------

Co-authored-by: Nicholas Riley <[email protected]>
Co-authored-by: Jeff Knaus <[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.

2 participants