Skip to content

Hat snapshots#318

Merged
pokey merged 55 commits intomainfrom
hat-snapshots
Dec 6, 2021
Merged

Hat snapshots#318
pokey merged 55 commits intomainfrom
hat-snapshots

Conversation

@pokey
Copy link
Copy Markdown
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

Comment thread package.json
"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
Copy Markdown
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

Comment thread src/core/Decorations.ts

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

this.disposables.push(
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread src/extension.ts
thatMark,
sourceMark,
addDecorations,
graph: isTesting() ? graph : undefined,
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread src/testUtil/TestCase.ts
partialTargets: PartialTarget[];
extraArgs: any[];
};
export type TestCaseCommand = CommandArgument;
Copy link
Copy Markdown
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

Comment thread src/core/Decorations.ts
@pokey pokey marked this pull request as ready for review December 3, 2021 17:41
@AndreasArvidsson
Copy link
Copy Markdown
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