Skip to content

Star Problems Extension && All Problems Category #188

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

Closed
wants to merge 10 commits into from
14 changes: 14 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@
"title": "Show Problem",
"category": "LeetCode"
},
{
"command": "leetcode.starProblem",
"title": "Star/Unstar Problem",
"category": "LeetCode"
},
{
"command": "leetcode.searchProblem",
"title": "Search Problem",
Expand Down Expand Up @@ -164,12 +169,21 @@
"command": "leetcode.showProblem",
"when": "view == leetCodeExplorer && viewItem == problem",
"group": "leetcode@1"
},
{
"command": "leetcode.starProblem",
"when": "view == leetCodeExplorer && viewItem == problem",
"group": "leetcode@1"
}
],
"commandPalette": [
{
"command": "leetcode.showProblem",
"when": "never"
},
{
"command": "leetcode.starProblem",
"when": "never"
}
],
"explorer/context": [
Expand Down
19 changes: 19 additions & 0 deletions src/commands/star.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) jdneo. All rights reserved.
// Licensed under the MIT license.

import { LeetCodeNode } from "../explorer/LeetCodeNode";
import { LeetCodeTreeDataProvider } from "../explorer/LeetCodeTreeDataProvider";
import { leetCodeExecutor } from "../leetCodeExecutor";
import { IProblem } from "../shared";
import { DialogType, promptForOpenOutputChannel } from "../utils/uiUtils";

export async function starProblem(provider: LeetCodeTreeDataProvider, node: LeetCodeNode): Promise<void> {
try {
const problem: IProblem = Object.assign({}, node.nodeData, {
isFavorite: await leetCodeExecutor.starProblem(node, !node.isFavorite),
});
provider.updateProblem(problem);
} catch (error) {
await promptForOpenOutputChannel("Failed to star the problem. Please open the output channel for details.", DialogType.error);
}
}
29 changes: 17 additions & 12 deletions src/explorer/LeetCodeNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,26 @@ import { IProblem, ProblemState } from "../shared";
export class LeetCodeNode {
constructor(private data: IProblem, private parentNodeName: string, private isProblemNode: boolean = true) { }

public get nodeData(): IProblem {
return this.data;
}

public get isProblem(): boolean {
return this.isProblemNode;
}

public get parentName(): string {
return this.parentNodeName;
}

public get locked(): boolean {
return this.data.locked;
}

public get name(): string {
return this.data.name;
}

public get state(): ProblemState {
return this.data.state;
}

public get id(): string {
return this.data.id;
}
Expand All @@ -37,15 +46,11 @@ export class LeetCodeNode {
return this.data.companies;
}

public get isFavorite(): boolean {
return this.data.isFavorite;
}

public get isProblem(): boolean {
return this.isProblemNode;
public get state(): ProblemState {
return this.data.state;
}

public get parentName(): string {
return this.parentNodeName;
public get isFavorite(): boolean {
return this.data.isFavorite;
}
}
55 changes: 50 additions & 5 deletions src/explorer/LeetCodeTreeDataProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { LeetCodeNode } from "./LeetCodeNode";

export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCodeNode> {

private allProblems: Map<string, IProblem>; // store reference of all problems.
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we separate allProblems with treeData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • allProblems is an id-IProblem dictionary primarily for fast lookup, whose structure is not compatible with treeData;
  • allProblems serves as a buffer which holds all the problems' ownership, where elements in treeData is just references to the problems;
  • with regard to this.onDidChangeTreeDataEvent.fire(problem) review below, we may need another lookup dictionary for fast retrieving relative nodes. allProblems seems more similar to it rather than treeData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now allProblems is decoupled with treeData, and will be grouped with allTreeNodes as an object pool in next PR.


private treeData: {
Difficulty: Map<string, IProblem[]>,
Tag: Map<string, IProblem[]>,
Expand All @@ -31,6 +33,14 @@ export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCod
this.onDidChangeTreeDataEvent.fire();
}

public async updateProblem(problem: IProblem): Promise<void> {
if (this.allProblems.has(problem.id)) {
this.updateTreeDataByProblem(problem); // only modify the content of tree data, problem is not updated.
Object.assign(this.allProblems.get(problem.id), problem); // update problem, where reference is preserved.
this.onDidChangeTreeDataEvent.fire();
Copy link
Member

Choose a reason for hiding this comment

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

I guess that the reason that explorer returns to uncollapsed state is that, we do not pass the node element into this.onDidChangeTreeDataEvent.fire() (This API can accept a node element. Then all the data are cleared before the explorer rerendering.

Copy link
Contributor Author

@Vigilans Vigilans Mar 6, 2019

Choose a reason for hiding this comment

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

After some investigations I seems to get the hang of this API. Maybe we can keep another reference container here:

allTreeNodes: Map<string, LeetcodeNode[]> // id -> relative nodes

Then, in updateProblem function, we can fire events to request all relative nodes to be updated.

Copy link
Member

Choose a reason for hiding this comment

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

@Vigilans Cool. Noticed that now the same problem in the different category has different id. So if we refresh the Two Sum in All Problem, will the Two Sum in Difficulty be refreshed?

That means, I'm thinking that if we should make the same problem has the same id in the whole explorer. Not sure the real behavior of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further investigations, to accomplish this feature, there are some important changes which I think should be discussed in next PR. In this PR, we may temporarily accept the current behavior?

Copy link
Member

Choose a reason for hiding this comment

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

So updateProblem() is not used in this PR, right?

}
}

public getTreeItem(element: LeetCodeNode): vscode.TreeItem | Thenable<vscode.TreeItem> {
if (element.id === "notSignIn") {
return {
Expand All @@ -46,7 +56,7 @@ export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCod

const idPrefix: number = Date.now();
return {
label: element.isProblem ? `[${element.id}] ${element.name}` : element.name,
label: element.isProblem ? `[${element.id}] ${element.name} ${element.isFavorite ? "♥" : ""}` : element.name,
Copy link
Member

Choose a reason for hiding this comment

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

Curious about the appearance on different OS platforms, haha.

tooltip: this.getSubCategoryTooltip(element),
id: `${idPrefix}.${element.parentName}.${element.id}`,
collapsibleState: element.isProblem ? vscode.TreeItemCollapsibleState.None : vscode.TreeItemCollapsibleState.Collapsed,
Expand All @@ -66,6 +76,10 @@ export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCod
}
if (!element) { // Root view
return [
new LeetCodeNode(Object.assign({}, defaultProblem, {
id: Category.All,
name: Category.All,
}), "ROOT", false),
new LeetCodeNode(Object.assign({}, defaultProblem, {
id: Category.Difficulty,
name: Category.Difficulty,
Expand All @@ -85,6 +99,9 @@ export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCod
];
} else {
switch (element.name) { // First-level
case Category.All:
const all: IProblem[] = [...this.allProblems.values()];
return all.map((p: IProblem) => new LeetCodeNode(p, Category.All));
case Category.Favorite:
const nodes: IProblem[] = this.treeData[Category.Favorite];
return nodes.map((p: IProblem) => new LeetCodeNode(p, Category.Favorite));
Expand All @@ -100,13 +117,15 @@ export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCod

private async getProblemData(): Promise<void> {
// clear cache
this.allProblems = new Map<string, IProblem>();
this.treeData = {
Difficulty: new Map<string, IProblem[]>(),
Tag: new Map<string, IProblem[]>(),
Company: new Map<string, IProblem[]>(),
Favorite: [],
};
for (const problem of await list.listProblems()) {
this.allProblems.set(problem.id, problem);
// Add favorite problem, no matter whether it is solved.
if (problem.isFavorite) {
this.treeData[Category.Favorite].push(problem);
Expand Down Expand Up @@ -168,12 +187,26 @@ export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCod
}

private getSubCategoryTooltip(element: LeetCodeNode): string {
// return '' unless it is a sub-category node
if (element.isProblem || !this.treeData[element.parentName]) {
// return '' if it does not directly hold problems.
if (element.isProblem) {
return "";
}

const problems: IProblem[] = this.treeData[element.parentName].get(element.id);
let problems: IProblem[];
switch (element.name) {
case Category.Difficulty:
case Category.Tag:
case Category.Company:
return "";
case Category.All:
problems = [...this.allProblems.values()];
break;
case Category.Favorite:
problems = this.treeData[Category.Favorite];
break;
default:
problems = this.treeData[element.parentName].get(element.id);
break;
}

let acceptedNum: number = 0;
let failedNum: number = 0;
Expand All @@ -197,6 +230,18 @@ export class LeetCodeTreeDataProvider implements vscode.TreeDataProvider<LeetCod
].join(os.EOL);
}

private updateTreeDataByProblem(problem: IProblem): void {
Copy link
Member

Choose a reason for hiding this comment

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

The method name her is confusing. Cuz we only handle isFavorite here, but the method name looks like we can update all the fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we might handle the case of updating other treeData fields the at a later time. Seems a overdesign here?
Should I leave a comment announcing modification of other categories is not needed yet here, or just change the method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sure now other fields will be updated here in the near PRs, e.g. when a problem is accepted, there will be some code here to update other fields.

const origin: IProblem | undefined = this.allProblems.get(problem.id);
if (origin && origin.isFavorite !== problem.isFavorite) {
const problemIndex: number = this.treeData.Favorite.findIndex((p: LeetCodeNode) => Number(p.id) >= Number(problem.id));
Copy link
Member

Choose a reason for hiding this comment

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

can we directly compare p.id with problem.id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problems are originally in number-ascending order, thus the comparation in index searching should also be done this way.
e.g. the favorite problems' ids are ["1", "4", "16", "23"], I want to delete the problem with id "23" and id is compared directly. Since "4" >= "23" === true, the search process stops at "4", thus the deletion is incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, the main problem here is that we store the Favorite problems as an array instead of a Map(id -> Problem).

So you may observe that we have to use findIndex and splice to add/remove items, which I think is hard to read.

Can we change it to a Map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it can be discussed in subsequent PRs, when updating treeData[Category.Company/Tag] field is necessary(delete problem from treeview after it is solved)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, can be solved in another PR.

if (problem.isFavorite) {
this.treeData.Favorite.splice(problemIndex, 0, origin); // insert original problem's reference as favorite
} else {
this.treeData.Favorite.splice(problemIndex, 1); // delete favorite
}
}
}

private addProblemToTreeData(problem: IProblem): void {
this.putProblemToMap(this.treeData.Difficulty, problem.difficulty, problem);
for (const tag of problem.tags) {
Expand Down
2 changes: 2 additions & 0 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { switchDefaultLanguage } from "./commands/language";
import * as plugin from "./commands/plugin";
import * as session from "./commands/session";
import * as show from "./commands/show";
import * as star from "./commands/star";
import * as submit from "./commands/submit";
import * as test from "./commands/test";
import { LeetCodeNode } from "./explorer/LeetCodeNode";
Expand Down Expand Up @@ -44,6 +45,7 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
vscode.commands.registerCommand("leetcode.selectSessions", () => session.selectSession()),
vscode.commands.registerCommand("leetcode.createSession", () => session.createSession()),
vscode.commands.registerCommand("leetcode.showProblem", (node: LeetCodeNode) => show.showProblem(node)),
vscode.commands.registerCommand("leetcode.starProblem", (node: LeetCodeNode) => star.starProblem(leetCodeTreeDataProvider, node)),
vscode.commands.registerCommand("leetcode.searchProblem", () => show.searchProblem()),
vscode.commands.registerCommand("leetcode.refreshExplorer", () => leetCodeTreeDataProvider.refresh()),
vscode.commands.registerCommand("leetcode.testSolution", (uri?: vscode.Uri) => test.testSolution(uri)),
Expand Down
10 changes: 10 additions & 0 deletions src/leetCodeExecutor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@ class LeetCodeExecutor {
return filePath;
}

public async starProblem(node: IProblem, markStarred: boolean): Promise<boolean> {
let description: string = "";
if (markStarred) {
description = await this.executeCommandEx("node", [await this.getLeetCodeBinaryPath(), "star", node.id]);
} else {
description = await this.executeCommandEx("node", [await this.getLeetCodeBinaryPath(), "star", node.id, "-d"]);
}
return description.includes("♥");
}

public async listSessions(): Promise<string> {
return await this.executeCommandEx("node", [await this.getLeetCodeBinaryPath(), "session"]);
}
Expand Down
1 change: 1 addition & 0 deletions src/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const defaultProblem: IProblem = {
};

export enum Category {
All = "All Problems",
Difficulty = "Difficulty",
Tag = "Tag",
Company = "Company",
Expand Down