Add hierarchical package view, for issue #57 #117

Flanker32 · 2018-11-30T08:52:20Z

Add hierarchical package view, for issue #57

…efactor get node function of package root

andxu · 2018-12-03T03:27:54Z

src/views/packageRootNode.ts

+    }
+
+    protected createChildNodeList(): ExplorerNode[] {
+        if (Settings.isHierarchicalView()) {


simple to ? :

yaohaizh

See commends. Mostly, you need packagerootnode designed.

yaohaizh · 2018-12-03T04:32:53Z

package.json

          "description": "Synchronize dependency viewer selection with folder explorer",
          "default": true
+        },
+        "java.dependency.isHierarchicalView": {


Should avoid use this value. It is not clear to the end user what the opposite is?

Should be enum values: "flat", "hierarchy"

yaohaizh · 2018-12-03T04:33:52Z

src/views/dependencyExplorer.ts

        if (!current) {
            return;
        }
-


Avoid the empty diff if possible. You can always provide another PR for format/coding convention to avoid distraction.

yaohaizh · 2018-12-03T04:46:11Z

src/views/packageRootNode.ts

+    private packageTree: PackageTreeNode;
+
+    constructor(nodeData: INodeData, parent: DataNode, private _project: ProjectNode, packageTree: PackageTreeNode = null) {
        super(nodeData, parent);


You should NOT mixed the flat package node and hierarchy package node in the same class. It violate SRP and will make the code hard to main in the future.

You can create two class/types for different purpose.

I made an abstract class named HierarchicalNode , which includes createFlatChildNodeList , createHierarchicalChildNodeList and revealPath, and PackageRootNode extends it, i didn't split packagenode into two parts for there are still some common methods like loadData and getIconPath, other methods for Hierarchical was moved to HierachicalPackageRootSubNode

yaohaizh · 2018-12-04T01:59:27Z

src/views/dependencyExplorer.ts

                    } else {
-                        paths.shift();
-                        this.revealPath(c, paths);
+                        // Resove Hierarchical packages


typo: Resolve ==> Resolve

andxu · 2018-12-05T07:41:50Z

src/settings.ts

    }

+    public static isHierarchicalView(): boolean {
+        return this._depdendencyConfig.get("packagePresentation") === "hierarchical";


should avoid to use the configuration directly since the config may change at runtime, in your impl, change of this options will take effective on next update. see https://github.com/Microsoft/vscode-java-debug/blob/bcd69b52e4d941323cae662a154afd2a7852f43d/src/debugCodeLensProvider.ts#L38

andxu · 2018-12-05T07:43:44Z

package.json

+        "java.dependency.packagePresentation": {
+          "type": "string",
+          "enum":["flat","hierarchical"],
+          "description": "Set the presentation model of packages",


Package presentation mode: flat or hierarchical

andxu · 2018-12-05T07:56:44Z

src/java/packageTreeNode.ts

+        }
+    }
+
+    public addPackage(packageName: string): void {


addPackage constructor compressTree should be better organized to clearance to detect tree structures based on a list of packages:
a.b
a.b.c.d
a.b.c.d.e

Second this. Currently, the compressTree will not work for this case.

yaohaizh · 2018-12-05T08:10:47Z

src/views/hierachicalPackageRootNode.ts

+
+    public static async convertPaths(paths: INodeData[]): Promise<INodeData[]> {
+        const index = paths.findIndex((nodeData) => nodeData.kind === NodeKind.PackageRoot);
+        const projectNodeData = paths.find((nodeData) => nodeData.kind === NodeKind.Project);


All these operations are model oriented. You can compute the tree without creating a view.

For example, you can make the packagetree as a the model and build the hierarchy model outof the the view.

andxu · 2018-12-06T06:54:34Z

src/java/packageTreeNode.ts

+
+    public addPackage(packageName: string): void {
+        const packages = packageName.split(".");
+        this.addSubPackage(packages);


this.addSubPackage(packageName.split("."));

andxu · 2018-12-06T08:37:22Z

src/views/hierachicalPackageRootNode.ts

+                }
+            });
+        }
+        const packageNodeList = this.getHierarchicalPackageNodes();


don't introduce local var here

andxu · 2018-12-06T08:44:29Z

src/views/hierachicalPackageRootSubNode.ts

+
+    public packageTree: PackageTreeNode;
+
+    constructor(nodeData: INodeData, parent: DataNode, private _project: ProjectNode, packageTree: PackageTreeNode = null) {


= null should be removed

yaohaizh · 2018-12-06T14:05:35Z

src/views/dependencyDataProvider.ts

    }

+    public async getRootNodeByData(nodeData: INodeData): Promise<DataNode> {
+        // Server only return project nodes, so use get root projects function


As said, in order to get the data, creating the view is not an acceptable solution here. DON't mix the model with your view. You either either directly get the data from the server, or patch the Workspsace data to the path side.

yaohaizh · 2018-12-06T14:23:48Z

src/views/hierachicalPackageRootSubNode.ts

+
+export class HierachicalPackageRootSubNode extends DataNode {
+
+    public packageTree: PackageTreeNode;


This class code is duplicate with the HierarchicalPackageRootNode, could we model these two types in one class?

yaohaizh · 2018-12-06T14:38:57Z

src/java/packageTreeNode.ts

+    }
+
+    public getNodeDataFromPackageTreeNode(nodeData: INodeData): INodeData {
+        return {


Typically, this is an adapter that we can apply here.

src/java/packageTreeNode.ts

yaohaizh · 2018-12-06T14:59:19Z

src/java/packageTreeNode.ts

+        }
+    }
+
+    public addPackage(packageName: string): void {


Second this. Currently, the compressTree will not work for this case.

src/views/hierachicalPackageRootSubNode.ts

…va-dependency into hanxiao_hierarchical

yaohaizh · 2018-12-10T03:03:07Z

src/views/hierachicalPackageRootNode.ts

+    public async revealPaths(paths: INodeData[]): Promise<DataNode> {
+        const hierachicalNodeData = paths[0];
+        const childs: ExplorerNode[] = await this.getChildren();
+        const childNode = <DataNode>childs.find((child: DataNode) =>


Null reference check.

yaohaizh

Fix merge conflication

yaohaizh · 2018-12-10T03:05:23Z

src/views/hierachicalPackageRootNode.ts

+            return HierachicalPackageNodeData.createHierachicalNodeDataByPackageList(nodeDataList);
+        } else {
+            // Return a empty hierachical node
+            return HierachicalPackageNodeData.createHierachicalNodeDataByPackageList([]);


Duplicate call. You just need create package list with arguments. Not necessary two calls here.

Add hierarchical package view, for issue #57

95bf714

Flanker32 requested a review from andxu November 30, 2018 08:52

Update vscode to 1.1.22 to fix vulnerability issue of event-stream; R…

6c8fad4

…efactor get node function of package root

andxu reviewed Dec 3, 2018

View reviewed changes

yaohaizh suggested changes Dec 3, 2018

View reviewed changes

Flanker32 and others added 3 commits December 3, 2018 16:59

Resolve comments

11269cd

Merge branch 'master' into hanxiao_hierarchical

4cbf6f7

Simple codes by comments

19e1c0b

yaohaizh reviewed Dec 4, 2018

View reviewed changes

Flanker32 added 2 commits December 5, 2018 12:59

Refactor hierachical package node structure

8b29ec1

Move path process to PathProcesser

053c463

andxu reviewed Dec 5, 2018

View reviewed changes

yaohaizh reviewed Dec 5, 2018

View reviewed changes

Flanker32 added 3 commits December 5, 2018 20:18

Refactor reveal paths, fix bug to work in workspace

cc21dcd

Refactor package tree node

bfd578b

update hierachical settings

4e75cad

andxu reviewed Dec 6, 2018

View reviewed changes

yaohaizh reviewed Dec 6, 2018

View reviewed changes

yaohaizh suggested changes Dec 6, 2018

View reviewed changes

yaohaizh reviewed Dec 7, 2018

View reviewed changes

src/views/hierachicalPackageRootSubNode.ts Outdated Show resolved Hide resolved

Flanker32 added 4 commits December 7, 2018 15:56

Add node to change representation, refactor code.

fdc2cb7

Merge branch 'master' into hanxiao_hierarchical

44fb789

remove unuse imports

6055b4b

Merge branch 'hanxiao_hierarchical' of github.com:Microsoft/vscode-ja…

bc78c92

…va-dependency into hanxiao_hierarchical

andxu previously approved these changes Dec 10, 2018

View reviewed changes

yaohaizh reviewed Dec 10, 2018

View reviewed changes

Resolve comments

7854efc

Flanker32 dismissed andxu’s stale review via 7854efc December 10, 2018 04:48

Merge branch 'master' into hanxiao_hierarchical

3543836

yaohaizh approved these changes Dec 10, 2018

View reviewed changes

Flanker32 merged commit 58050b0 into master Dec 10, 2018

Flanker32 deleted the hanxiao_hierarchical branch December 10, 2018 04:59

Flanker32 mentioned this pull request Dec 10, 2018

Should have a option for hierarchical tree view other than flat view for package. #57

Closed


		public packageTree: PackageTreeNode;

		constructor(nodeData: INodeData, parent: DataNode, private _project: ProjectNode, packageTree: PackageTreeNode = null) {


		export class HierachicalPackageRootSubNode extends DataNode {

		public packageTree: PackageTreeNode;

Add hierarchical package view, for issue #57 #117

Add hierarchical package view, for issue #57 #117

Uh oh!

Conversation

Flanker32 commented Nov 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaohaizh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaohaizh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants