-
Notifications
You must be signed in to change notification settings - Fork 778
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
Generalize file-based RPC command client beyond VSCode #956
Conversation
ctx.matches = r""" | ||
app: vscode | ||
app: visual_studio | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do the following:
- add a new tag called
command_client
, - activate the tag in both visual studio and talon
- use the tag as the
matches
for the context here instead of listing all the apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what I was striving for for but could no make the final leap.
@ctx.action_class("user") | ||
class UserActions: | ||
def emit_pre_phrase_signal() -> bool: | ||
get_signal_path("prePhrase").touch() | ||
|
||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add this class back
def live_pre_phrase_signal() -> bool: | ||
"""Used by application contexts emit_pre_phrase_signal to emit a pre phrase signal""" | ||
get_signal_path("prePhrase").touch() | ||
return True | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
visual_studio_ctx = Context() | ||
|
||
visual_studio_ctx.matches = r""" | ||
app: visual_studio | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can just be called ctx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, originally I had this in command_client and never renamed when I copied in out.
vs_code_ctx = Context() | ||
|
||
vs_code_ctx.matches = r""" | ||
app: vscode | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this as well can just be ctx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -210,7 +199,6 @@ def run_vscode_command( | |||
|
|||
def get_communication_dir_path(): | |||
"""Returns directory that is used by command-server for communication | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
Raises: | ||
Exception: If there is an issue with the file-based communication, or | ||
VSCode raises an exception | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more
|
||
Args: | ||
request (Request): The request to serialize | ||
path (Path): The path to write to | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't look fixed? still deleting lines here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checked should be fixed.
def write_json_exclusive(path: Path, body: Any): | ||
"""Writes jsonified object to file, failing if the file already exists | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
def vscode(command_id: str): | ||
"""Execute command via vscode command server, if available, or fallback | ||
to command palette.""" | ||
actions.user.post_command(command_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this supposed to do? This is causing the following error when I try to run pretty much any command in VSCode (other than Cursorless commands, which don't use this function) 😅:
KeyError: "Action 'user.post_command' is not declared."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed I think it was part of embroyonic attempt to have both new command_server command as vscommands working for backwards compatibility, before I decided that that could wait.
Good stuff! Left a bunch of comments but I think overall this is a nice direction In the future we should probably have an action called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close. As discussed, it would be good to
- rename the actions starting with
vscode_
torun_command_*
- Then add shims in vscode that call these actions
- Open a PR in cursorless that tries the
run_command_
versions, falling back tovscode_
if action not found
Also see below for the idea I had around raising an exception
@@ -153,7 +144,7 @@ def run_vscode_command( | |||
if args or return_command_output: | |||
raise Exception("Must use command-server extension for advanced commands") | |||
print("Communication dir not found; falling back to command palette") | |||
run_vscode_command_by_command_palette(command_id) | |||
actions.user.command_client_fallback(command_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea would be to throw an exception here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
class VsCodeAction: | ||
def command_server_directory() -> string: | ||
return "vscode-command-server" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then define the action vscode
here, and call the run_command
action, wrapped in a try-catch. If it throws an error, do the fallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, this but duplicated code
def command_server_directory() -> string: | ||
return "vscode-command-server" | ||
|
||
def command_client_fallback(command_id: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you can remove this action; just inline into vscode
above
for more information, see https://pre-commit.ci
…hnEffo/knausj_talon into feature/command-client-re-use
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a few changes to this PR, I hope you don't mind. In particular:
-
I removed some changes that you made to the formatters that I assume weren't intended to be part of this PR.
-
I restored some empty lines you deleted.
-
I fixed the type annotation on
*args
inrun_command
and added this to its docstring; and moved the default implementation ofemit_pre_phrase_signal
asreturn False
from the global context to the module where it's defined. This could have been a separate PR but they would conflict, so I thought it would be easier for pokey to review them together.
Looking pretty good! I hope this lands soon, I will probably need this to get cursorless-in-emacs working. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good; left a few more comments
# These commands are shims, to provide backwards compatibility, they may be removed in the fuuture. | ||
# Prefer the run_command... version in command_client. | ||
@mod.action_class | ||
class Actions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to the pre-phrase signal code? Unless I've missed something, it appears to have been dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nvm I see it now. A bit confusing how it's declared in multiple places
What's the purpose of allowing applications to turn it off? Seems simpler to just leave it on and let all applications share the same impl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See command client 368 its in ctx which uses the match belwo to check we are in the correct context. I put a comment in command_client_tag to explain when to overwrite
ctx.matches = r"""
tag: command_client
"""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it from the actions so that it cannot be turned of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, this seems to have been changed into a non-working state. There's no module which declares emit_pre_phrase_signal
, only a context which implements it, which should be producing an error in the talon log. We probably want the default implementation to be return False
. There's no sense touching a file if we're not in a command client. I'll go change this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this should be working now. @pokey is this okay? Did you want us to always touch the pre-phrase file, even if we don't have a command-client app focused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interestingly, it doesn't seem to be causing an error in the talon log. not sure why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. it does error if I actually try to use the action, though. I'll try to minimize that and report it as a talon bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I've reported the underlying bug to talon, talonvoice/talon#525.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…hnEffo/knausj_talon into feature/command-client-re-use
…hnEffo/knausj_talon into feature/command-client-re-use
ok, @pokey, I think this is ready for prime time. I've merged it with my local branch and it works for me. If you're ok with it I think we should merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I did some cleanup, and tested that things are still working for me locally. I left one comment; otherwise good to go!
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
@pokey I think this is just waiting on you to clear your "changes requested" review, until you do that I can't merge it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy, but let's give @JohnEffo a chance to review the change to the server directory, as that will likely break his server implementation until he's done the rename Visual Studio-side whoops missed #956 (comment)
I'd also like to give @lunixbochs a chance to have a look before we merge, as this PR establishes the RPC mechanism that Visual Studio, VSCode, Emacs, and JetBrains will be using until there is an official Talon RPC, so it would be good to make sure we're not doing anything that will make our lives harder when we migrate to official Talon RPC
One other thought: we should probably move this directory in a follow-up PR, as it is still under |
# What is this Split command-client so that other application can reuse the logic for of command server for RPC using existing VS code Command-Client. ## Reasoning In slack advised by aegis, when discussing RPC mechanism for Visual Studio. " if you replicate the vscode file rpc, please try to port the existing code as precisely as possible, without changing the sequence of file reads/writes or anything, because we spent a while making that work well " Initially copied and pasted existing code, wrote the Visual Studio side of things, then thought that best port is re-use of existing code ## Discussion * If this PR goes ahead is Command-Client in the correct location? Probably not, but was not sure were to place it and wanted to float this idea before continuing with anymore work. * Should the methods not be renamed so that they are not so VSCode specific? Yes probably, for the moment I'm happy with the way things work and there is existing code which would need updating (Cursorless) which I did not want to touch as part of this PR. ## Apologies I've installed pre-commit but have not gotten it working correctly as of yet, but wanted to get your opinion on, whether this is this a valid direction of travel. Co-authored-by: johneffo <johneffo@gmailcom.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Michael Arntzenius <daekharel@gmail.com> Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
What is this
Split command-client so that other application can reuse the logic for of command server for RPC using existing VS code Command-Client.
Reasoning
In slack advised by aegis, when discussing RPC mechanism for Visual Studio.
"
if you replicate the vscode file rpc, please try to port the existing code as precisely as possible, without changing the sequence of file reads/writes or anything, because we spent a while making that work well
"
Initially copied and pasted existing code, wrote the Visual Studio side of things, then thought that best port is re-use of existing code
Discussion
Apologies
I've installed pre-commit but have not gotten it working correctly as of yet, but wanted to get your opinion on, whether this is this a valid direction of travel.