Skip to content

Add text-based surrounding pair implementation #218

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
pokey opened this issue Aug 12, 2021 · 11 comments · Fixed by #324
Closed

Add text-based surrounding pair implementation #218

pokey opened this issue Aug 12, 2021 · 11 comments · Fixed by #324
Labels
enhancement New feature or request
Milestone

Comments

@pokey
Copy link
Member

pokey commented Aug 12, 2021

  • If we are in a lang without syntax tree, use text-based
  • If we are in a string or comment within a language with a syntax tree, start with text-based, but switch to parse tree if no delimiters found within current string / comment
    • Bonus: Treat a sequence of comments or strings as a single comment / string from this perspective
  • Impl for text-based is to just walk left from char, keeping track of what you see and walking over pairs
@pokey pokey added the enhancement New feature or request label Aug 12, 2021
@maciejklimek
Copy link
Contributor

Why do we want to switch to this method for languages with syntax tree?

@pokey
Copy link
Member Author

pokey commented Aug 12, 2021

The main driver for me was being able to select inside brackets within a string, esp eg in talon capture definitions. I could see an alternative where we first run the text-based version if we're within a string or comment, then switch to syntax tree version if we don't find delimiters within the given string / comment. But @AndreasArvidsson may have other thoughts

@pokey
Copy link
Member Author

pokey commented Aug 12, 2021

Fwiw one disadvantage of text-based approach in languages with syntax tree is that it might be tougher to support those pair types where we know it's a pair because of its node type, eg for html tags

@AndreasArvidsson
Copy link
Member

Combining text with parse tree sounds good

@pokey
Copy link
Member Author

pokey commented Aug 12, 2021

ok @maciejklimek your code lives to see another day 😊. I updated issue title / description. Lmk if makes sense to you

@maciejklimek
Copy link
Contributor

I think a good general rule is to have a wide choice of different modifiers available at the backend(vscode side),
provide sensible default at the talon side, but also make it configurable if the user wishes to do a slightly different thing.
In this case, this would mean having a different modifier or and argument to an existing modifier that would implement the new behaviour.
Currently though, I think configuring cursorless on the talon side is not that easy because it requires one to fork the code, or do some weird stuff like sed and the code is changing so fast that forking is not good.

what do you think?

@pokey
Copy link
Member Author

pokey commented Aug 12, 2021

Not sure I totally follow, but would a VSCode extension setting not make sense here? Can you give an example of the type of configuration you're after?

@maciejklimek
Copy link
Contributor

I wasn't sure if the behavior in the second case(the one with the string), would be always preferred.
I imagine that one could prefer to take the syntax tree based matching, instead of the text one in the case with the string.
This is probably a very infrequent case, but it seems to me we could support even this case,
with very low cost.
I think that having this as parameter of the request, is better than having it as the extension setting in vscode,
cause it is more flexible.

But this case is infrequent enough, that i am also fine with the version you proposed :)
I really need this text version in these talon files, I often want to clear the strings with clear maching

@AndreasArvidsson
Copy link
Member

I think we can start with one behavior and if people find that they have problem with it we can always add a setting or an additional word/modifier later. Does that work for every one?

@maciejklimek
Copy link
Contributor

👍

@pokey pokey changed the title Switch surrounding pair implementation to be text-based Add text-based surrounding pair implementation Aug 12, 2021
@pokey
Copy link
Member Author

pokey commented Aug 12, 2021

works for me. fwiw we could potentially get around needing the override behaviour using #124, or by making it so that the scope finder code will expand again if you currently have something of that scope type selected. With the latter you'd just repeat the command to get the bigger scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants