Skip to content

Implementing jumping to location when clicking on a usage in the details panel #2679

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.

Sign up for GitHub

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

Merged
robertbrignull merged 5 commits into from
Aug 9, 2023
Merged

Implementing jumping to location when clicking on a usage in the details panel #2679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f4d6990
Move duplicated error handling into showResolvableLocation
robertbrignull Aug 8, 2023
9466070
Add command to jump to usage location
robertbrignull Aug 8, 2023
16d5f0f
Add DatabaseItem to model details panel state
robertbrignull Aug 8, 2023
7443b35
Jump to location when clicking on usage
robertbrignull Aug 8, 2023
1139790
Rename command from codeQLDataExtensions => codeQLDataExtensionsEditor
robertbrignull Aug 9, 2023
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 extensions/ql-vscode/src/common/commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
} from "../variant-analysis/shared/variant-analysis";
import type { QLDebugConfiguration } from "../debugger/debug-configuration";
import type { QueryTreeViewItem } from "../queries-panel/query-tree-view-item";
import type { Usage } from "../data-extensions-editor/external-api-usage";

// A command function matching the signature that VS Code calls when
// a command is invoked from a context menu on a TreeView with
Expand Down Expand Up @@ -304,6 +305,10 @@ export type PackagingCommands = {

export type DataExtensionsEditorCommands = {
"codeQL.openDataExtensionsEditor": () => Promise<void>;
"codeQLDataExtensionsEditor.jumpToUsageLocation": (
usage: Usage,
databaseItem: DatabaseItem,
) => Promise<void>;
};

export type EvalLogViewerCommands = {
Expand Down
12 changes: 10 additions & 2 deletions extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { DataExtensionsEditorView } from "./data-extensions-editor-view";
import { DataExtensionsEditorCommands } from "../common/commands";
import { CliVersionConstraint, CodeQLCliServer } from "../codeql-cli/cli";
import { QueryRunner } from "../query-server";
import { DatabaseManager } from "../databases/local-databases";
import { DatabaseItem, DatabaseManager } from "../databases/local-databases";
import { ensureDir } from "fs-extra";
import { join } from "path";
import { App } from "../common/app";
Expand All @@ -23,6 +23,8 @@ import { setUpPack } from "./external-api-usage-query";
import { DisposableObject } from "../common/disposable-object";
import { ModelDetailsPanel } from "./model-details/model-details-panel";
import { Mode } from "./shared/mode";
import { showResolvableLocation } from "../databases/local-databases/locations";
import { Usage } from "./external-api-usage";

const SUPPORTED_LANGUAGES: string[] = ["java", "csharp"];

Expand Down Expand Up @@ -145,7 +147,7 @@ export class DataExtensionsEditorModule extends DisposableObject {
db,
modelFile,
Mode.Application,
(usages) => this.modelDetailsPanel.setExternalApiUsages(usages),
this.modelDetailsPanel.setState.bind(this.modelDetailsPanel),
);
await view.openView();
},
Expand All @@ -154,6 +156,12 @@ export class DataExtensionsEditorModule extends DisposableObject {
},
);
},
"codeQLDataExtensionsEditor.jumpToUsageLocation": async (
usage: Usage,
databaseItem: DatabaseItem,
) => {
await showResolvableLocation(usage.url, databaseItem, this.app.logger);
},
};
}

Expand Down
26 changes: 8 additions & 18 deletions extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ export class DataExtensionsEditorView extends AbstractWebview<
private readonly databaseItem: DatabaseItem,
private readonly extensionPack: ExtensionPack,
private mode: Mode,
private readonly onExternalApiUsagesChanged: (
private readonly updateModelDetailsPanelState: (
externalApiUsages: ExternalApiUsage[],
databaseItem: DatabaseItem,
) => void,
) {
super(ctx);
Expand Down Expand Up @@ -230,21 +231,7 @@ export class DataExtensionsEditorView extends AbstractWebview<
protected async jumpToUsage(
location: ResolvableLocationValue,
): Promise<void> {
try {
await showResolvableLocation(location, this.databaseItem);
} catch (e) {
if (e instanceof Error) {
if (e.message.match(/File not found/)) {
void window.showErrorMessage(
"Original file of this result is not in the database's source archive.",
);
} else {
void this.app.logger.log(`Unable to handleMsgFromView: ${e.message}`);
}
} else {
void this.app.logger.log(`Unable to handleMsgFromView: ${e}`);
}
}
await showResolvableLocation(location, this.databaseItem, this.app.logger);
}

protected async loadExistingModeledMethods(): Promise<void> {
Expand Down Expand Up @@ -310,7 +297,10 @@ export class DataExtensionsEditorView extends AbstractWebview<
t: "setExternalApiUsages",
externalApiUsages,
});
this.onExternalApiUsagesChanged(externalApiUsages);
this.updateModelDetailsPanelState(
externalApiUsages,
this.databaseItem,
);
} catch (err) {
void showAndLogExceptionWithTelemetry(
this.app.logger,
Expand Down Expand Up @@ -592,7 +582,7 @@ export class DataExtensionsEditorView extends AbstractWebview<
addedDatabase,
modelFile,
Mode.Framework,
this.onExternalApiUsagesChanged,
this.updateModelDetailsPanelState,
);
await view.openView();
});
Expand Down
13 changes: 12 additions & 1 deletion extensions/ql-vscode/src/data-extensions-editor/model-details/model-details-data-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import {
} from "vscode";
import { DisposableObject } from "../../common/disposable-object";
import { ExternalApiUsage, Usage } from "../external-api-usage";
import { DatabaseItem } from "../../databases/local-databases";

export class ModelDetailsDataProvider
extends DisposableObject
implements TreeDataProvider<ModelDetailsTreeViewItem>
{
private externalApiUsages: ExternalApiUsage[] = [];
private databaseItem: DatabaseItem | undefined = undefined;

private readonly onDidChangeTreeDataEmitter = this.push(
new EventEmitter<void>(),
Expand All @@ -22,8 +24,12 @@ export class ModelDetailsDataProvider
return this.onDidChangeTreeDataEmitter.event;
}

public setExternalApiUsages(externalApiUsages: ExternalApiUsage[]): void {
public setState(
externalApiUsages: ExternalApiUsage[],
databaseItem: DatabaseItem,
): void {
this.externalApiUsages = externalApiUsages;
this.databaseItem = databaseItem;
this.onDidChangeTreeDataEmitter.fire();
}

Expand All @@ -37,6 +43,11 @@ export class ModelDetailsDataProvider
return {
label: item.label,
collapsibleState: TreeItemCollapsibleState.None,
command: {
title: "Show usage",
command: "codeQLDataExtensionsEditor.jumpToUsageLocation",
arguments: [item, this.databaseItem],
},
};
}
}
Expand Down
8 changes: 6 additions & 2 deletions extensions/ql-vscode/src/data-extensions-editor/model-details/model-details-panel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { window } from "vscode";
import { DisposableObject } from "../../common/disposable-object";
import { ModelDetailsDataProvider } from "./model-details-data-provider";
import { ExternalApiUsage } from "../external-api-usage";
import { DatabaseItem } from "../../databases/local-databases";

export class ModelDetailsPanel extends DisposableObject {
private readonly dataProvider: ModelDetailsDataProvider;
Expand All @@ -17,7 +18,10 @@ export class ModelDetailsPanel extends DisposableObject {
this.push(treeView);
}

public setExternalApiUsages(externalApiUsages: ExternalApiUsage[]): void {
this.dataProvider.setExternalApiUsages(externalApiUsages);
public setState(
externalApiUsages: ExternalApiUsage[],
databaseItem: DatabaseItem,
): void {
this.dataProvider.setState(externalApiUsages, databaseItem);
Copy link
Copy Markdown
Contributor

@charisk charisk Aug 9, 2023

Choose a reason for hiding this comment

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

This surprised me a little bit. I've generally thought of the "DataProvider" classes as the ones managing state and expected the data extensions editor view to use the provider rather than the panel. Having said that, I can't think of a problem with what you've done here 🤔

Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Aug 9, 2023

Choose a reason for hiding this comment

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

I guess it depends whether you think of the data provider as the public interface to the panel or as an internal detail. Personally I think of them as more internal, but I'm also happy with either option.

Currently the data provider is constructed and owned by the panel. We could have it be constructed outside and passed into the panel. Then the panel is literally nothing more than a single call to window.createTreeView.

Currently the panel is constructed from DataExtensionsEditorModule and there isn't a separate "module" just for the detail panel. So in this case I think it's better to have the data provider be constructed inside ModelDetailsPanel as it keeps things slightly contained without introducing another "module" class which would itself be tiny.

Copy link
Copy Markdown
Contributor

@charisk charisk Aug 9, 2023

Choose a reason for hiding this comment

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

Cool! Lets go with that and see if we can identify any pros/cons about either approach. In general it feels like it would be good to standardise a bit to make the codebase a bit easier.

}
}
25 changes: 13 additions & 12 deletions extensions/ql-vscode/src/databases/local-databases/locations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,19 @@ export function tryResolveLocation(
export async function showResolvableLocation(
loc: ResolvableLocationValue,
databaseItem: DatabaseItem,
logger: Logger,
): Promise<void> {
await showLocation(tryResolveLocation(loc, databaseItem));
try {
await showLocation(tryResolveLocation(loc, databaseItem));
} catch (e) {
if (e instanceof Error && e.message.match(/File not found/)) {
void Window.showErrorMessage(
"Original file of this result is not in the database's source archive.",
);
} else {
void logger.log(`Unable to jump to location: ${getErrorMessage(e)}`);
}
}
}

export async function showLocation(location?: Location) {
Expand Down Expand Up @@ -146,16 +157,6 @@ export async function jumpToLocation(
) {
const databaseItem = databaseManager.findDatabaseItem(Uri.parse(databaseUri));
if (databaseItem !== undefined) {
try {
await showResolvableLocation(loc, databaseItem);
} catch (e) {
if (e instanceof Error && e.message.match(/File not found/)) {
void Window.showErrorMessage(
"Original file of this result is not in the database's source archive.",
);
} else {
void logger.log(`Unable to jump to location: ${getErrorMessage(e)}`);
}
}
await showResolvableLocation(loc, databaseItem, logger);
}
}
3 changes: 2 additions & 1 deletion extensions/ql-vscode/test/unit-tests/command-lint.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ describe("commands declared in package.json", () => {
command.match(/^codeQLQueryHistory\./) ||
command.match(/^codeQLAstViewer\./) ||
command.match(/^codeQLEvalLogViewer\./) ||
command.match(/^codeQLTests\./)
command.match(/^codeQLTests\./) ||
command.match(/^codeQLDataExtensionsEditor\./)
) {
scopedCmds.add(command);
expect(title).toBeDefined();
Expand Down