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

expand line command actions #1449

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fidgetingbits
Copy link
Collaborator

@fidgetingbits fidgetingbits commented Jun 2, 2024

This adds two new actions for lines that allow for something like f<key> in vim (and other places) which will just jump to the first occurrence of that character.

The only debatable bit is likely the use of find as the command. Open to suggestions though.

@jaresty
Copy link
Contributor

jaresty commented Jun 2, 2024

I dig it. I'd be interested in your feedback on this one too: #1424 (similar to vim's gm). An alternative word that could work maybe would be char (last char). Or go char and go last char.

@fidgetingbits
Copy link
Collaborator Author

I dig it. I'd be interested in your feedback on this one too: #1424 (similar to vim's gm). An alternative word that could work maybe would be char (last char). Or go char and go last char.

Ya, I'm a bit iffy on using find here, just happens to be what I had used in vim. I'd be fine with go char and go last char I think.

Copy link
Contributor

@jaresty jaresty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@jaresty
Copy link
Contributor

jaresty commented Jun 14, 2024

I wanted to share an idea that I just had about this: it would be really cool if we could add a similar command for adding a cursor at the next glyph (meaning: leave a cursor where you are now and then add another one at the next instance of this glyph)

@jaresty
Copy link
Contributor

jaresty commented Jun 14, 2024

I wish I could merge this for you 😄

@fidgetingbits
Copy link
Collaborator Author

fidgetingbits commented Jun 15, 2024

I wanted to share an idea that I just had about this: it would be really cool if we could add a similar command for adding a cursor at the next glyph (meaning: leave a cursor where you are now and then add another one at the next instance of this glyph)

can file an issue or open a separate PR for it. you'd want that to only add a cursor to the next glyph on the same line or next in the file?

makes me wonder if there should be a file for 'multiple cursor' related commands. I don't ever use them aside from with cursorless, but I suspect lots of others do. would need some common command to indicate you want to add a cursor too.

"curse" would be good but curse on/off is used for mouse cursor, so a bit confusing. but something like "curse add next char this"?

You could just do what you want in cursorless already with something like (untested) "pre char this and next view char this", but it's a bit wordy.
edit: Also I use view instead of instance.

anyway probably better to discuss in a different issue or PR if you prefer not to use cursorless for that.

@jaresty
Copy link
Contributor

jaresty commented Jun 15, 2024

👍fwiw you can also use cursorless to navigate to the next glyph. I use multi cursor most often when I don't have access to cursorless because I'm having to work in IntelliJ.

@AndreasArvidsson
Copy link
Collaborator

We discussed this in the latest community session.

  • We think the action name and description should probably reflect that this is not a normal find/search command and is more like a cursor jump. Something like jump cursor to next instance of character.
  • We probably should have an implementation for vim. If we add an implementation of these actions to vimscript.py

@fidgetingbits
Copy link
Collaborator Author

We discussed this in the latest community session.

* We think the action name and description should probably reflect that this is not a normal find/search command and is more like a cursor jump. Something like jump cursor to next instance of character.

Changed the names and doc strings to reflect the feedback.

* We probably should have an implementation for vim. If we add an implementation of these actions to `vimscript.py`

vimscript.py is for the vimscript language, not for the vim app, so not applicable there. community doesn't have any native support for vim atm. Adding it is non-trivial because of mode switching, so it's not as simple as just enabling it if vim is active somewhere.

Most people using vim (afaik) used to use my personal repo, which then got broken out into talon-vim, and then partially got broken into a much simpler implementation neovim-talon, which is what will be paired with cursorless in neovim.

The neovim-talon repo will use these actions being added in this PR, so I don't think there needs to be something in community using them right away. I suspect there is probably some emacs equivalent that could be added if you think there really should be something. Or we could add something for https://marketplace.visualstudio.com/items?itemName=ArturoDent.jump-and-select, but I doubt many people are using that in vscode (could be wrong).

@fidgetingbits
Copy link
Collaborator Author

I did notice that zsh by default seems to bind a readline shortcut for doing this, but bash doesn't. So will need shell-specific readline overrides, which we don't support atm. I plan to add this to my branch and then port it over eventually after a bunch of testing, but not sure if it needs to be part of this PR.

I don't mind to wait though if it's preferred to have some real use cases in community.

I guess maybe of note, I have a vscode plugin scope in my branch so can enable certain commands if a vscode plugin is installed, which could be used to enable jump-and-select extension mentioned above to more cleanly add support for this into vscode as well. I haven't opened a PR to add that to community yet though.

@AndreasArvidsson
Copy link
Collaborator

Update from the community session. We think that we should probably wait with this until we actually have an implementation in community that utilizes the spoken forms.

@fidgetingbits
Copy link
Collaborator Author

No worries. I'll add something that uses it as part of this PR then.

@fidgetingbits
Copy link
Collaborator Author

Ok the scope of this is expanding now... but this implements a plugins tag and associated scope so that you can now specify if an app has a specific plugin installed. I also include action implementation for the new line command if the jump-and-select plugin is installed in vscode.

I haven't bothered moving all of the other plugin usage from vscode.talon to use this new user.plugin_installed scope, as I think it can be a follow up. But can add it here if preferred.

Also atm the test if the plugin is installed is tested on Linux only. Will test on Mac later, but if someone on Windows can hopefully chime in about where the settings file is located I'll add that too.

@knausj85
Copy link
Member

knausj85 commented Dec 7, 2024

Notes from community session:
I really like this plugin idea; I'll be putting it thru some paces over the next week or so to see if we can't get this PR moving again.

@knausj85 knausj85 self-requested a review December 7, 2024 23:19
@nriley nriley marked this pull request as draft December 14, 2024 17:25
Copy link
Member

@knausj85 knausj85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works very well, and I think we should bring it in. Few minor details to sort through.

There were some concerns raised vis-a-vis parsing the extension file (performance?), but this approach makes sense to me.

if not extensions.exists():
return []
with extensions.open() as f:
return [x["identifier"]["id"] for x in json.loads(f.read())]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return [x["identifier"]["id"] for x in json.loads(f.read())]
return [x["identifier"]["id"].lower() for x in json.loads(f.read())]

Just in case there are case differences, perhaps we should standardize on lower case?

@@ -74,3 +74,6 @@ select camel left: user.extend_camel_left()
select camel right: user.extend_camel_right()
go camel left: user.camel_left()
go camel right: user.camel_right()

go char <user.unmodified_key>: user.jump_cursor_to_next_char(unmodified_key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure these should live in line commands. Maybe they make sense alongside the plugin and be enabled with the appropriate tag? As it is, this would add some commands that won't work in a lot of applications

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm. I do think there is the potential for this to be used elsewhere in community, though not sure of any good example where to add it off the top of my head. But for instance, I already used it in my repo for zsh line editor (would like eventually to PR that), and was going to adapt vim to use it after (though that won't become part of community) and similarly I guess people could add it for emacs (maybe there is no emacs in community). I added a separate vscode example in this PR to community specifically to justify it since we didn't want an otherwise completely dead command, but I do see your point..

Having it on the plugin side only doesn't entirely make sense due to the potential to be used inside community, but if you guys decide you don't want it in here and just want to lift the plugin stuff from this PR instead I'm good with that. Can always add it later once there is a second use case.



ui.register("win_focus", scope.update)
ui.register("win_title", scope.update)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need win_title to update the scope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm, I think probably not. I don't entirely remember why I added that. This code was simplified from some vim plugin parsing I had written awhile back though, so it may have been required for vim, but I can't see why it would be for vscode. Will remove.

apps/vscode/plugins/jump-and-select.py Show resolved Hide resolved

def vscode_update_plugins_list():
"""Queries the list of vscode plugins"""
extensions = pathlib.Path.home() / ".vscode/extensions/extensions.json"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will not work for all the different clones of vscode. If you look at our application definitions we support applications like VSCodium and Cursor. Anything we add in the vscode file should ideally work on all of these.


@mod.scope
def scope():
return {"plugin_installed": actions.user.plugins_list()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're thinking actions.user.plugins_list should have a VScode exclusive implementation for now. For other variants, these could be added by folks.

Long term, we're thinking to explore getting this via the command server.

Copy link
Collaborator

@AndreasArvidsson AndreasArvidsson Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked into this and there isn't a simple way from the command server to know this folder path, but we can return a list of all the extensions from the command server.

Comment on lines +46 to +47
def jump_cursor_to_prev_char(char: str):
"""Jumps the first to the previous instance of the specified character on the current line"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: could you elaborate why limiting search results to the current line is valuable? I thought it was because it would wrap so you could get to stuff closer to the beginning quickly, but it looks like in vim f doesn't do that. Probably something we were missing. cc @nriley who tried it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I often fuzzily (as in I don't know the exact index so overshoot) jump to the last character on a line (and don't want to jump off the line) and also don't always want to have to speak multiple commands to go to the end of the line and then search backwards. Also helps if you are working on a single line and definitely don't want to go off that line in the event of a misrecognition/mispeak (a regularity for me).

If others don't think that's actually useful to general users happy to just close.

@knausj85
Copy link
Member

knausj85 commented Feb 16, 2025

Alright, I wanna try to get this one moving again.

My take
I like both the plugin functionality and the example go char. go char's proven useful to me; so as long as no other maintainers feel super strongly, I vote we keep it as-is.

My proposal:

  • scope the plugin list action implementation to Microsoft VSCode proper for now. Other vscode clones can be added similarly as desired by other community members.

  • add a comment that the go char commands likely require a plugin to the .talon file

  • remove the scope update on win title if we can.

  • merge and move on 🙂

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.

5 participants