From 4ec5bec719e3bffb9d0f2f3ff90bd9e6690d901a Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Thu, 12 Mar 2020 11:53:29 -0700 Subject: [PATCH] fix: load unsave handlers only after editors mount (#351) --- src/renderer/app.tsx | 22 ---------------------- src/renderer/components/editors.tsx | 10 +++++----- src/renderer/file-manager.ts | 1 - src/renderer/state.ts | 15 +++++++++++++-- tests/renderer/app-spec.tsx | 21 --------------------- tests/renderer/state-spec.ts | 23 +++++++++++++++++++---- 6 files changed, 37 insertions(+), 55 deletions(-) diff --git a/src/renderer/app.tsx b/src/renderer/app.tsx index 2a7d3f0f06..dffed17238 100644 --- a/src/renderer/app.tsx +++ b/src/renderer/app.tsx @@ -78,7 +78,6 @@ export class App { // once loaded, we have a "saved" state this.state.isUnsaved = false; - this.setupUnsavedOnChangeListener(); return true; } @@ -177,30 +176,9 @@ export class App { ipcRenderer.send(WEBCONTENTS_READY_FOR_IPC_SIGNAL); - // TODO: A timer here is terrible. Let's fix this - // and ensure we actually do it once Editors have mounted. - setTimeout(() => { - this.setupUnsavedOnChangeListener(); - }, 1500); - return rendered; } - /** - * If the editor is changed for the first time, we'll - * set `isUnsaved` to true. That way, the app can warn you - * if you're about to throw things away. - */ - public setupUnsavedOnChangeListener() { - Object.keys(window.ElectronFiddle.editors).forEach((key) => { - const editor = window.ElectronFiddle.editors[key]; - const disposable = editor.onDidChangeModelContent(() => { - this.state.isUnsaved = true; - disposable.dispose(); - }); - }); - } - /** * Loads theme CSS into the HTML document. * diff --git a/src/renderer/components/editors.tsx b/src/renderer/components/editors.tsx index 1060984aaa..9b5bc17d10 100644 --- a/src/renderer/components/editors.tsx +++ b/src/renderer/components/editors.tsx @@ -66,8 +66,6 @@ export class Editors extends React.Component { this.setFocused = this.setFocused.bind(this); this.state = { monacoOptions: defaultMonacoOptions }; - - this.loadMonaco(); } /** @@ -75,7 +73,7 @@ export class Editors extends React.Component { * * @memberof Editors */ - public componentDidMount() { + public async componentDidMount() { ipcRendererManager.on(IpcEvents.MONACO_EXECUTE_COMMAND, (_event, cmd: string) => { this.executeCommand(cmd); }); @@ -95,6 +93,8 @@ export class Editors extends React.Component { }); this.setState({ isMounted: true }); + await this.loadMonaco(); + this.props.appState.isUnsaved = false; } public componentWillUnmount() { @@ -291,7 +291,7 @@ export class Editors extends React.Component { * We're doing things a bit roundabout to ensure that we're not overloading the * mobx state with a gigantic Monaco tree. */ - public async loadMonaco(): Promise { + public async loadMonaco() { const { app } = window.ElectronFiddle; const loader = require('monaco-loader'); const monaco = app.monaco || await loader(); @@ -309,7 +309,7 @@ export class Editors extends React.Component { this.setState({ monaco }); } - activateTheme(monaco, undefined, this.props.appState.theme); + await activateTheme(monaco, undefined, this.props.appState.theme); } /** diff --git a/src/renderer/file-manager.ts b/src/renderer/file-manager.ts index 10bc6aaa3c..3eeac53832 100644 --- a/src/renderer/file-manager.ts +++ b/src/renderer/file-manager.ts @@ -110,7 +110,6 @@ export class FileManager { } this.appState.isUnsaved = false; - window.ElectronFiddle.app.setupUnsavedOnChangeListener(); } } diff --git a/src/renderer/state.ts b/src/renderer/state.ts index 6c19c653c3..6ee7442a79 100644 --- a/src/renderer/state.ts +++ b/src/renderer/state.ts @@ -104,7 +104,7 @@ export class AppState { @observable public isPublishing: boolean = false; @observable public isRunning: boolean = false; - @observable public isUnsaved: boolean = false; + @observable public isUnsaved: boolean; @observable public isUpdatingElectronVersions: boolean = false; // -- Various "isShowing" settings ------------------ @@ -167,7 +167,9 @@ export class AppState { autorun(() => this.save('statesToShow', this.statesToShow)); autorun(() => { - if (this.isUnsaved) { + if (typeof this.isUnsaved === 'undefined') return; + + if (!!this.isUnsaved) { window.onbeforeunload = () => { ipcRendererManager.send(IpcEvents.SHOW_INACTIVE); this.setGenericDialogOptions({ @@ -200,6 +202,15 @@ export class AppState { }; } else { window.onbeforeunload = null; + + // set up editor listeners to verify if unsaved + Object.keys(window.ElectronFiddle.editors).forEach((key) => { + const editor = window.ElectronFiddle.editors[key]; + const disposable = editor.onDidChangeModelContent(() => { + this.isUnsaved = true; + disposable.dispose(); + }); + }); } }); diff --git a/tests/renderer/app-spec.tsx b/tests/renderer/app-spec.tsx index c329e7776f..b643a917a9 100644 --- a/tests/renderer/app-spec.tsx +++ b/tests/renderer/app-spec.tsx @@ -40,11 +40,9 @@ describe('Editors component', () => { const app = new App(); const result = (await app.setup()) as HTMLDivElement; - app.setupUnsavedOnChangeListener = jest.fn(); jest.runAllTimers(); expect(result.innerHTML).toBe('Dialogs;Header;OutputEditorsWrapper;'); - expect(app.setupUnsavedOnChangeListener).toHaveBeenCalled(); jest.useRealTimers(); }); @@ -174,7 +172,6 @@ describe('Editors component', () => { const app = new App(); (app.state as Partial) = new MockState(); app.state.isUnsaved = false; - app.setupUnsavedOnChangeListener = jest.fn(); app.setEditorValues = jest.fn(); const editorValues = { @@ -190,7 +187,6 @@ describe('Editors component', () => { }) .then(() => { expect(app.state.isUnsaved).toBe(false); - expect(app.setupUnsavedOnChangeListener).toHaveBeenCalled(); done(); }); }); @@ -336,23 +332,6 @@ describe('Editors component', () => { }); }); - describe('setupUnsavedOnChangeListener()', () => { - it('listens for model change events', async () => { - const app = new App(); - - app.setupUnsavedOnChangeListener(); - - const fn = window.ElectronFiddle.editors!.renderer! - .onDidChangeModelContent; - const call = (fn as jest.Mock).mock.calls[0]; - const cb = call[0]; - - cb(); - - expect(app.state.isUnsaved).toBe(true); - }); - }); - describe('setupResizeListener()', () => { it('attaches to the handler', () => { window.addEventListener = jest.fn(); diff --git a/tests/renderer/state-spec.ts b/tests/renderer/state-spec.ts index ae3cf04ae9..a297bc8fdf 100644 --- a/tests/renderer/state-spec.ts +++ b/tests/renderer/state-spec.ts @@ -55,8 +55,8 @@ describe('AppState', () => { expect(appState).toBeTruthy(); }); - describe('onbeforeunload handler', () => { - it('closes the window', (done) => { + describe('isUnsaved autorun handler', () => { + it('can close the window if user accepts the dialog', (done) => { window.close = jest.fn(); appState.isUnsaved = true; expect(window.onbeforeunload).toBeTruthy(); @@ -73,7 +73,7 @@ describe('AppState', () => { }); }); - it('closes the app', (done) => { + it('can close the app after user accepts dialog', (done) => { const { remote } = require('electron'); window.close = jest.fn(); appState.isUnsaved = true; @@ -93,7 +93,7 @@ describe('AppState', () => { }); }); - it('does not close the window', (done) => { + it('takes no action if user cancels the dialog', (done) => { window.close = jest.fn(); appState.isUnsaved = true; expect(window.onbeforeunload).toBeTruthy(); @@ -109,6 +109,21 @@ describe('AppState', () => { done(); }); }); + + it('sets the onDidChangeModelContent handler if saved', () => { + appState.isUnsaved = false; + + expect(window.onbeforeunload).toBe(null); + + const fn = window.ElectronFiddle.editors!.renderer! + .onDidChangeModelContent; + const call = (fn as jest.Mock).mock.calls[0]; + const cb = call[0]; + + cb(); + + expect(appState.isUnsaved).toBe(true); + }); }); describe('updateElectronVersions()', () => {