Skip to content

Commit

Permalink
Fix keyboard broken by #2235 (#2309)
Browse files Browse the repository at this point in the history
This was not caught by our tests because we mock away command server.
Not sure how catch this sort of thing in our tests; it's one of the
hazards of mocking

This PR introduces a more robust approach, where `undefined` means that
we don't know which element is focused, either because there is no
command server, or we have not been called by the command server

- See also cursorless-dev/command-server#25

## Checklist

- [-] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [-] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [-] I have not broken the cheatsheet
  • Loading branch information
pokey authored Apr 28, 2024
1 parent d7dd800 commit cb66508
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 16 deletions.
24 changes: 24 additions & 0 deletions data/fixtures/recorded/fallback/takeToken2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
languageId: plaintext
focusedElementType: textEditor
command:
version: 7
spokenForm: take token
action:
name: setSelection
target:
type: primitive
modifiers:
- type: containingScope
scopeType: {type: token}
usePrePhraseSnapshot: true
initialState:
documentContents: foo
selections:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 0}
marks: {}
finalState:
documentContents: foo
selections:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 3}
1 change: 0 additions & 1 deletion packages/common/src/FakeCommandServerApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export class FakeCommandServerApi implements CommandServerApi {

constructor() {
this.signals = { prePhrase: { getVersion: async () => null } };
this.focusedElementType = "textEditor";
}

async getFocusedElementType(): Promise<FocusedElementType | undefined> {
Expand Down
2 changes: 1 addition & 1 deletion packages/common/src/types/CommandServerApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export interface CommandServerApi {
};
}

export type FocusedElementType = "textEditor" | "terminal";
export type FocusedElementType = "textEditor" | "terminal" | "other";

export interface InboundSignal {
getVersion(): Promise<string | null>;
Expand Down
2 changes: 1 addition & 1 deletion packages/common/src/types/TestCaseFixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ interface TestCaseFixtureBase {
/**
* The type of element that is focused before the command is executed. If undefined default to text editor.
*/
focusedElementType?: FocusedElementType | "other";
focusedElementType?: FocusedElementType;

/**
* A list of marks to check in the case of navigation map test otherwise undefined
Expand Down
7 changes: 3 additions & 4 deletions packages/cursorless-engine/src/core/getCommandFallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ export async function getCommandFallback(
runAction: (actionDescriptor: ActionDescriptor) => Promise<ActionReturnValue>,
command: CommandComplete,
): Promise<Fallback | null> {
if (
commandServerApi == null ||
(await commandServerApi.getFocusedElementType()) === "textEditor"
) {
const focusedElementType = await commandServerApi?.getFocusedElementType();

if (focusedElementType == null || focusedElementType === "textEditor") {
return null;
}

Expand Down
6 changes: 3 additions & 3 deletions packages/cursorless-engine/src/testCaseRecorder/TestCase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@ export class TestCase {
const fixture: EnforceUndefined<TestCaseFixture> = {
languageId: this.languageId,
focusedElementType:
this.focusedElementType !== "textEditor"
? this.focusedElementType ?? "other"
: undefined,
this.focusedElementType === "textEditor"
? undefined
: this.focusedElementType,
postEditorOpenSleepTimeMs: undefined,
postCommandSleepTimeMs: undefined,
command: this.command,
Expand Down
4 changes: 3 additions & 1 deletion packages/cursorless-vscode-e2e/src/endToEndTestSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ export function endToEndTestSetup(suite: Mocha.Suite) {
const title = this.test!.fullTitle();
retryCount = title === previousTestTitle ? retryCount + 1 : 0;
previousTestTitle = title;
({ ide, injectIde } = (await getCursorlessApi()).testHelpers!);
const testHelpers = (await getCursorlessApi()).testHelpers!;
({ ide, injectIde } = testHelpers);
testHelpers.commandServerApi.setFocusedElementType(undefined);
spy = new SpyIDE(ide);
injectIde(spy);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,7 @@ async function runTest(file: string, spyIde: SpyIDE) {
// spyIde.clipboard.writeText(fixture.initialState.clipboard);
}

commandServerApi.setFocusedElementType(
fixture.focusedElementType === "other"
? undefined
: fixture.focusedElementType ?? "textEditor",
);
commandServerApi.setFocusedElementType(fixture.focusedElementType);

// Ensure that the expected hats are present
await hatTokenMap.allocateHats(
Expand Down

0 comments on commit cb66508

Please sign in to comment.