Skip to content

Update the format of selected state and propagate it to the treeview #1817

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
charisk merged 3 commits into from
Dec 1, 2022
Merged

Update the format of selected state and propagate it to the treeview #1817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
943c547
Update selected state format.
charisk Dec 1, 2022
7b54b6c
Propagate the selected state from the db config to the tree view.
charisk Dec 1, 2022
db16005
Merge branch 'main' into charisk/selected-db-state
charisk Dec 1, 2022
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
91 changes: 82 additions & 9 deletions extensions/ql-vscode/src/databases/config/db-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,53 @@ export interface DbConfigDatabases {
local: LocalDbConfig;
}

export interface SelectedDbItem {
kind: SelectedDbItemKind;
value: string;
}
export type SelectedDbItem =
| SelectedLocalUserDefinedList
| SelectedLocalDatabase
| SelectedRemoteSystemDefinedList
| SelectedRemoteUserDefinedList
| SelectedRemoteOwner
| SelectedRemoteRepository;

export enum SelectedDbItemKind {
ConfigDefined = "configDefined",
LocalUserDefinedList = "localUserDefinedList",
LocalDatabase = "localDatabase",
RemoteSystemDefinedList = "remoteSystemDefinedList",
RemoteUserDefinedList = "remoteUserDefinedList",
RemoteOwner = "remoteOwner",
RemoteRepository = "remoteRepository",
}

export interface SelectedLocalUserDefinedList {
kind: SelectedDbItemKind.LocalUserDefinedList;
listName: string;
}

export interface SelectedLocalDatabase {
kind: SelectedDbItemKind.LocalDatabase;
databaseName: string;
listName?: string;
Comment thread
norascheuch marked this conversation as resolved.
}

export interface SelectedRemoteSystemDefinedList {
kind: SelectedDbItemKind.RemoteSystemDefinedList;
listName: string;
}

export interface SelectedRemoteUserDefinedList {
kind: SelectedDbItemKind.RemoteUserDefinedList;
listName: string;
}

export interface SelectedRemoteOwner {
kind: SelectedDbItemKind.RemoteOwner;
ownerName: string;
}

export interface SelectedRemoteRepository {
kind: SelectedDbItemKind.RemoteRepository;
repositoryName: string;
listName?: string;
Comment thread
norascheuch marked this conversation as resolved.
}

export interface RemoteDbConfig {
Expand Down Expand Up @@ -70,10 +109,44 @@ export function cloneDbConfig(config: DbConfig): DbConfig {
},
},
selected: config.selected
? {
kind: config.selected.kind,
value: config.selected.value,
}
? cloneDbConfigSelectedItem(config.selected)
: undefined,
};
}

function cloneDbConfigSelectedItem(selected: SelectedDbItem): SelectedDbItem {
switch (selected.kind) {
case SelectedDbItemKind.LocalUserDefinedList:
return {
kind: SelectedDbItemKind.LocalUserDefinedList,
listName: selected.listName,
};
case SelectedDbItemKind.LocalDatabase:
return {
kind: SelectedDbItemKind.LocalDatabase,
databaseName: selected.databaseName,
listName: selected.listName,
};
case SelectedDbItemKind.RemoteSystemDefinedList:
return {
kind: SelectedDbItemKind.RemoteSystemDefinedList,
listName: selected.listName,
};
case SelectedDbItemKind.RemoteUserDefinedList:
return {
kind: SelectedDbItemKind.RemoteUserDefinedList,
listName: selected.listName,
};
case SelectedDbItemKind.RemoteOwner:
return {
kind: SelectedDbItemKind.RemoteOwner,
ownerName: selected.ownerName,
};
case SelectedDbItemKind.RemoteRepository:
return {
kind: SelectedDbItemKind.RemoteRepository,
repositoryName: selected.repositoryName,
listName: selected.listName,
Comment thread
norascheuch marked this conversation as resolved.
};
}
}
53 changes: 53 additions & 0 deletions extensions/ql-vscode/src/databases/db-item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ export type LocalDbItem = LocalListDbItem | LocalDatabaseDbItem;

export interface LocalListDbItem {
kind: DbItemKind.LocalList;
selected: boolean;
listName: string;
databases: LocalDatabaseDbItem[];
}

export interface LocalDatabaseDbItem {
kind: DbItemKind.LocalDatabase;
selected: boolean;
databaseName: string;
dateAdded: number;
language: string;
Expand All @@ -51,23 +53,74 @@ export type RemoteDbItem =

export interface RemoteSystemDefinedListDbItem {
kind: DbItemKind.RemoteSystemDefinedList;
selected: boolean;
listName: string;
listDisplayName: string;
listDescription: string;
}

export interface RemoteUserDefinedListDbItem {
kind: DbItemKind.RemoteUserDefinedList;
selected: boolean;
listName: string;
repos: RemoteRepoDbItem[];
}

export interface RemoteOwnerDbItem {
kind: DbItemKind.RemoteOwner;
selected: boolean;
ownerName: string;
}

export interface RemoteRepoDbItem {
kind: DbItemKind.RemoteRepo;
selected: boolean;
repoFullName: string;
}

export function isRemoteSystemDefinedListDbItem(
dbItem: DbItem,
): dbItem is RemoteSystemDefinedListDbItem {
return dbItem.kind === DbItemKind.RemoteSystemDefinedList;
}

export function isRemoteUserDefinedListDbItem(
dbItem: DbItem,
): dbItem is RemoteUserDefinedListDbItem {
return dbItem.kind === DbItemKind.RemoteUserDefinedList;
}

export function isRemoteOwnerDbItem(
dbItem: DbItem,
): dbItem is RemoteOwnerDbItem {
return dbItem.kind === DbItemKind.RemoteOwner;
}

export function isRemoteRepoDbItem(dbItem: DbItem): dbItem is RemoteRepoDbItem {
return dbItem.kind === DbItemKind.RemoteRepo;
}

export function isLocalListDbItem(dbItem: DbItem): dbItem is LocalListDbItem {
return dbItem.kind === DbItemKind.LocalList;
}

export function isLocalDatabaseDbItem(
dbItem: DbItem,
): dbItem is LocalDatabaseDbItem {
return dbItem.kind === DbItemKind.LocalDatabase;
}
Comment on lines +81 to +111
Copy link
Copy Markdown
Contributor

@norascheuch norascheuch Dec 1, 2022

Choose a reason for hiding this comment

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

These 30 lines are only created for tests. Isn't that a huge code smell? Can't we solve the problem differently? The two isLocal... are not used at all.

Copy link
Copy Markdown
Contributor Author

@charisk charisk Dec 1, 2022

Choose a reason for hiding this comment

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

We can definitely solve this differently - it requires checking the dbItem.kind and then casting the item to the relevant type, which is a little bit more fiddly that simply using type guards.

We could move the code somewhere else too since it's just for tests but I think that would make it harder to find and much more likely for this code to get duplicated elsewhere. We could also only implement what is needed (and don't include the type guards for local dbs) but to me that feels incomplete.

And despite this being 30 LoC, it's 30 lines of straight-forward code.

So in general I think you have the right reactions seeing this code, but I think this is a case where it's ok to break the "rules" a bit. And the db-item module comes with a more complete interface.


export type SelectableDbItem = RemoteDbItem | LocalDbItem;

export function isSelectableDbItem(dbItem: DbItem): dbItem is SelectableDbItem {
return SelectableDbItemKinds.includes(dbItem.kind);
}

const SelectableDbItemKinds = [
DbItemKind.LocalList,
DbItemKind.LocalDatabase,
DbItemKind.RemoteSystemDefinedList,
DbItemKind.RemoteUserDefinedList,
DbItemKind.RemoteOwner,
DbItemKind.RemoteRepo,
];
95 changes: 78 additions & 17 deletions extensions/ql-vscode/src/databases/db-tree-creator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
LocalDatabase,
LocalList,
RemoteRepositoryList,
SelectedDbItemKind,
} from "./config/db-config";
import {
DbItemKind,
Expand All @@ -18,16 +19,20 @@ import {

export function createRemoteTree(dbConfig: DbConfig): RootRemoteDbItem {
const systemDefinedLists = [
createSystemDefinedList(10),
createSystemDefinedList(100),
createSystemDefinedList(1000),
createSystemDefinedList(10, dbConfig),
createSystemDefinedList(100, dbConfig),
createSystemDefinedList(1000, dbConfig),
];

const userDefinedRepoLists = dbConfig.databases.remote.repositoryLists.map(
createUserDefinedList,
(r) => createRemoteUserDefinedList(r, dbConfig),
);
const owners = dbConfig.databases.remote.owners.map((o) =>
createOwnerItem(o, dbConfig),
);
const repos = dbConfig.databases.remote.repositories.map((r) =>
createRepoItem(r, dbConfig),
);
const owners = dbConfig.databases.remote.owners.map(createOwnerItem);
const repos = dbConfig.databases.remote.repositories.map(createRepoItem);

return {
kind: DbItemKind.RootRemote,
Expand All @@ -41,62 +46,118 @@ export function createRemoteTree(dbConfig: DbConfig): RootRemoteDbItem {
}

export function createLocalTree(dbConfig: DbConfig): RootLocalDbItem {
const localLists = dbConfig.databases.local.lists.map(createLocalList);
const localDbs = dbConfig.databases.local.databases.map(createLocalDb);
const localLists = dbConfig.databases.local.lists.map((l) =>
createLocalList(l, dbConfig),
);
const localDbs = dbConfig.databases.local.databases.map((l) =>
createLocalDb(l, dbConfig),
);

return {
kind: DbItemKind.RootLocal,
children: [...localLists, ...localDbs],
};
}

function createSystemDefinedList(n: number): RemoteSystemDefinedListDbItem {
function createSystemDefinedList(
n: number,
dbConfig: DbConfig,
): RemoteSystemDefinedListDbItem {
const listName = `top_${n}`;

const selected =
dbConfig.selected &&
dbConfig.selected.kind === SelectedDbItemKind.RemoteSystemDefinedList &&
dbConfig.selected.listName === listName;

return {
kind: DbItemKind.RemoteSystemDefinedList,
listName: `top_${n}`,
listName,
listDisplayName: `Top ${n} repositories`,
listDescription: `Top ${n} repositories of a language`,
selected: !!selected,
};
}

function createUserDefinedList(
function createRemoteUserDefinedList(
list: RemoteRepositoryList,
dbConfig: DbConfig,
): RemoteUserDefinedListDbItem {
const selected =
dbConfig.selected &&
dbConfig.selected.kind === SelectedDbItemKind.RemoteUserDefinedList &&
dbConfig.selected.listName === list.name;

return {
kind: DbItemKind.RemoteUserDefinedList,
listName: list.name,
repos: list.repositories.map((r) => createRepoItem(r)),
repos: list.repositories.map((r) => createRepoItem(r, dbConfig, list.name)),
selected: !!selected,
};
}

function createOwnerItem(owner: string): RemoteOwnerDbItem {
function createOwnerItem(owner: string, dbConfig: DbConfig): RemoteOwnerDbItem {
const selected =
dbConfig.selected &&
dbConfig.selected.kind === SelectedDbItemKind.RemoteOwner &&
dbConfig.selected.ownerName === owner;

return {
kind: DbItemKind.RemoteOwner,
ownerName: owner,
selected: !!selected,
};
}

function createRepoItem(repo: string): RemoteRepoDbItem {
function createRepoItem(
repo: string,
dbConfig: DbConfig,
listName?: string,
): RemoteRepoDbItem {
const selected =
dbConfig.selected &&
dbConfig.selected.kind === SelectedDbItemKind.RemoteRepository &&
dbConfig.selected.repositoryName === repo &&
dbConfig.selected.listName === listName;

return {
kind: DbItemKind.RemoteRepo,
repoFullName: repo,
selected: !!selected,
};
}

function createLocalList(list: LocalList): LocalListDbItem {
function createLocalList(list: LocalList, dbConfig: DbConfig): LocalListDbItem {
const selected =
dbConfig.selected &&
dbConfig.selected.kind === SelectedDbItemKind.LocalUserDefinedList &&
dbConfig.selected.listName === list.name;

return {
kind: DbItemKind.LocalList,
listName: list.name,
databases: list.databases.map(createLocalDb),
databases: list.databases.map((d) => createLocalDb(d, dbConfig, list.name)),
selected: !!selected,
};
}

function createLocalDb(db: LocalDatabase): LocalDatabaseDbItem {
function createLocalDb(
db: LocalDatabase,
dbConfig: DbConfig,
listName?: string,
): LocalDatabaseDbItem {
const selected =
dbConfig.selected &&
dbConfig.selected.kind === SelectedDbItemKind.LocalDatabase &&
dbConfig.selected.databaseName === db.name &&
dbConfig.selected.listName === listName;

return {
kind: DbItemKind.LocalDatabase,
databaseName: db.name,
dateAdded: db.dateAdded,
language: db.language,
storagePath: db.storagePath,
selected: !!selected,
};
}
11 changes: 11 additions & 0 deletions extensions/ql-vscode/src/databases/ui/db-tree-view-item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as vscode from "vscode";
import {
DbItem,
isSelectableDbItem,
LocalDatabaseDbItem,
LocalListDbItem,
RemoteOwnerDbItem,
Expand Down Expand Up @@ -28,6 +29,16 @@ export class DbTreeViewItem extends vscode.TreeItem {
public readonly children: DbTreeViewItem[],
) {
super(label, collapsibleState);

if (dbItem && isSelectableDbItem(dbItem)) {
if (dbItem.selected) {
// Define the resource id to drive the UI to render this item as selected.
this.resourceUri = vscode.Uri.parse("codeql://databases?selected=true");
} else {
// Define a context value to drive the UI to show an action to select the item.
this.contextValue = "selectableDbItem";
}
}
}
}

Expand Down
Loading