Repository: electron/fiddle
Pull request #538 (2020-11-25 23:56:52)

fix: improve Mosaic behaviour on Fiddle load
Couple changes here. To see examples of expected behaviour, see added tests for `replaceFiddle()`.

### Don't show empty mosaics

Previously, we would only avoid showing mosaics that had "empty" values according to what we published to Gist:

https://github.com/electron/fiddle/blob/445e17361ce3dcf4a3334bb9948f383bbcf3b711/src/renderer/constants.ts#L35-L39

This was problematic for Show Me demos and local Fiddles, which would have actual empty strings instead. This is fixed with an additional guard against the length of the content string.

### Enforced mosaic arrangements

Following the work done in #525, I decided to nip this problem at the bud and load our mosaic in some exact order. The `replaceFiddle()` function now sorts the `Object.keys` result of the editorValues being passed in the exact order of Main, Renderer, HTML, Preload, CSS.

### Test cleanup

Added some `replaceFiddle` specs. Noticed that there was a lot of boilerplate setup in each test. Moved that code to a `beforeEach()` block.

cc @issacgerges 

coveralls: 
[![Coverage Status](https://coveralls.io/builds/35252293/badge)](https://coveralls.io/builds/35252293)

Coverage increased (+0.03%) to 93.631% when pulling **ead6f602ab7e55be01eea30f8d11019e4f169729 on erick/show-me-result** into **445e17361ce3dcf4a3334bb9948f383bbcf3b711 on master**.


Patch:
diff --git a/src/main/main.ts b/src/main/main.ts
index 457d669c..7c3c11b3 100644
--- a/src/main/main.ts
+++ b/src/main/main.ts
@@ -45,16 +45,7 @@ export async function onReady() {
  */
 export function onBeforeQuit() {
   ipcMainManager.send(IpcEvents.BEFORE_QUIT);
-  ipcMainManager.on(IpcEvents.CONFIRM_QUIT, quitAppIfConfirmed);
-}
-
-export function quitAppIfConfirmed(
-  _: Electron.IpcMainEvent,
-  quitConfirmed: boolean,
-) {
-  if (quitConfirmed) {
-    app.quit();
-  }
+  ipcMainManager.on(IpcEvents.CONFIRM_QUIT, app.quit);
 }
 
 export function setupMenuHandler() {
diff --git a/src/renderer/app.tsx b/src/renderer/app.tsx
index 808d55e8..ffa9db2f 100644
--- a/src/renderer/app.tsx
+++ b/src/renderer/app.tsx
@@ -71,11 +71,28 @@ export class App {
 
     // display all editors that have content
     const visibleEditors = [];
-    for (const id of Object.keys(editorValues)) {
+
+    // ensure consistent sorting of mosaics
+    const sortedEditors = Object.keys(editorValues).sort((a, b) => {
+      const order: Array<string> = [
+        'main',
+        'renderer',
+        'html',
+        'preload',
+        'css',
+      ];
+
+      return order.indexOf(a) - order.indexOf(b);
+    });
+
+    for (const id of sortedEditors) {
       const content = editorValues[id];
 
-      // if the gist content matches the empty file output, don't show it
-      if (!Object.values(EMPTY_EDITOR_CONTENT).includes(content)) {
+      // if the gist content is empty or matches the empty file output, don't show it
+      if (
+        content.length > 0 &&
+        !Object.values(EMPTY_EDITOR_CONTENT).includes(content)
+      ) {
         visibleEditors.push(id as EditorId);
       }
     }
diff --git a/src/renderer/components/commands-action-button.tsx b/src/renderer/components/commands-action-button.tsx
index 7c4ad5ba..b0083d31 100644
--- a/src/renderer/components/commands-action-button.tsx
+++ b/src/renderer/components/commands-action-button.tsx
@@ -176,6 +176,7 @@ export class GistActionButton extends React.Component<
 
     if (description) {
       await this.publishGist(description);
+      appState.isUnsaved = false;
     }
 
     appState.genericDialogLastInput = null;
@@ -199,6 +200,7 @@ export class GistActionButton extends React.Component<
         files: this.gistFilesList(values) as any,
       });
 
+      appState.isUnsaved = false;
       console.log('Updating: Updating done', { gist });
       this.renderToast({ message: 'Successfully updated gist!' });
     } catch (error) {
@@ -231,6 +233,7 @@ export class GistActionButton extends React.Component<
         gist_id: appState.gistId!,
       });
 
+      appState.isUnsaved = true;
       console.log('Deleting: Deleting done', { gist });
       this.renderToast({ message: 'Successfully deleted gist!' });
     } catch (error) {
diff --git a/src/renderer/state.ts b/src/renderer/state.ts
index fc1a3a28..80c0fb34 100644
--- a/src/renderer/state.ts
+++ b/src/renderer/state.ts
@@ -250,32 +250,30 @@ export class AppState {
     autorun(() => {
       if (typeof this.isUnsaved === 'undefined') return;
 
-      if (!!this.isUnsaved) {
+      if (this.isUnsaved) {
         window.onbeforeunload = () => {
           ipcRendererManager.send(IpcEvents.SHOW_INACTIVE);
           this.setGenericDialogOptions({
             type: GenericDialogType.warning,
             label: `The current Fiddle is unsaved. Do you want to exit anyway?`,
-            ok: 'Quit',
+            ok: 'Exit',
           });
 
           this.isGenericDialogShowing = true;
 
           // We'll wait until the warning dialog was closed
           when(() => !this.isGenericDialogShowing).then(() => {
-            const quitConfirmed = this.genericDialogLastResult;
+            const closeConfirmed = this.genericDialogLastResult;
             // The user confirmed, let's close for real.
-            if (quitConfirmed) {
+            if (closeConfirmed) {
+              // isQuitting checks if we're trying to quit the app
+              // or just close the window
+              if (this.isQuitting) {
+                ipcRendererManager.send(IpcEvents.CONFIRM_QUIT);
+              }
               window.onbeforeunload = null;
               window.close();
             }
-
-            // isQuitting checks if we're trying to quit the app
-            // or just close the window
-            if (this.isQuitting) {
-              ipcRendererManager.send(IpcEvents.CONFIRM_QUIT, quitConfirmed);
-              this.isQuitting = false;
-            }
           });
 
           // return value doesn't matter, we just want to cancel the event


Test patch:
diff --git a/tests/main/main-spec.ts b/tests/main/main-spec.ts
index bc4427ae..b097a984 100644
--- a/tests/main/main-spec.ts
+++ b/tests/main/main-spec.ts
@@ -11,7 +11,6 @@ import {
   onBeforeQuit,
   onReady,
   onWindowsAllClosed,
-  quitAppIfConfirmed,
   setupMenuHandler,
 } from '../../src/main/main';
 import { shouldQuit } from '../../src/main/squirrel';
@@ -78,23 +77,11 @@ describe('main', () => {
       expect(ipcMainManager.send).toHaveBeenCalledWith(IpcEvents.BEFORE_QUIT);
       expect(ipcMainManager.on).toHaveBeenCalledWith(
         IpcEvents.CONFIRM_QUIT,
-        quitAppIfConfirmed,
+        app.quit,
       );
     });
   });
 
-  describe('quitAppIfConfirmed()', () => {
-    it('quits app if passed a truthy value', () => {
-      quitAppIfConfirmed({} as any, true);
-      expect(app.quit).toHaveBeenCalledTimes(1);
-    });
-
-    it('does nothing if passed a value', () => {
-      quitAppIfConfirmed({} as any, false);
-      expect(app.quit).toHaveBeenCalledTimes(0);
-    });
-  });
-
   describe('onReady()', () => {
     it('opens a BrowserWindow, sets up updates', async () => {
       await onReady();
diff --git a/tests/renderer/app-spec.tsx b/tests/renderer/app-spec.tsx
index a5bdc23e..c2cca04f 100644
--- a/tests/renderer/app-spec.tsx
+++ b/tests/renderer/app-spec.tsx
@@ -4,6 +4,7 @@ import { EditorBackup } from '../../src/utils/editor-backup';
 import { ElectronFiddleMock } from '../mocks/electron-fiddle';
 import { MockState } from '../mocks/state';
 import { overridePlatform, resetPlatform } from '../utils';
+import { EditorId } from '../../src/interfaces';
 
 jest.mock('../../src/renderer/file-manager', () =>
   require('../mocks/file-manager'),
@@ -110,18 +111,24 @@ describe('Editors component', () => {
   });
 
   describe('replaceFiddle()', () => {
-    it('sets editor values and source info', (done) => {
-      const app = new App();
+    let app: App;
+
+    beforeEach(() => {
+      app = new App();
       (app.state as Partial<AppState>) = new MockState();
       app.state.isUnsaved = false;
+      app.state.setGenericDialogOptions = jest.fn();
       app.state.setVisibleMosaics = jest.fn();
       app.setEditorValues = jest.fn();
+    });
 
+    it('sets editor values and source info', (done) => {
       const editorValues = {
         html: 'html-value',
         main: 'main-value',
         renderer: 'renderer-value',
         preload: '',
+        css: '',
       };
 
       app
@@ -139,14 +146,55 @@ describe('Editors component', () => {
         });
     });
 
+    it('only shows mosaic for non-empty editor contents', (done) => {
+      const editorValues = {
+        main: 'main-value',
+        renderer: 'renderer-value',
+        html: 'html-value',
+        preload: '',
+        css: '/* Empty */',
+      };
+
+      app
+        .replaceFiddle(editorValues, {
+          gistId: 'gistId',
+        })
+        .then(() => {
+          expect(app.state.setVisibleMosaics).toHaveBeenCalledWith([
+            EditorId.main,
+            EditorId.renderer,
+            EditorId.html,
+          ]);
+          done();
+        });
+    });
+
+    it('shows visible mosaics in the correct pre-defined order', (done) => {
+      // this order is defined inside the replaceFiddle() function
+      const editorValues = {
+        css: 'css-value',
+        html: 'html-value',
+        renderer: 'renderer-value',
+        main: 'main-value',
+      };
+      app
+        .replaceFiddle(editorValues, {
+          gistId: 'gistId',
+        })
+        .then(() => {
+          expect(app.state.setVisibleMosaics).toHaveBeenCalledWith([
+            EditorId.main,
+            EditorId.renderer,
+            EditorId.html,
+            EditorId.css,
+          ]);
+          done();
+        });
+    });
+
     it('unsets state of previous source when called', (done) => {
-      const app = new App();
-      (app.state as Partial<AppState>) = new MockState();
       app.state.isUnsaved = true;
       app.state.localPath = '/fake/path';
-      app.state.setGenericDialogOptions = jest.fn();
-      app.state.setVisibleMosaics = jest.fn();
-      app.setEditorValues = jest.fn();
 
       const editorValues = {
         html: 'html-value',
@@ -173,12 +221,6 @@ describe('Editors component', () => {
     });
 
     it('marks the new Fiddle as Saved', (done) => {
-      const app = new App();
-      (app.state as Partial<AppState>) = new MockState();
-      app.state.isUnsaved = false;
-      app.state.setVisibleMosaics = jest.fn();
-      app.setEditorValues = jest.fn();
-
       const editorValues = {
         html: 'html-value',
         main: 'main-value',
@@ -196,15 +238,10 @@ describe('Editors component', () => {
           done();
         });
     });
+
     describe('when current Fiddle is unsaved and prompt appears', () => {
       it('takes no action if prompt is rejected', (done) => {
-        const app = new App();
-        (app.state as Partial<AppState>) = new MockState();
         app.state.isUnsaved = true;
-        app.state.setVisibleMosaics = jest.fn();
-        app.state.setGenericDialogOptions = jest.fn();
-        app.setEditorValues = jest.fn();
-
         expect(app.state.localPath).toBeUndefined();
         expect(app.state.gistId).toBe('');
         expect(app.state.templateName).toBeUndefined();
diff --git a/tests/renderer/state-spec.ts b/tests/renderer/state-spec.ts
index 2e2d3a18..2f7220b8 100644
--- a/tests/renderer/state-spec.ts
+++ b/tests/renderer/state-spec.ts
@@ -74,6 +74,7 @@ describe('AppState', () => {
 
   beforeEach(() => {
     appState = new AppState();
+    appState.updateDownloadedVersionState = jest.fn();
     ipcRendererManager.removeAllListeners();
   });
 
@@ -115,7 +116,6 @@ describe('AppState', () => {
         expect(window.close).toHaveBeenCalledTimes(1);
         expect(ipcRendererManager.send).toHaveBeenCalledWith(
           IpcEvents.CONFIRM_QUIT,
-          true,
         );
         done();
       });
@@ -135,9 +135,8 @@ describe('AppState', () => {
       appState.isQuitting = true;
       process.nextTick(() => {
         expect(window.close).toHaveBeenCalledTimes(0);
-        expect(ipcRendererManager.send).toHaveBeenCalledWith(
+        expect(ipcRendererManager.send).not.toHaveBeenCalledWith(
           IpcEvents.CONFIRM_QUIT,
-          false,
         );
         done();
       });
@@ -477,6 +476,8 @@ describe('AppState', () => {
 
   describe('updateDownloadedVersionState()', () => {
     beforeEach(() => {
+      appState = new AppState();
+      ipcRendererManager.removeAllListeners();
       (getDownloadingVersions as jest.Mock).mockReturnValue(['2.0.1']);
       (getDownloadedVersions as jest.Mock).mockReturnValue(
         Promise.resolve(['2.0.2']),