No save dialog if another widget shares identical model (#10614)

examples/api-tests/src/saveable.spec.js

@@ -33,6 +33,7 @@ describe('Saveable', function () {
     const { Disposable, DisposableCollection } = require('@theia/core/lib/common/disposable');
 
     const container = window.theia.container;
+    /** @type {EditorManager} */
     const editorManager = container.get(EditorManager);
     const workspaceService = container.get(WorkspaceService);
     const fileService = container.get(FileService);
@@ -266,6 +267,17 @@ describe('Saveable', function () {
         assert.equal(state.value.trimRight(), 'bar', 'fs should be updated');
     });
 
+    it('no save prompt when multiple editors open for same file', async () => {
+        const secondWidget = await editorManager.openToSide(fileUri);
+        editor.getControl().setValue('two widgets');
+        assert.isTrue(Saveable.isDirty(widget), 'the first widget should be dirty');
+        assert.isTrue(Saveable.isDirty(secondWidget), 'the second widget should also be dirty');
+        await Promise.resolve(secondWidget.close());
+        assert.isTrue(secondWidget.isDisposed, 'the widget should have closed without requesting user action');
+        assert.isTrue(Saveable.isDirty(widget), 'the original widget should still be dirty.');
+        assert.equal(editor.getControl().getValue(), 'two widgets', 'should still have the same value');
+    });
+
     it('normal close', async () => {
         editor.getControl().setValue('bar');
         assert.isTrue(Saveable.isDirty(widget), 'should be dirty before before close');

packages/core/src/browser/saveable.ts

@@ -93,51 +93,93 @@ export namespace Saveable {
             await saveable.save(options);
         }
     }
-    export function apply(widget: Widget): SaveableWidget | undefined {
-        if (SaveableWidget.is(widget)) {
-            return widget;
-        }
-        const saveable = Saveable.get(widget);
-        if (!saveable) {
-            return undefined;
+
+    async function closeWithoutSaving(this: PostCreationSaveableWidget, doRevert: boolean = true): Promise<void> {
+        const saveable = get(this);
+        if (saveable && doRevert && saveable.dirty && saveable.revert) {
+            await saveable.revert();
         }
-        setDirty(widget, saveable.dirty);
-        saveable.onDirtyChanged(() => setDirty(widget, saveable.dirty));
-        const closeWidget = widget.close.bind(widget);
-        const closeWithoutSaving: SaveableWidget['closeWithoutSaving'] = async () => {
-            if (saveable.dirty && saveable.revert) {
-                await saveable.revert();
-            }
-            closeWidget();
-            return waitForClosed(widget);
-        };
+        this[close]();
+        return waitForClosed(this);
+    }
+
+    function createCloseWithSaving(getOtherSaveables?: () => Array<Widget | SaveableWidget>): (this: SaveableWidget, options?: SaveableWidget.CloseOptions) => Promise<void> {
         let closing = false;
-        const closeWithSaving: SaveableWidget['closeWithSaving'] = async options => {
-            if (closing) {
-                return;
-            }
+        return async function (this: SaveableWidget, options: SaveableWidget.CloseOptions): Promise<void> {
+            if (closing) { return; }
+            const saveable = get(this);
+            if (!saveable) { return; }
             closing = true;
             try {
                 const result = await shouldSave(saveable, () => {
+                    const notLastWithDocument = !closingWidgetWouldLoseSaveable(this, getOtherSaveables?.() ?? []);
+                    if (notLastWithDocument) {
+                        return this.closeWithoutSaving(false).then(() => undefined);
+                    }
                     if (options && options.shouldSave) {
                         return options.shouldSave();
                     }
-                    return new ShouldSaveDialog(widget).open();
+                    return new ShouldSaveDialog(this).open();
                 });
                 if (typeof result === 'boolean') {
                     if (result) {
-                        await Saveable.save(widget);
+                        await Saveable.save(this);
                     }
-                    await closeWithoutSaving();
+                    await this.closeWithoutSaving();
                 }
             } finally {
                 closing = false;
             }
         };
-        return Object.assign(widget, {
+    }
+
+    export async function confirmSaveBeforeClose(toClose: Iterable<Widget>, others: Widget[]): Promise<boolean | undefined> {
+        for (const widget of toClose) {
+            const saveable = Saveable.get(widget);
+            if (saveable?.dirty) {
+                if (!closingWidgetWouldLoseSaveable(widget, others)) {
+                    continue;
+                }
+                const userWantsToSave = await new ShouldSaveDialog(widget).open();
+                if (userWantsToSave === undefined) { // User clicked cancel.
+                    return undefined;
+                } else if (userWantsToSave) {
+                    await saveable.save();
+                } else {
+                    await saveable.revert?.();
+                }
+            }
+        }
+        return true;
+    }
+
+    /**
+     * @param widget the widget that may be closed
+     * @param others widgets that will not be closed.
+     * @returns `true` if widget is saveable and no widget among the `others` refers to the same saveable. `false` otherwise.
+     */
+    function closingWidgetWouldLoseSaveable(widget: Widget, others: Widget[]): boolean {
+        const saveable = get(widget);
+        return !!saveable && !others.some(otherWidget => otherWidget !== widget && get(otherWidget) === saveable);
+    }
+
+    export function apply(widget: Widget, getOtherSaveables?: () => Array<Widget | SaveableWidget>): SaveableWidget | undefined {
+        if (SaveableWidget.is(widget)) {
+            return widget;
+        }
+        const saveable = Saveable.get(widget);
+        if (!saveable) {
+            return undefined;
+        }
+        const saveableWidget = widget as SaveableWidget;
+        setDirty(saveableWidget, saveable.dirty);
+        saveable.onDirtyChanged(() => setDirty(saveableWidget, saveable.dirty));
+        const closeWithSaving = createCloseWithSaving(getOtherSaveables);
+        return Object.assign(saveableWidget, {
             closeWithoutSaving,
             closeWithSaving,
-            close: () => closeWithSaving()
+            close: closeWithSaving,
+            [close]: saveableWidget.close,
         });
     }
     export async function shouldSave(saveable: Saveable, cb: () => MaybePromise<boolean | undefined>): Promise<boolean | undefined> {
@@ -154,9 +196,24 @@ export namespace Saveable {
 }
 
 export interface SaveableWidget extends Widget {
-    closeWithoutSaving(): Promise<void>;
+    /**
+     * @param doRevert whether the saveable should be reverted before being saved. Defaults to `true`.
+     */
+    closeWithoutSaving(doRevert?: boolean): Promise<void>;
     closeWithSaving(options?: SaveableWidget.CloseOptions): Promise<void>;
 }
+
+export const close = Symbol('close');
+/**
+ * An interface describing saveable widgets that are created by the `Saveable.apply` function.
+ * The original `close` function is reassigned to a locally-defined `Symbol`
+ */
+export interface PostCreationSaveableWidget extends SaveableWidget {
+    /**
+     * The original `close` function of the widget
+     */
+    [close](): void;
+}
 export namespace SaveableWidget {
     export function is(widget: Widget | undefined): widget is SaveableWidget {
         return !!widget && 'closeWithoutSaving' in widget;

packages/core/src/browser/shell/application-shell.ts

@@ -24,7 +24,7 @@ import { Message } from '@phosphor/messaging';
 import { IDragEvent } from '@phosphor/dragdrop';
 import { RecursivePartial, Event as CommonEvent, DisposableCollection, Disposable, environment } from '../../common';
 import { animationFrame } from '../browser';
-import { Saveable, SaveableWidget, SaveOptions } from '../saveable';
+import { Saveable, SaveableWidget, SaveOptions, SaveableSource } from '../saveable';
 import { StatusBarImpl, StatusBarEntry, StatusBarAlignment } from '../status-bar/status-bar';
 import { TheiaDockPanel, BOTTOM_AREA_ID, MAIN_AREA_ID } from './theia-dock-panel';
 import { SidePanelHandler, SidePanel, SidePanelHandlerFactory } from './side-panel-handler';
@@ -1025,7 +1025,7 @@ export class ApplicationShell extends Widget {
         }
         this.tracker.add(widget);
         this.checkActivation(widget);
-        Saveable.apply(widget);
+        Saveable.apply(widget, () => this.widgets.filter((maybeSaveable): maybeSaveable is Widget & SaveableSource => !!Saveable.get(maybeSaveable)));
         if (ApplicationShell.TrackableWidgetProvider.is(widget)) {
             for (const toTrack of widget.getTrackableWidgets()) {
                 this.track(toTrack);
@@ -1036,6 +1036,11 @@ export class ApplicationShell extends Widget {
         }
     }
 
+    /**
+     * @returns an array of Widgets, all of which are tracked by the focus tracker
+     * The first member of the array is the widget whose id is passed in, and the other widgets
+     * are its tracked parents in ascending order
+     */
     protected toTrackedStack(id: string): Widget[] {
         const tracked = new Map<string, Widget>(this.tracker.widgets.map(w => [w.id, w] as [string, Widget]));
         let current = tracked.get(id);
@@ -1392,24 +1397,23 @@ export class ApplicationShell extends Widget {
      * @param filter
      *      If undefined, all tabs are closed; otherwise only those tabs that match the filter are closed.
      */
-    closeTabs(tabBarOrArea: TabBar<Widget> | ApplicationShell.Area,
-        filter?: (title: Title<Widget>, index: number) => boolean): void {
+    async closeTabs(tabBarOrArea: TabBar<Widget> | ApplicationShell.Area,
+        filter?: (title: Title<Widget>, index: number) => boolean): Promise<void> {
+        const titles: Array<Title<Widget>> = [];
         if (tabBarOrArea === 'main') {
-            this.mainAreaTabBars.forEach(tb => this.closeTabs(tb, filter));
+            this.mainAreaTabBars.forEach(tabbar => titles.push(...toArray(tabbar.titles)));
         } else if (tabBarOrArea === 'bottom') {
-            this.bottomAreaTabBars.forEach(tb => this.closeTabs(tb, filter));
+            this.bottomAreaTabBars.forEach(tabbar => titles.push(...toArray(tabbar.titles)));
         } else if (typeof tabBarOrArea === 'string') {
-            const tabBar = this.getTabBarFor(tabBarOrArea);
-            if (tabBar) {
-                this.closeTabs(tabBar, filter);
+            const tabbar = this.getTabBarFor(tabBarOrArea);
+            if (tabbar) {
+                titles.push(...toArray(tabbar.titles));
             }
         } else if (tabBarOrArea) {
-            const titles = toArray(tabBarOrArea.titles);
-            for (let i = 0; i < titles.length; i++) {
-                if (filter === undefined || filter(titles[i], i)) {
-                    titles[i].owner.close();
-                }
-            }
+            titles.push(...toArray(tabBarOrArea.titles));
+        }
+        if (titles.length) {
+            await this.closeMany((filter ? titles.filter(filter) : titles).map(title => title.owner));
         }
     }
 
@@ -1436,24 +1440,33 @@ export class ApplicationShell extends Widget {
         }
     }
 
+    /**
+     * @param targets the widgets to be closed
+     * @return an array of all the widgets that were actually closed.
+     */
+    async closeMany(targets: Widget[], options?: ApplicationShell.CloseOptions): Promise<Widget[]> {
+        if (options?.save === false || await Saveable.confirmSaveBeforeClose(targets, this.widgets.filter(widget => !targets.includes(widget)))) {
+            return (await Promise.all(targets.map(target => this.closeWidget(target.id, options)))).filter((widget): widget is Widget => widget !== undefined);
+        }
+        return [];
+    }
+
+    /**
+     * @returns the widget that was closed, if any, `undefined` otherwise.
+     *
+     * If your use case requires closing multiple widgets, use {@link ApplicationShell#closeMany} instead. That method handles closing saveable widgets more reliably.
+     */
     async closeWidget(id: string, options?: ApplicationShell.CloseOptions): Promise<Widget | undefined> {
         // TODO handle save for composite widgets, i.e. the preference widget has 2 editors
         const stack = this.toTrackedStack(id);
         const current = stack.pop();
         if (!current) {
             return undefined;
         }
-        let pendingClose;
-        if (SaveableWidget.is(current)) {
-            let shouldSave;
-            if (options && 'save' in options) {
-                shouldSave = () => options.save;
-            }
-            pendingClose = current.closeWithSaving({ shouldSave });
-        } else {
-            current.close();
-            pendingClose = waitForClosed(current);
-        };
+        const saveableOptions = options && { shouldSave: () => options.save };
+        const pendingClose = SaveableWidget.is(current)
+            ? current.closeWithSaving(saveableOptions)
+            : (current.close(), waitForClosed(current));
         await Promise.all([
             pendingClose,
             this.pendingUpdates

packages/core/src/browser/widget-open-handler.ts

@@ -160,8 +160,6 @@ export abstract class WidgetOpenHandler<W extends BaseWidget> implements OpenHan
      * @returns a promise of all closed widgets that resolves after they have been closed.
      */
     async closeAll(options?: ApplicationShell.CloseOptions): Promise<W[]> {
-        const closed = await Promise.all(this.all.map(widget => this.shell.closeWidget(widget.id, options)));
-        return closed.filter(widget => !!widget) as W[];
+        return this.shell.closeMany(this.all, options) as Promise<W[]>;
     }
-
 }

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

@@ -22,9 +22,9 @@ import { Command, CommandContribution, CommandRegistry } from '@theia/core/lib/c
 import {
     FrontendApplicationContribution, ApplicationShell,
     NavigatableWidget, NavigatableWidgetOptions,
-    Saveable, WidgetManager, StatefulWidget, FrontendApplication, ExpandableTreeNode, waitForClosed,
+    Saveable, WidgetManager, StatefulWidget, FrontendApplication, ExpandableTreeNode,
     CorePreferences,
-    CommonCommands
+    CommonCommands,
 } from '@theia/core/lib/browser';
 import { MimeService } from '@theia/core/lib/browser/mime-service';
 import { TreeWidgetSelection } from '@theia/core/lib/browser/tree/tree-widget-selection';
@@ -272,24 +272,20 @@ export class FileSystemFrontendContribution implements FrontendApplicationContri
         if (!event.gotDeleted() && !event.gotAdded()) {
             return;
         }
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        const pending: Promise<any>[] = [];
-
         const dirty = new Set<string>();
         const toClose = new Map<string, NavigatableWidget[]>();
         for (const [uri, widget] of NavigatableWidget.get(this.shell.widgets)) {
-            this.updateWidget(uri, widget, event, { dirty, toClose });
+            this.updateWidget(uri, widget, event, { dirty, toClose: toClose });
         }
-        for (const [uriString, widgets] of toClose.entries()) {
-            if (!dirty.has(uriString) && this.corePreferences['workbench.editor.closeOnFileDelete']) {
-                for (const widget of widgets) {
-                    widget.close();
-                    pending.push(waitForClosed(widget));
+        if (this.corePreferences['workbench.editor.closeOnFileDelete']) {
+            const doClose = [];
+            for (const [uri, widgets] of toClose.entries()) {
+                if (!dirty.has(uri)) {
+                    doClose.push(...widgets);
                 }
             }
+            await this.shell.closeMany(doClose);
         }
-
-        await Promise.all(pending);
     }
     protected updateWidget(uri: URI, widget: NavigatableWidget, event: FileChangesEvent, { dirty, toClose }: {
         dirty: Set<string>;

packages/navigator/src/browser/navigator-contribution.ts

@@ -348,7 +348,7 @@ export class FileNavigatorContribution extends AbstractViewContribution<FileNavi
             }
         });
         registry.registerCommand(OpenEditorsCommands.CLOSE_ALL_TABS_FROM_TOOLBAR, {
-            execute: widget => this.withOpenEditorsWidget(widget, () => this.editorWidgets.forEach(editor => editor.close())),
+            execute: widget => this.withOpenEditorsWidget(widget, () => this.shell.closeMany(this.editorWidgets)),
             isEnabled: widget => this.withOpenEditorsWidget(widget, () => !!this.editorWidgets.length),
             isVisible: widget => this.withOpenEditorsWidget(widget, () => !!this.editorWidgets.length)
         });

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

@@ -349,11 +349,8 @@ export class PluginVscodeCommandsContribution implements CommandContribution {
                         return (resourceUri && resourceUri.toString()) === uriString;
                     });
                 }
-                for (const widget of this.shell.widgets) {
-                    if (this.codeEditorWidgetUtil.is(widget) && widget !== editor) {
-                        await this.shell.closeWidget(widget.id);
-                    }
-                }
+                const toClose = this.shell.widgets.filter(widget => widget !== editor && this.codeEditorWidgetUtil.is(widget));
+                await this.shell.closeMany(toClose);
             }
         });
 
@@ -449,13 +446,8 @@ export class PluginVscodeCommandsContribution implements CommandContribution {
         });
         commands.registerCommand({ id: 'workbench.action.closeAllEditors' }, {
             execute: async () => {
-                const promises = [];
-                for (const widget of this.shell.widgets) {
-                    if (this.codeEditorWidgetUtil.is(widget)) {
-                        promises.push(this.shell.closeWidget(widget.id));
-                    }
-                }
-                await Promise.all(promises);
+                const toClose = this.shell.widgets.filter(widget => this.codeEditorWidgetUtil.is(widget));
+                await this.shell.closeMany(toClose);
             }
         });
         commands.registerCommand({ id: 'workbench.action.nextEditor' }, {

packages/workspace/src/browser/workspace-delete-handler.ts

@@ -206,12 +206,7 @@ export class WorkspaceDeleteHandler implements UriCommandHandler<URI[]> {
      * @param uri URI of a selected resource.
      */
     protected async closeWithoutSaving(uri: URI): Promise<void> {
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        const pending: Promise<any>[] = [];
-        for (const [, widget] of NavigatableWidget.getAffected(this.shell.widgets, uri)) {
-            pending.push(this.shell.closeWidget(widget.id, { save: false }));
-        }
-        await Promise.all(pending);
+        const toClose = [...NavigatableWidget.getAffected(this.shell.widgets, uri)].map(([, widget]) => widget);
+        await this.shell.closeMany(toClose, { save: false });
     }
-
 }