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

Command client pre-phrase signal #676

Merged
merged 7 commits into from
Dec 6, 2021
Merged

Conversation

pokey
Copy link
Collaborator

@pokey pokey commented Nov 27, 2021

Touches a file known to the VSCode command server before executing any phrase, to allow VSCode extensions to detect start of phrase and maintain consistency over the course of a phrase.

Inaugural use case is allowing cursorless to freeze a snapshot of the hats so that they don't shift over the course of a single command phrase (cursorless-dev/cursorless#318)

@pokey pokey changed the title Command client pre phrase signal Command client pre-phrase signal Nov 27, 2021
@pokey pokey marked this pull request as ready for review November 30, 2021 13:18
@AndreasArvidsson
Copy link
Collaborator

Overall I think it looks good

apps/vscode/command_client/command_client.py Outdated Show resolved Hide resolved
apps/vscode/command_client/command_client.py Outdated Show resolved Hide resolved
@@ -357,6 +358,14 @@ def trigger_command_server_command_execution():
was written to the file. For internal use only"""
actions.key("ctrl-shift-f17")

def emit_pre_phrase_signal():
"""Touches a file to indicate that a phrase is about to begin execution"""
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect that return None would work here to make Talon think this method is implemented. Probably want a comment explaining that we're deliberately creating a no-op action, not an unimplemented action.

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've added a comment to the global scope implementation. I think I'd lean towards keeping that impl; otherwise it feels like we're trying to trick Talon's static analysis

@pokey pokey mentioned this pull request Dec 6, 2021
13 tasks
@pokey pokey requested a review from rntz December 6, 2021 17:04
@pokey pokey merged commit e373780 into master Dec 6, 2021
@pokey pokey deleted the command-client-pre-phrase-signal branch December 6, 2021 17:10
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.

3 participants