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

Only show debug log in development mode #380

Merged
merged 19 commits into from
Dec 16, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@
"default": true,
"description": "Whether to show decorations on vscode start."
},
"cursorless.debug": {
"type": "boolean",
"default": false,
"description": "Whether to show debug logs."
},
"cursorless.pendingEditDecorationTime": {
"type": "integer",
"default": 100,
Expand Down
128 changes: 128 additions & 0 deletions src/core/Debug.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
import {
Disposable,
ExtensionMode,
Location,
TextEditorSelectionChangeEvent,
window,
workspace,
} from "vscode";
import { SyntaxNode, TreeCursor } from "web-tree-sitter";
import { Graph } from "../typings/Types";

export default class Debug {
private disposableConfiguration?: Disposable;
private disposableSelection?: Disposable;
active: boolean;

constructor(private graph: Graph) {
this.graph.extensionContext.subscriptions.push(this);

this.evaluateSetting = this.evaluateSetting.bind(this);
this.logBranchTypes = this.logBranchTypes.bind(this);
this.active = true;

switch (this.graph.extensionContext.extensionMode) {
// Development mode. Always enable.
case ExtensionMode.Development:
this.enableDebugLog();
break;
// Test mode. Always disable.
case ExtensionMode.Test:
this.disableDebugLog();
break;
// Production mode. Enable based on user setting.
case ExtensionMode.Production:
this.evaluateSetting();
this.disposableConfiguration = workspace.onDidChangeConfiguration(
this.evaluateSetting
);
break;
}
}

init() {}

log(...args: any[]) {
if (this.active) {
console.debug(...args);
}
}

dispose() {
if (this.disposableConfiguration) {
this.disposableConfiguration.dispose();
}
if (this.disposableSelection) {
this.disposableSelection.dispose();
}
}

private enableDebugLog() {
this.active = true;
this.disposableSelection = window.onDidChangeTextEditorSelection(
this.logBranchTypes
);
}

private disableDebugLog() {
this.active = false;
if (this.disposableSelection) {
this.disposableSelection.dispose();
Copy link
Member

Choose a reason for hiding this comment

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

do we need to set this to null? or is it idempotent?

Copy link
Member Author

Choose a reason for hiding this comment

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

You shouldn't be able to end up in disable twice without having enabled in between. I tried toggling it back and forward without any problem.

Copy link
Member

Choose a reason for hiding this comment

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

no but the issue is that if the extension gets deactivated it will get called twice. probably not a big deal, but just being fastidious

Copy link
Member

Choose a reason for hiding this comment

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

eg if you enable log, then disable it, then deactivate extension

}
}

private evaluateSetting() {
const debugEnabled = workspace
.getConfiguration("cursorless")
.get<boolean>("debug")!;
if (debugEnabled) {
this.enableDebugLog();
} else {
this.disableDebugLog();
}
}

private logBranchTypes(event: TextEditorSelectionChangeEvent) {
const location = new Location(
window.activeTextEditor!.document.uri,
event.selections[0]
);

let node: SyntaxNode;
try {
node = this.graph.getNodeAtLocation(location);
} catch (error) {
return;
}

const ancestors: SyntaxNode[] = [node];
while (node.parent != null) {
ancestors.unshift(node.parent);
node = node.parent;
}

const cursor = node.tree.walk();
this.printCursorLocationInfo(cursor, 0);

for (let i = 1; i < ancestors.length; ++i) {
cursor.gotoFirstChild();
while (cursor.currentNode().id !== ancestors[i].id) {
if (!cursor.gotoNextSibling()) {
return;
}
}
this.printCursorLocationInfo(cursor, i);
}

const leafText = ancestors[ancestors.length - 1].text
.replace(/\s+/g, " ")
.substring(0, 100);
console.debug(">".repeat(ancestors.length), `"${leafText}"`);
}

private printCursorLocationInfo(cursor: TreeCursor, depth: number) {
const field = cursor.currentFieldName();
const fieldText = field != null ? `${field}: ` : "";
console.debug(">".repeat(depth + 1), `${fieldText}${cursor.nodeType}`);
}
}
2 changes: 1 addition & 1 deletion src/core/HatTokenMap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export default class HatTokenMap {
const newSignalVersion = await phraseStartSignal.getVersion();

if (newSignalVersion !== this.lastSignalVersion) {
console.debug("taking snapshot");
this.graph.debug.log("taking snapshot");
this.lastSignalVersion = newSignalVersion;

if (newSignalVersion != null) {
Expand Down
14 changes: 7 additions & 7 deletions src/core/commandRunner/CommandRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
import { ThatMark } from "../ThatMark";
import { TestCaseRecorder } from "../../testUtil/TestCaseRecorder";
import { canonicalizeAndValidateCommand } from "../../util/canonicalizeAndValidateCommand";
import { SyntaxNode } from "web-tree-sitter";
import { CommandArgument } from "./types";
import { isString } from "../../util/type";

Expand All @@ -22,7 +21,6 @@ export default class CommandRunner {
private graph: Graph,
private thatMark: ThatMark,
private sourceMark: ThatMark,
private getNodeAtLocation: (location: vscode.Location) => SyntaxNode,
private testCaseRecorder: TestCaseRecorder
) {
graph.extensionContext.subscriptions.push(this);
Expand All @@ -40,8 +38,10 @@ export default class CommandRunner {

async runCommand(commandArgument: CommandArgument) {
try {
console.debug(`commandArgument:`);
console.debug(JSON.stringify(commandArgument, null, 3));
if (this.graph.debug.active) {
this.graph.debug.log(`commandArgument:`);
this.graph.debug.log(JSON.stringify(commandArgument, null, 3));
}

const {
spokenForm,
Expand Down Expand Up @@ -76,7 +76,7 @@ export default class CommandRunner {
hatTokenMap: readableHatMap,
thatMark: this.thatMark.exists() ? this.thatMark.get() : [],
sourceMark: this.sourceMark.exists() ? this.sourceMark.get() : [],
getNodeAtLocation: this.getNodeAtLocation,
getNodeAtLocation: this.graph.getNodeAtLocation,
};

const selections = processTargets(processedTargetsContext, targets);
Expand Down Expand Up @@ -110,8 +110,8 @@ export default class CommandRunner {
this.testCaseRecorder.commandErrorHook();
const err = e as Error;
vscode.window.showErrorMessage(err.message);
console.debug(err.message);
console.debug(err.stack);
this.graph.debug.log(err.message);
this.graph.debug.log(err.stack);
Copy link
Member

Choose a reason for hiding this comment

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

I'm tempted to make these just console.error, because that way we can catch the stack trace the first time an error occurs. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was also thinking that.

throw err;
}
}
Expand Down
11 changes: 3 additions & 8 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import * as vscode from "vscode";
import graphFactories from "./util/graphFactories";
import { Graph } from "./typings/Types";
import makeGraph, { FactoryMap } from "./util/makeGraph";
import { logBranchTypes } from "./util/debug";
import { ThatMark } from "./core/ThatMark";
import { TestCaseRecorder } from "./testUtil/TestCaseRecorder";
import { getCommandServerApi, getParseTreeApi } from "./util/getExtensionApi";
Expand All @@ -17,7 +16,9 @@ export async function activate(context: vscode.ExtensionContext) {
...graphFactories,
extensionContext: () => context,
commandServerApi: () => commandServerApi,
getNodeAtLocation: () => getNodeAtLocation,
} as FactoryMap<Graph>);
graph.debug.init();
graph.snippets.init();
await graph.decorations.init();
graph.hatTokenMap.init();
Expand Down Expand Up @@ -47,7 +48,6 @@ export async function activate(context: vscode.ExtensionContext) {
graph,
thatMark,
sourceMark,
getNodeAtLocation,
testCaseRecorder
);

Expand Down Expand Up @@ -95,12 +95,7 @@ export async function activate(context: vscode.ExtensionContext) {
}
}

context.subscriptions.push(
cursorlessRecordTestCaseDisposable,
vscode.window.onDidChangeTextEditorSelection(
logBranchTypes(getNodeAtLocation)
)
);
context.subscriptions.push(cursorlessRecordTestCaseDisposable);
AndreasArvidsson marked this conversation as resolved.
Show resolved Hide resolved

return {
thatMark,
Expand Down
4 changes: 0 additions & 4 deletions src/test/suite/recorded.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,9 @@ import {
SerializedMarks,
} from "../../testUtil/toPlainObject";
import { getCursorlessApi } from "../../util/getExtensionApi";
import { enableDebugLog } from "../../util/debug";
import { extractTargetedMarks } from "../../testUtil/extractTargetedMarks";
import asyncSafety from "./asyncSafety";
import { ReadOnlyHatMap } from "../../core/IndividualHatMap";
import { mockPrePhraseGetVersion } from "../mockPrePhraseGetVersion";
import { openNewEditor } from "../openNewEditor";
import getRecordedTestPaths from "./getRecordedTestPaths";

Expand All @@ -39,8 +37,6 @@ suite("recorded test cases", async function () {
this.timeout("100s");
this.retries(5);

enableDebugLog(false);
AndreasArvidsson marked this conversation as resolved.
Show resolved Hide resolved

teardown(() => {
sinon.restore();
});
Expand Down
11 changes: 11 additions & 0 deletions src/typings/Types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Decorations from "../core/Decorations";
import FontMeasurements from "../core/FontMeasurements";
import { CommandServerApi } from "../util/getExtensionApi";
import { ReadOnlyHatMap } from "../core/IndividualHatMap";
import Debug from "../core/Debug";

/**
* A token within a text editor, including the current display line of the token
Expand Down Expand Up @@ -445,6 +446,16 @@ export interface Graph {
* API object for interacting with the command server, if it exists
*/
readonly commandServerApi: CommandServerApi | null;

/**
* Function to access nodes in the tree sitter.
*/
readonly getNodeAtLocation: (location: vscode.Location) => SyntaxNode;

/**
* Debug logger
*/
readonly debug: Debug;
}

export type NodeMatcherValue = {
Expand Down
53 changes: 0 additions & 53 deletions src/util/debug.ts

This file was deleted.

2 changes: 2 additions & 0 deletions src/util/graphFactories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Snippets } from "../core/Snippets";
import { RangeUpdater } from "../core/updateSelections/RangeUpdater";
import Decorations from "../core/Decorations";
import FontMeasurements from "../core/FontMeasurements";
import Debug from "../core/Debug";

type ConstructorMap<T> = {
[P in keyof T]: new (t: T) => T[P];
Expand All @@ -20,6 +21,7 @@ const graphConstructors: Partial<ConstructorMap<Graph>> = {
fontMeasurements: FontMeasurements,
snippets: Snippets,
rangeUpdater: RangeUpdater,
debug: Debug,
};

const graphFactories: Partial<FactoryMap<Graph>> = Object.fromEntries(
Expand Down