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

Hat snapshots #318

Merged
merged 55 commits into from
Dec 6, 2021
Merged

Hat snapshots #318

merged 55 commits into from
Dec 6, 2021

Conversation

pokey
Copy link
Member

@pokey pokey commented Nov 8, 2021

Description

Allows voice engine to emit a signal to indicate start of phrase, which causes the hat token map to be frozen during the course of all commands issued during the given phrase. The ranges will be updated as the document changes, but the same hat names will refer to the same logical document token during the course of the phrase. This feature allows users to issue longer voice commands without worrying about the hats shifting during the course of phrase execution

Related issues

Todo

  • Add transformation that can upgrade old tests
  • Improve documentation for transform script
  • Record video walkthrough of using transform script
  • Add code links to hat snapshots doc (will do after merge)
  • Figure out how to mock prePhrase signal
  • Don't try to touch signal from talon if communication dir doesn't exist
  • Handle case where user has this version of cursorless-talon but old command-client; need to fall back to not using snapshot? Do this talon side or extension side?
  • Check age of snapshot if it is requested. If too old, show error message and don't use snapshot
  • Switch signals to inboundSignals
  • Remove getNamedSignal from command-server API (here and in extension)
  • Fix issue with toggle decorations
  • Figure out why seeing duplicate hats after clone line then undo
  • Figure out promise rejection undefined

@@ -432,15 +432,15 @@
"watch": "tsc -watch -p ./",
"pretest": "yarn run compile && yarn run lint && yarn run esbuild",
"lint": "eslint src --ext ts",
"test": "node ./out/test/runTest.js"
"test": "env CURSORLESS_TEST=true node ./out/test/runTest.js"
Copy link
Member Author

Choose a reason for hiding this comment

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

For consistency with local unit testing, where this variable is set by launch.json


this.recomputeDecorationStyles = this.recomputeDecorationStyles.bind(this);

this.disposables.push(
Copy link
Member Author

Choose a reason for hiding this comment

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

Note how graph components do their own registration now

Copy link
Member

Choose a reason for hiding this comment

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

Looks good really clean things up

thatMark,
sourceMark,
addDecorations,
graph: isTesting() ? graph : undefined,
Copy link
Member Author

Choose a reason for hiding this comment

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

We now return the entire graph object, but only if we're in testing mode

@@ -0,0 +1,49 @@
import { ActionType, PartialTarget } from "../../typings/Types";
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the new command argument type definitions

Comment on lines +127 to +128
spokenFormOrCommandArgument: string | CommandArgument,
...rest: unknown[]
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the ugliness that allows us to handle either old or new command arguments

Copy link
Member

Choose a reason for hiding this comment

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

I think that works fine

@@ -1,8 +1,9 @@
spokenForm: bring air and bat and cap
Copy link
Member Author

Choose a reason for hiding this comment

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

New recorded test syntax just stores an exact snapshot of the command argument that comes in to the cursorless command

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Really inline with our discussion about that the tests should only capture what the user actually said.

inputPartialTargets: PartialTarget[],
inputExtraArgs: any[]
) {
commandArgument: CommandArgument
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we have one argument packaged together, we can just input a command argument and return a new command argument that is latest version

partialTargets: PartialTarget[];
extraArgs: any[];
};
export type TestCaseCommand = CommandArgument;
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice how this one is just a regular command argument now

@pokey pokey marked this pull request as ready for review December 3, 2021 17:41
@AndreasArvidsson
Copy link
Member

I must admit I haven't looked through all the code. But I think the changes to the api and command version looks good.

@pokey pokey merged commit 0b685a7 into main Dec 6, 2021
@pokey pokey deleted the hat-snapshots branch December 6, 2021 18:13
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.

2 participants