[WIP] [editor] Renaming a file should rename a corresponding editor #582

lmcgupe · 2017-09-27T23:30:41Z

Signed-off-by: Guy Perron [email protected]

hexa00 · 2017-09-28T00:57:09Z

I tought you were trying to close and open just for experimentation.

But if you ask for review, you should know this is not the proper way forward.
You should listen on filesystem changes and update the title for the widget.

svenefftinge · 2017-09-28T05:50:24Z

We don't need anything in the widget-manager for closing. Just call widget.close().

lmcgupe · 2017-09-28T20:05:58Z

The scope of this fix is when the rename is done within the IDE.

hexa00 · 2017-09-29T15:34:59Z

packages/core/src/browser/widget-manager.ts

+        let key = this.toKey({ factoryId, options });
+        let existingWidget = this.widgets.get(key);
+        if (existingWidget) {
+            existingWidget.title.label = newLabel;


Changing the label of a widget and changing it's key are 2 different things.

You should have a method to change the key here called updateWidgetKey for example.

And the title should be changed after filesysem.move is done and sucessful

It should happen in the editor manager, nothing to do with the widget manager

hexa00 · 2017-09-29T15:35:54Z

packages/workspace/src/browser/workspace-commands.ts

+                dialog.open().then(name => {
+                    renameWidget(this.openerService, uri.parent.resolve(uri.path.base), uri.parent.resolve(name), name).then(() => {
+                        this.fileSystem.move(uri.toString(), uri.parent.resolve(name).toString());
+                    });


we should:

move the file and make sure it worked

update the widget key

change the title

akosyakov · 2017-09-29T17:13:19Z

packages/core/src/browser/opener-service.ts

@@ -43,6 +43,8 @@ export interface OpenHandler {
     * Never reject if `canHandle` return a positive number; otherwise should reject.
     */
    open(uri: URI, options?: OpenerOptions): MaybePromise<object | undefined>;
+
+    renameWidget(uri: URI, newUri: URI, newLabel: string, options?: OpenerOptions): void;


You should not change OpenerService, only EditorManager should be changed.

akosyakov · 2017-09-29T17:16:28Z

To make it right, i think it shold be based on Resource. Editors are not working with files, but with resources, e.g. java resource to open source code, git resource to read history and so on.

@svenefftinge What do you think about adding optional onDidChangeUri event to Resource interface. FileResource will listen to file changes and if a resource is renamed when notify an editor that it could update label and icon.

akosyakov · 2017-09-29T17:20:05Z

To implement it we would need also expose an underlying editor resource via TextEditor interface to allow EditorManager to access it here:
https://github.com/theia-ide/theia/blob/05f2d772f7c6effdcc9afb2642944b5786d4ba6a/packages/editor/src/browser/editor-manager.ts#L116-L121

So it would be here something like:

if (textEditor.resource.onDidChangeUri) {
  textEditor.resource.onDidChangeUri(newUri => {
    newEditor.id = this.id + ":" + newUri.toString();
    newEditor.title.label = newUri.path.base; 
    newEditor.title.iconClass = this.iconProvider.getFileIconForURI(textEditor.newUri);
  });
}

akosyakov · 2017-09-29T17:22:14Z

We also don't have an event so far to indicate a file rename, you should look in the chokidar watcher code in vscode to learn how to identify it. I saw something like that there.

hexa00 · 2017-09-29T17:30:45Z

Nice thanks for the pointers @akosyakov!

Note I think we could do this in 2 steps, first use the UI rename as a trigger to the onDidChangeUri and after use the FileWatcher.

Also note that vscode doesn't rename if a file is moved from the filesystem, it marks the editor as delete from disk if you had unsaved changes or it closes the editor if you had no change.

svenefftinge · 2017-10-01T06:16:47Z

Modifying a widget after creation will break the contract of the widget manager. After reload it will try to create a widget based on the old uri.

akosyakov · 2017-10-01T06:27:07Z

Modifying a widget after creation will break the contract of the widget manager. After reload it will try to create a widget based on the old uri.

Won't it flicker, lose an editor context (selection, viewport and so on) and break an existing layout if we just close one and open another?

akosyakov · 2017-10-01T06:35:06Z

packages/core/src/browser/widget-manager.ts

+     *  check if a widget exists already and rename it
+     */
+    renameWidget(factoryId: string, options: any, newOptions: any, newLabel: any): void {
+        let key = this.toKey({ factoryId, options });


please check that tslint is enabled in VS code, you should have errors here

akosyakov · 2017-10-01T06:35:34Z

packages/core/src/browser/widget-manager.ts

+        let key = this.toKey({ factoryId, options });
+        let existingWidget = this.widgets.get(key);
+        if (existingWidget) {
+            existingWidget.title.label = newLabel;


It should happen in the editor manager, nothing to do with the widget manager

akosyakov · 2017-10-01T06:36:04Z

packages/workspace/src/browser/workspace-commands.ts

@@ -61,7 +62,9 @@ export class WorkspaceCommandContribution implements CommandContribution {
        @inject(FileSystem) protected readonly fileSystem: FileSystem,
        @inject(WorkspaceServer) protected readonly workspaceServer: WorkspaceServer,
        @inject(SelectionService) protected readonly selectionService: SelectionService,
-        @inject(OpenerService) protected readonly openerService: OpenerService
+        @inject(OpenerService) protected readonly openerService: OpenerService,
+        @inject(EditorManagerImpl) private editorService: EditorManagerImpl


program against the interface not implementation, i.e. EditorManager

I moved the code over to the EditorManager, this introduced a dependency on the editor package. But now since the editor package also has a dependency on workspace, it won't compile, complaining that editor package doesn't exist (circular dependency). Any idea ?

Could you try to remove a dependency from @theia/editor to @theia/workspace? The latter does not seem to be used in the former.

akosyakov · 2017-10-01T06:49:32Z

packages/workspace/src/browser/workspace-commands.ts

-                    this.fileSystem.move(uri.toString(), uri.parent.resolve(name).toString())
-                );
+                dialog.open().then(name => {
+                    this.fileSystem.move(uri.toString(), uri.parent.resolve(name).toString()).then(() => {


I think @svenefftinge meant that you only should close an existing editor and open new:

const newUri = uri.parent.resolve(name); this.fileSystem.move(uri.toString(), newUri.toString()).then(() => { const widget = this.editorService.getEditor(uri); if (editor) { const selection = widget.editor.selection; widget.close(); open(this.openerService, newUri, {selection}); } });

Ideally, it should be done not here but on move/rename events from the filesystem in the editor extension. Making it here:

breaks the unidirectional flow, only the filesystem should be the source of truth;

covers only a specific case, e.g. renaming files in Finder won't make the same effect;

makes the workspace extension depend on the editor extension.

No, I did not mean that. A real rename seems better to me in general. But we have to take care of all the loose ends.

Maybe we should have an generic key to reference the widget directly to avoid lose ends ?

Could you elaborate on what you mean?

I mean that if something has to keep a reference to an editor widget it should reference the widget object directly or we should provide a key that is not bound to any resource to refer to it. Like some abstract id.

So the URI would be used as key to open a new widget or find a widget for that URI but once you get a reference to it, it should be the id or the object directly, a bit like files names and files descriptors.. you open(filename) get a file descriptor so you could getWidgetForURI() get a widget for a URI but use getWidgetId() if you already had a widget and now know only it's id. I don't see where however that would be useful.. just keeping the object seems ok since serializing that doesn't make sense as the widget-manager is in the frontend.

Modifying a widget after creation will break the contract of the widget manager. After reload it will try to create a widget based on the old uri.

Why would it try to reload with an old uri ? if you updated it before you serialized the widget ?

Granted if it's changed between save and restore there's nothing we can do but that is true whatever we do...

svenefftinge · 2017-10-01T18:40:52Z

Won't it flicker, lose an editor context (selection, viewport and so on) and break an existing layout if we just close one and open another?

Yes, that's probably not a good solution, but simply changing the widget state doesn't work either.

lmcgupe · 2017-10-02T18:04:59Z

hum.. with the workspace dependency removed from editor, now it makes another error pop up. In the editor package it complains that preferences is not there. [compile] src/browser/editor-preferences.ts(16,8): error TS2307: Cannot find module '@theia/preferences/lib/common Is there a way to specify the order of the compilation ?

________________________________ From: Anton Kosyakov <[email protected]> Sent: October 2, 2017 10:32:04 AM To: theia-ide/theia Cc: Guy Perron; Author Subject: Re: [theia-ide/theia] [WIP] [editor] Renaming a file should rename a corresponding editor (#582) @akosyakov commented on this pull request.

________________________________ In packages/workspace/src/browser/workspace-commands.ts<#582 (comment)>:

@@ -61,7 +62,9 @@ export class WorkspaceCommandContribution implements CommandContribution {

@Inject(FileSystem) protected readonly fileSystem: FileSystem, @Inject(WorkspaceServer) protected readonly workspaceServer: WorkspaceServer, @Inject(SelectionService) protected readonly selectionService: SelectionService, - @Inject(OpenerService) protected readonly openerService: OpenerService + @Inject(OpenerService) protected readonly openerService: OpenerService, + @Inject(EditorManagerImpl) private editorService: EditorManagerImpl Could you try to remove a dependency from @theia/editor to @theia/workspace? The latter does not seem to be used in the former. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#582 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/Aa23i_sYEAdOqprk6VeEt5qnn8nJ18ZFks5soPPkgaJpZM4PmiDZ>.

lmcgupe · 2017-10-02T20:08:15Z

It actually fails now because preferences has a dependency on workspace. So we are back again with another circular dependency : workspace - editor - preferences. That's since the introduction of the editor dependency in workspace. You can fetch my patch if you want to try it out.

________________________________ From: Guy Perron Sent: October 2, 2017 2:04:55 PM To: theia-ide/theia Subject: Re: [theia-ide/theia] [WIP] [editor] Renaming a file should rename a corresponding editor (#582) hum.. with the workspace dependency removed from editor, now it makes another error pop up. In the editor package it complains that preferences is not there. [compile] src/browser/editor-preferences.ts(16,8): error TS2307: Cannot find module '@theia/preferences/lib/common Is there a way to specify the order of the compilation ?

________________________________ From: Anton Kosyakov <[email protected]> Sent: October 2, 2017 10:32:04 AM To: theia-ide/theia Cc: Guy Perron; Author Subject: Re: [theia-ide/theia] [WIP] [editor] Renaming a file should rename a corresponding editor (#582) @akosyakov commented on this pull request.

________________________________ In packages/workspace/src/browser/workspace-commands.ts<#582 (comment)>:

@@ -61,7 +62,9 @@ export class WorkspaceCommandContribution implements CommandContribution {

@Inject(FileSystem) protected readonly fileSystem: FileSystem, @Inject(WorkspaceServer) protected readonly workspaceServer: WorkspaceServer, @Inject(SelectionService) protected readonly selectionService: SelectionService, - @Inject(OpenerService) protected readonly openerService: OpenerService + @Inject(OpenerService) protected readonly openerService: OpenerService, + @Inject(EditorManagerImpl) private editorService: EditorManagerImpl Could you try to remove a dependency from @theia/editor to @theia/workspace? The latter does not seem to be used in the former. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#582 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/Aa23i_sYEAdOqprk6VeEt5qnn8nJ18ZFks5soPPkgaJpZM4PmiDZ>.

akosyakov · 2017-10-03T05:46:49Z

hum.. with the workspace dependency removed from editor, now it makes another error pop up. In the editor package it complains that preferences is not there.

@theia/editor should not depend on @theia/preferences only on @theia/preferences-api, the import should be replaced with from '@theia/preferences-api'.

lmcgupe · 2017-10-03T13:31:34Z

Changing to preferences-api got one step further. Now it fails to find
[compile] src/browser/editor-manager.ts(16,34): error TS2307: Cannot find module '@theia/filesystem/lib/browser/icons/file-icons'.

lmcgupe · 2017-10-03T14:40:39Z

got over it after adding filesystem dependency inside editor's package.json

lmcgupe · 2017-10-05T15:20:57Z

To implement it we would need also expose an underlying editor resource via TextEditor interface to allow EditorManager to access it here:
I modified common/resource.ts by add an onDidChangeUri. Added a resource field inside browser/editor.ts.
In a similar way what is done with OnDidChangeUriEmitter. Concerning this latter method, I tried hitting a break point inside file-resource.ts, in the onDidChangeContents method, after modifying a file, but it didn't trigger. Is that supposed to work ?

akosyakov · 2017-10-06T08:33:34Z

You should put a breakpoint here: https://github.com/theia-ide/theia/blob/31d18c38aa08e6b81d127fdc8de3a03d6796dbbf/packages/filesystem/src/common/file-resource.ts#L28

onDidChangeContents is only hit when someone wants to install a listener, we don't have such clients in the code base, but I used it in the markdown extension:
https://github.com/theia-ide/theia-extension-example/blob/0731d92b3ede2d46e60ddaf61c1e6ff526ad4ad4/theia-markdown/src/browser/markdown-resource.ts#L33-L35

epatpol · 2017-10-06T20:03:24Z

@akosyakov I didn't find anything for a file "rename" in chokidar i.e paulmillr/chokidar#303. I guess we would have to implement it chokidar-client side ourselves?

epatpol · 2017-10-06T20:07:39Z

Note I think we could do this in 2 steps, first use the UI rename as a trigger to the onDidChangeUri and after use the FileWatcher.

I'm not sure about doing it in 2 steps though, how would the ui trigger the onDidChangeUri method of file-resource? Would we add a temporary method to that resource that can trigger the event from outside (instead of from the fs-watcher). Sorry maybe I misunderstood but I'm having an easier time understanding the event from the FS in the backend then UI -> Resource.OnDidChangeUri.fire() (usually the emitter is private right?).

akosyakov · 2017-10-09T10:45:56Z

I meant to look into how vscode using chokidar. I've just looked myself, I was wrong: they are not able to detect rename as well and it only works if it triggered via UI.

hexa00 · 2017-10-10T13:18:36Z

@epatpol even the fs-watcher would need such a method, it would fire it's own moved event I guess and all file resources that match would fire the onDidChangeURI event.

We could do the same with the UI, a move would fire an event and the file resource would listen on that and fire its own event.

If we were to change the UI to the fs-watcher we could just change how fires that first event.

hexa00 · 2017-10-13T15:12:20Z

packages/workspace/src/browser/workspace-commands.ts

+                    this.fileSystem.move(uri.toString(), uri.parent.resolve(name).toString()).then(() => {
+                        this.editorService.renameWidget(uri.parent.resolve(uri.path.base), uri.parent.resolve(name), name);
+                        let event: IUriChangedEvent = { factoryId: uri.parent.resolve(uri.path.base), options: uri.parent.resolve(name), newOptions: name, newLabel: ' ' };
+                        this.uiWatcher.onIUriChangedEmitter.fire(event);


couple of things

you don't need a watcher

nothing actually uses that event at the moment ?

you still call editorservice rename-widget

It should be that workspace-commands fires the event
and editor service receive it at the very least

Might be other considerations too.. need to think about this more

hexa00 · 2017-10-19T17:43:24Z

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

@@ -36,8 +36,11 @@ export default new ContainerModule((bind: interfaces.Bind, unbind: interfaces.Un
        (props: FileDialogProps) =>
            createFileDialog(ctx.container, props)
    );
+    bind(WorkspaceCommandContribution).toSelf().inSingletonScope();


This should not be there

hexa00 · 2017-10-19T17:43:37Z

packages/core/src/browser/widget-manager.ts

@@ -56,7 +56,8 @@ export class WidgetManager {

    constructor(
        @inject(ContributionProvider) @named(WidgetFactory) protected readonly factoryProvider: ContributionProvider<WidgetFactory>,
-        @inject(ILogger) protected logger: ILogger) {
+        @inject(ILogger) protected logger: ILogger
+    ) {


no need to change this

hexa00 · 2017-10-19T17:44:01Z

packages/core/src/browser/widget-manager.ts

+    getWidgetsMap(): Map<string, Widget> {
+        return this.widgets;
+    }
+


this is wrong you should not expose the internal map

hexa00 · 2017-10-19T17:44:53Z

packages/core/src/browser/widget-manager.ts

@@ -105,7 +110,7 @@ export class WidgetManager {
        return undefined;
    }

-    protected toKey(options: WidgetConstructionOptions) {
+    public toKey(options: WidgetConstructionOptions) {


You should not need this public either

hexa00 · 2017-10-19T17:45:10Z

packages/editor/src/browser/editor-frontend-module.ts

@@ -16,6 +16,7 @@ import { EditorCommandHandlers } from "./editor-command";
 import { EditorKeybindingContribution, EditorKeybindingContext } from "./editor-keybinding";
 import { bindEditorPreferences } from './editor-preferences';
 import { WidgetFactory } from '@theia/core/lib/browser/widget-manager';
+// import { WorkspaceCommandContribution } from '@theia/workspace/lib/browser/workspace-commands';


hexa00 · 2017-10-19T17:54:06Z

packages/editor/src/browser/editor-manager.ts

+        options = newOptions;
+        key = this.widgetManager.toKey({ factoryId, options });
+        this.widgetManager.getWidgetsMap().set(key, widget);
+    }


Ditto... this one makes sense in the widget-manager

hexa00 · 2017-10-19T17:55:04Z

packages/workspace/src/browser/workspace-commands.ts

+                            newOptions: uri.parent.resolve(name).toString(),
+                            newLabel: name
+                        };
+                        this.onIUriChangedEmitter.fire(event);


Actually thinking more about this it would make more sense to have FileSytem.move() fire this event and have the event emitter there.
From the workspace command it looks quite odd..

hexa00 · 2017-10-19T17:55:10Z

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

    bind(CommandContribution).to(WorkspaceCommandContribution).inSingletonScope();
    bind(MenuContribution).to(FileMenuContribution).inSingletonScope();

+
+


hexa00 · 2017-10-19T17:56:16Z

packages/workspace/src/browser/workspace-commands.ts

+
+    protected readonly onIUriChangedEmitter = new Emitter<IUriChangedEvent>();
+
+    get onIUriChanged(): Event<IUriChangedEvent> {


We should move this to FileSytem and call it FileMovedEvent and it should be about what moved.

Once the EditorManager receives it it can check if it has a editor for that URI and update it.

hexa00 · 2017-10-19T18:01:39Z

packages/editor/src/browser/editor-manager.ts

    ) {
        this.currentObserver = new EditorManagerImpl.Observer('current', app);
        this.activeObserver = new EditorManagerImpl.Observer('active', app);
+        uiEvent.onIUriChanged(event => {
+            this.renameWidget(event.factoryId.toString(), event.options, event.newOptions, event.newLabel);


We should get the FileMovedEvent from FS here.
And then figureout the proper way to rename based on the URI change. we have all the info needed.

Signed-off-by: Guy Perron <[email protected]>

marcdumais-work · 2018-01-17T16:45:39Z

This PR is not being actively worked-on - closing for now.

lmcgupe requested a review from svenefftinge September 27, 2017 23:31

lmcgupe force-pushed the editor branch from 4ed270d to d30ae55 Compare September 28, 2017 19:55

lmcgupe requested a review from hexa00 September 29, 2017 00:01

hexa00 suggested changes Sep 29, 2017

View reviewed changes

akosyakov reviewed Sep 29, 2017

View reviewed changes

lmcgupe force-pushed the editor branch 2 times, most recently from 43d065c to 8fa4026 Compare September 29, 2017 20:50

akosyakov reviewed Oct 1, 2017

View reviewed changes

lmcgupe force-pushed the editor branch from 8fa4026 to 9f174dd Compare October 2, 2017 14:11

lmcgupe force-pushed the editor branch from 9f174dd to d2e01e7 Compare October 3, 2017 14:38

lmcgupe force-pushed the editor branch from d2e01e7 to d99f55c Compare October 3, 2017 15:10

lmcgupe force-pushed the editor branch from d99f55c to 94593e0 Compare October 6, 2017 14:02

lmcgupe force-pushed the editor branch from 94593e0 to f313f0b Compare October 6, 2017 20:48

lmcgupe force-pushed the editor branch 3 times, most recently from 25cbc36 to 476d42c Compare October 13, 2017 15:01

hexa00 reviewed Oct 13, 2017

View reviewed changes

lmcgupe force-pushed the editor branch 4 times, most recently from 4c16fa7 to 0280980 Compare October 19, 2017 17:33

hexa00 suggested changes Oct 19, 2017

View reviewed changes

[WIP] [editor] Renaming a file should rename a corresponding editor

67187c0

Signed-off-by: Guy Perron <[email protected]>

lmcgupe force-pushed the editor branch from 0280980 to 67187c0 Compare October 23, 2017 18:30

marcdumais-work closed this Jan 17, 2018

epatpol mentioned this pull request Mar 14, 2018

[editor] Renaming a file should rename a corresponding editor #230

Closed

		bind(CommandContribution).to(WorkspaceCommandContribution).inSingletonScope();
		bind(MenuContribution).to(FileMenuContribution).inSingletonScope();


		protected readonly onIUriChangedEmitter = new Emitter<IUriChangedEvent>();

		get onIUriChanged(): Event<IUriChangedEvent> {

[WIP] [editor] Renaming a file should rename a corresponding editor #582

[WIP] [editor] Renaming a file should rename a corresponding editor #582

Conversation

lmcgupe commented Sep 27, 2017

hexa00 commented Sep 28, 2017

svenefftinge commented Sep 28, 2017

lmcgupe commented Sep 28, 2017

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 commented Sep 29, 2017 • edited Loading

akosyakov commented Sep 29, 2017 • edited Loading

akosyakov commented Sep 29, 2017

hexa00 commented Sep 29, 2017

svenefftinge commented Oct 1, 2017

akosyakov commented Oct 1, 2017 • edited Loading

akosyakov Oct 1, 2017 • 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

Choose a reason for hiding this comment

akosyakov Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

svenefftinge Oct 1, 2017 • 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

svenefftinge commented Oct 1, 2017 • edited Loading

lmcgupe commented Oct 2, 2017 via email

lmcgupe commented Oct 2, 2017 via email

akosyakov commented Oct 3, 2017 • edited Loading

lmcgupe commented Oct 3, 2017

lmcgupe commented Oct 3, 2017

lmcgupe commented Oct 5, 2017

akosyakov commented Oct 6, 2017

epatpol commented Oct 6, 2017

epatpol commented Oct 6, 2017

akosyakov commented Oct 9, 2017

hexa00 commented Oct 10, 2017 • 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

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

marcdumais-work commented Jan 17, 2018

akosyakov commented Sep 29, 2017 •

edited

Loading

akosyakov commented Sep 29, 2017 •

edited

Loading

akosyakov commented Oct 1, 2017 •

edited

Loading

akosyakov Oct 1, 2017 •

edited

Loading

akosyakov Oct 1, 2017 •

edited

Loading

svenefftinge Oct 1, 2017 •

edited

Loading

svenefftinge commented Oct 1, 2017 •

edited

Loading

akosyakov commented Oct 3, 2017 •

edited

Loading

hexa00 commented Oct 10, 2017 •

edited

Loading