Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generalize file-based RPC command client beyond VSCode #956
Changes from all commits
423a261
df2e523
a345741
9304e22
09ef97e
613281d
2f5c726
dbb6893
211802a
a7c1c52
01bc65d
8d57e34
ce14610
4c9c482
27aa1f3
640af19
c2fbf32
295f843
c41f842
7717e82
340b707
2d83c9d
4badb6c
eb5254b
00343b4
34ae8e9
9fdac7e
8cb4444
0522403
0f87908
40d8ee0
4d7609f
054e81f
c5eb3ab
a8a1409
5b0d283
347c9b3
571abce
b9719f5
13e7d70
39478d5
f831999
033eae5
29afd9a
173ebef
d7e89b1
3d1e016
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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 bereturn 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.