OpenDialog Plugin API #2398

vitaliy-guliy · 2018-07-19T14:15:30Z

Implementation of Open Dialog Plugin API

Issue #2153

Depends on #2401

Signed-off-by: Vitaliy Gulyy vgulyy@redhat.com

akosyakov · 2018-07-20T06:00:24Z

I will review the core part next week.

akosyakov · 2018-07-24T04:49:46Z

packages/filesystem/src/browser/file-dialog/file-dialog-tree-filters-renderer.ts

+    [name: string]: string[];
+}
+
+export class FileDialogTreeFiltersRenderer extends VirtualRenderer {


we should not use VirtualRenderer anymore, please read deprecation note

Used ReactRenderer like in LocationListRenderer

akosyakov · 2018-07-24T04:53:44Z

packages/filesystem/src/browser/file-dialog/file-dialog-tree.ts

+        const result = await Promise.all(
+            fileStat.children
+                .filter(child => {
+                    return this.visible(child);


please get rid of linting warnings in new files

akosyakov · 2018-07-24T04:56:16Z

packages/filesystem/src/browser/file-dialog/file-dialog-tree.ts

+     *
+     * @param fileStat resource to check
+     */
+    protected visible(fileStat: FileStat): boolean {


method name should start from a verb: visible -> filter os isFiltered

Changed to isVisible(..)

akosyakov · 2018-07-24T04:59:28Z

packages/filesystem/src/browser/file-dialog/file-dialog-container.ts

@@ -31,13 +32,16 @@ export function createFileDialogContainer(parent: interfaces.Container): Contain
    child.unbind(FileTreeWidget);
    child.bind(FileDialogWidget).toSelf();

+    child.bind(FileDialogTree).toSelf();
+    child.rebind(Tree).toDynamicValue(ctx => ctx.container.get(FileDialogTree));


child.rebind(Tree).toService(FileDialogTree);

toService should be used for transitive bindings, toDynamicValue is an old workaround

Done. Thanks!

akosyakov · 2018-07-24T05:05:52Z

packages/filesystem/src/browser/file-dialog/file-dialog-container.ts

-export function createFileDialog(parent: interfaces.Container, props: FileDialogProps): FileDialog {
-    const container = createFileDialogContainer(parent);
+export function createFileDialog(parent: interfaces.Container, props: FileDialogProps, treeProps?: TreeProps): FileDialog {
+    const container = createFileDialogContainer(parent, treeProps);


TreeProps are supposed to be rebound, please revert and rebind them here:

const container = createFileDialogContainer(parent); container.rebind(TreeProps).toConstantValue({ ...defaultTreeProps, multiSelect: props.canSelectMany });

Done. Thanks!

akosyakov · 2018-07-24T05:13:18Z

packages/filesystem/src/browser/file-tree/file-tree-container.ts

 import { FileTree } from "./file-tree";
 import { FileTreeModel } from './file-tree-model';
 import { FileTreeWidget } from "./file-tree-widget";

-export function createFileTreeContainer(parent: interfaces.Container): Container {
-    const child = createTreeContainer(parent);
+export function createFileTreeContainer(parent: interfaces.Container, treeProps?: TreeProps): Container {


please revert this file

akosyakov · 2018-07-24T05:13:25Z

packages/core/src/browser/tree/tree-container.ts

@@ -24,7 +24,7 @@ import { TreeExpansionService, TreeExpansionServiceImpl } from "./tree-expansion
 import { TreeNavigationService } from './tree-navigation';
 import { TreeDecoratorService, NoopTreeDecoratorService } from './tree-decorator';

-export function createTreeContainer(parent: interfaces.Container): Container {
+export function createTreeContainer(parent: interfaces.Container, treeProps?: TreeProps): Container {


please revert this file

akosyakov · 2018-07-24T05:18:00Z

packages/plugin-ext/src/main/browser/plugin-manager-client.ts

-        const name = this.labelProvider.getName(rootUri);
-        const label = await this.labelProvider.getIcon(root);
+        const name = this.labelProvider.getName(rootStat);
+        const label = await this.labelProvider.getIcon(rootStat);
        const rootNode = DirNode.createRoot(rootStat, name, label);
        const dialog = this.fileDialogFactory({ title: HostedPluginCommands.SELECT_PATH.label! });


Can we somehow trick ts to return a result of different types depending on incoming dialog parameters? i.e. if multiSelect is not true, then it should be a single file select dialog that clients don't need to check for other cases. Type guards below are redundant, woud be nice to remove unnecessary code.

Reworked a little bit. Could you please give a look again?

akosyakov · 2018-07-24T05:18:45Z

packages/plugin-ext/src/main/browser/quick-open-main.ts

+            const result = await dialog.open();
+
+            // Check the result
+            if (result) {


Have you seen UriSelection.getUris?

akosyakov · 2018-07-24T05:20:00Z

packages/workspace/src/browser/workspace-frontend-contribution.ts

-                    const node = await dialog.open();
-                    this.openFile(node);
+                    const result = await dialog.open();
+                    if (result instanceof Array) {


We should find a way to get rid it of it. all type checking is redundant here

I added FileSelection return type and wrote a corresponding namespace with set of functions like UriSelection.

akosyakov · 2018-08-02T06:01:40Z

packages/filesystem/src/browser/file-dialog/file-dialog.ts

+
+export type FileSelection = Readonly<FileStatNode> | Readonly<FileStatNode>[] | undefined;
+
+export namespace FileSelection {


Sorry for the confusion, I meant you can just use UriSelection, FileSelection is redundant, i.e.

const uri = UriSelection.getUri(await dialog.open()); const URIs = UriSelection.getUris(await dialog.open());

Well. How can we use URI here?
https://github.com/theia-ide/theia/blob/68e8ef46e5aab9cc82b7546d565704c9ae86035a/packages/workspace/src/browser/workspace-frontend-contribution.ts#L80

Why do we expect multiple files here? It could be:

if (FileStatNode.is(result)) { this.open(result); }

It was a workaround. But it could be working when the dialog will return an array.
I used MaybeArray for the file dialog

class FileDialog extends AbstractDialog<MaybeArray<FileStatNode>>

Then it gives FileStatNode | FileStatNode[] | undeifned
But FileStatNode.is(..) receives only node: TreeNode | undefined and it's not suitable for an array. I think we have to improve FileStatNode somehow.

feel free to change FileStatNode.is that it accepts an arg of object | undefined type: https://github.com/theia-ide/theia/blob/3ef537c96f57cb1c9ebb1b331e8c93bc0fb74db1/packages/filesystem/src/browser/file-tree/file-tree.ts#L96

I like it :)

akosyakov · 2018-08-02T06:03:43Z

packages/filesystem/src/browser/file-dialog/file-dialog.ts

 }

 @injectable()
-export class FileDialog extends AbstractDialog<Readonly<FileStatNode> | undefined> {
+export class FileDialog extends AbstractDialog<FileSelection> {


fyi: AbstractDialog<MaybeArray<FileStatNode>>

akosyakov

LGTM

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

vitaliy-guliy requested review from benoitf, evidolob, ashumilova and mmorhun July 19, 2018 14:15

benoitf requested a review from akosyakov July 19, 2018 14:27

akosyakov reviewed Jul 24, 2018

View reviewed changes

vitaliy-guliy force-pushed the show-open-dialog branch 3 times, most recently from de3a125 to 68e8ef4 Compare August 1, 2018 14:06

akosyakov reviewed Aug 2, 2018

View reviewed changes

ashumilova mentioned this pull request Aug 2, 2018

IDE2-2018-08-21 (Sprint: 153) eclipse-che/che#10461

Closed

26 tasks

vitaliy-guliy force-pushed the show-open-dialog branch from c5b882e to 7b33b0f Compare August 2, 2018 10:10

akosyakov approved these changes Aug 2, 2018

View reviewed changes

Open Dialog plugin API

840b49f

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

vitaliy-guliy force-pushed the show-open-dialog branch from 9d89ee8 to 840b49f Compare August 2, 2018 13:06

vitaliy-guliy merged commit 26aecff into master Aug 2, 2018

vitaliy-guliy deleted the show-open-dialog branch August 2, 2018 14:22

vitaliy-guliy restored the show-open-dialog branch August 2, 2018 14:32

vitaliy-guliy deleted the show-open-dialog branch August 3, 2018 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenDialog Plugin API #2398

OpenDialog Plugin API #2398

vitaliy-guliy commented Jul 19, 2018 •

edited

Loading

akosyakov commented Jul 20, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Jul 30, 2018

akosyakov Jul 24, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Jul 30, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Jul 31, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Jul 31, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Jul 31, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Jul 31, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Aug 1, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Aug 1, 2018

akosyakov Jul 24, 2018

vitaliy-guliy Aug 1, 2018

akosyakov Aug 2, 2018 •

edited

Loading

vitaliy-guliy Aug 2, 2018

akosyakov Aug 2, 2018 •

edited

Loading

vitaliy-guliy Aug 2, 2018 •

edited

Loading

akosyakov Aug 2, 2018

vitaliy-guliy Aug 2, 2018

akosyakov Aug 2, 2018

akosyakov left a comment


		export type FileSelection = Readonly<FileStatNode> \| Readonly<FileStatNode>[] \| undefined;

		export namespace FileSelection {

OpenDialog Plugin API #2398

OpenDialog Plugin API #2398

Conversation

vitaliy-guliy commented Jul 19, 2018 • edited Loading

akosyakov commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

vitaliy-guliy Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

vitaliy-guliy commented Jul 19, 2018 •

edited

Loading

akosyakov Aug 2, 2018 •

edited

Loading

akosyakov Aug 2, 2018 •

edited

Loading

vitaliy-guliy Aug 2, 2018 •

edited

Loading