From 0ad6d10e354af57b43fa7ac677c43c8a4948e494 Mon Sep 17 00:00:00 2001 From: colin-grant-work Date: Mon, 31 Jan 2022 16:48:10 -0600 Subject: [PATCH] No save dialog if another widget shares identical model (#10614) --- examples/api-tests/src/saveable.spec.js | 12 ++ packages/core/src/browser/saveable.ts | 111 +++++++++++++----- .../src/browser/shell/application-shell.ts | 65 ++++++---- .../core/src/browser/widget-open-handler.ts | 4 +- .../filesystem-frontend-contribution.ts | 22 ++-- .../src/browser/navigator-contribution.ts | 2 +- .../plugin-vscode-commands-contribution.ts | 16 +-- .../src/browser/workspace-delete-handler.ts | 9 +- 8 files changed, 152 insertions(+), 89 deletions(-) diff --git a/examples/api-tests/src/saveable.spec.js b/examples/api-tests/src/saveable.spec.js index c9be5aca36917..b5629bf3d0be6 100644 --- a/examples/api-tests/src/saveable.spec.js +++ b/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'); diff --git a/packages/core/src/browser/saveable.ts b/packages/core/src/browser/saveable.ts index cea603bfaa8f2..772b7bbb77866 100644 --- a/packages/core/src/browser/saveable.ts +++ b/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 { + 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): (this: SaveableWidget, options?: SaveableWidget.CloseOptions) => Promise { let closing = false; - const closeWithSaving: SaveableWidget['closeWithSaving'] = async options => { - if (closing) { - return; - } + return async function (this: SaveableWidget, options: SaveableWidget.CloseOptions): Promise { + 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, others: Widget[]): Promise { + 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): 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): Promise { @@ -154,9 +196,24 @@ export namespace Saveable { } export interface SaveableWidget extends Widget { - closeWithoutSaving(): Promise; + /** + * @param doRevert whether the saveable should be reverted before being saved. Defaults to `true`. + */ + closeWithoutSaving(doRevert?: boolean): Promise; closeWithSaving(options?: SaveableWidget.CloseOptions): Promise; } + +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; diff --git a/packages/core/src/browser/shell/application-shell.ts b/packages/core/src/browser/shell/application-shell.ts index 607dae503a1ad..6748a855f6e91 100644 --- a/packages/core/src/browser/shell/application-shell.ts +++ b/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(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 | ApplicationShell.Area, - filter?: (title: Title, index: number) => boolean): void { + async closeTabs(tabBarOrArea: TabBar | ApplicationShell.Area, + filter?: (title: Title, index: number) => boolean): Promise { + const titles: Array> = []; 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,6 +1440,22 @@ 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 { + 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 { // TODO handle save for composite widgets, i.e. the preference widget has 2 editors const stack = this.toTrackedStack(id); @@ -1443,17 +1463,10 @@ export class ApplicationShell extends Widget { 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 diff --git a/packages/core/src/browser/widget-open-handler.ts b/packages/core/src/browser/widget-open-handler.ts index b2f73ff7f0d66..f9edc01906253 100644 --- a/packages/core/src/browser/widget-open-handler.ts +++ b/packages/core/src/browser/widget-open-handler.ts @@ -160,8 +160,6 @@ export abstract class WidgetOpenHandler implements OpenHan * @returns a promise of all closed widgets that resolves after they have been closed. */ async closeAll(options?: ApplicationShell.CloseOptions): Promise { - 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; } - } diff --git a/packages/filesystem/src/browser/filesystem-frontend-contribution.ts b/packages/filesystem/src/browser/filesystem-frontend-contribution.ts index 693761f5a577b..21417438f8b6f 100644 --- a/packages/filesystem/src/browser/filesystem-frontend-contribution.ts +++ b/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[] = []; - const dirty = new Set(); const toClose = new Map(); 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; diff --git a/packages/navigator/src/browser/navigator-contribution.ts b/packages/navigator/src/browser/navigator-contribution.ts index cab4afd12d79c..e6e8b46a48c3e 100644 --- a/packages/navigator/src/browser/navigator-contribution.ts +++ b/packages/navigator/src/browser/navigator-contribution.ts @@ -348,7 +348,7 @@ export class FileNavigatorContribution extends AbstractViewContribution 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) }); diff --git a/packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts b/packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts index 7f679ccefe416..d760b83e68574 100755 --- a/packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts +++ b/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' }, { diff --git a/packages/workspace/src/browser/workspace-delete-handler.ts b/packages/workspace/src/browser/workspace-delete-handler.ts index 54ee0c6cfd05f..ddf793c14f3c5 100644 --- a/packages/workspace/src/browser/workspace-delete-handler.ts +++ b/packages/workspace/src/browser/workspace-delete-handler.ts @@ -206,12 +206,7 @@ export class WorkspaceDeleteHandler implements UriCommandHandler { * @param uri URI of a selected resource. */ protected async closeWithoutSaving(uri: URI): Promise { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const pending: Promise[] = []; - 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 }); } - }