Merge pull request #291 from charlielee/issue-289

Fix bugs with confirmTake
charlielee · Apr 4, 2021 · 6424e60 · 6424e60
2 parents 707b830 + afbd018
commit 6424e60
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 39 deletions.
diff --git a/src/js/animator/core/ExportVideo.js b/src/js/animator/core/ExportVideo.js
@@ -4,7 +4,7 @@
   const path = require("path");
 
   const ConfirmDialog = require("../ui/ConfirmDialog");
-  const Loader = require("./Loader");
+  const Notification = require("../../common/Notification");
 
   const DEFAULT_FILE_NAME = "output.mp4";
 
@@ -96,22 +96,22 @@
         content: dialogContents,
         buttons: [true, "Export video"]
       })
-      .then((response) => {
+      .then(async (response) => {
         // Confirm the take and render the video if "export video" selected
         if (response) {
-          Loader.show("Confirming take");
-          global.projectInst.currentTake.confirmTake(false)
-          .then(() => {
-            Loader.hide();
+          let isTakeConfirmed = await global.projectInst.currentTake.confirmTake(false);
 
+          if (isTakeConfirmed) {
             // The render method expects an array so convert input from string into array
             // Regexes are to handle arguments in quotes
             // https://stackoverflow.com/a/56119602
             let argumentsArray = customArgumentsInput.value.match(/[^\s"']+|"([^"]*)"/gmi);
             argumentsArray = argumentsArray.map((arg) => arg.replace(/"|'/g, ""));
 
             ExportVideo.render(argumentsArray, outputPath);
-          });
+          } else {
+            Notification.error("Unable to export video due to an error renaming files with confirm take.");
+          }
         }
       });
 

diff --git a/src/js/animator/projects/Take.js b/src/js/animator/projects/Take.js
@@ -2,17 +2,18 @@
   "use strict";
 
   // UI imports
-  var FrameReel = require("../core/FrameReel");
-  var Notification = require("../../common/Notification");
-  var OnionSkin = require("../ui/OnionSkin");
-  var PlaybackCanvas = require("../ui/PlaybackCanvas");
-  var StatusBar = require("../ui/StatusBar");
+  const FrameReel = require("../core/FrameReel");
+  const Loader = require("../core/Loader");
+  const Notification = require("../../common/Notification");
+  const OnionSkin = require("../ui/OnionSkin");
+  const PlaybackCanvas = require("../ui/PlaybackCanvas");
+  const StatusBar = require("../ui/StatusBar");
 
   // Common imports
-  var AudioManager = require("../core/AudioManager");
-  var File = require("../core/File");
+  const AudioManager = require("../core/AudioManager");
+  const File = require("../core/File");
 
-  var preview = document.querySelector("#preview");
+  const preview = document.querySelector("#preview");
 
   /** Represents a single take (image sequence). */
   class Take {
@@ -117,47 +118,64 @@
 
     /**
      * "Confirms" a take by renaming each captured frame to be sequential.
+     * @param {Boolean} notify Display a notification when the process completes/fails
+     * @returns {Boolean} Returns true if confirm is successful, false if there is an error
      */
-    confirmTake(notify = true) {
+    async confirmTake(notify = true) {
       let self = this;
+      let outputDir = this.saveDirPath;
+      let promisesList = [];
+      let response = null;
 
       // Return if no captured frames
       if (this.getTotalFrames() < 1) {
-        return;
+        return true;
       }
 
-      return new Promise((resolve, reject) => {
-        let outputDir = this.saveDirPath;
-
-        let promisesList = [];
+      try {
+        Loader.show("Confirming take");
 
+        // Give all of the files a temporary name
+        // (required because async renaming could cause naming conflicts otherwise)
         for (let i = 0; i < self.getTotalFrames(); i++) {
           let oldFilePath = self.exportedFramesPaths[i];
+          let newFilePath = oldFilePath.replace(/\.png$/,".tmp.png");
 
+          promisesList.push(File.renamePromise(oldFilePath, newFilePath));
+          self.exportedFramesPaths[i] = newFilePath;
+        }
+        await Promise.all(promisesList);
+
+        // Give the files their new name
+        promisesList = [];
+        for (let i = 0; i < self.getTotalFrames(); i++) {
+          let oldFilePath = self.exportedFramesPaths[i];
           let newFileName = this.buildFileName(Take.getPaddedFrameNumber(i+1));
           let newFilePath = `${outputDir}/${newFileName}`;
 
-          // Rename the file to the updated name
-          self.exportedFramesPaths[i] = newFilePath;
           promisesList.push(File.renamePromise(oldFilePath, newFilePath));
+          self.exportedFramesPaths[i] = newFilePath;
         }
+        await Promise.all(promisesList);
 
-        // Rename all of the files
-        Promise.all(promisesList).then(() => {
-          // Reset last export frame id
-          self.exportFrameId = self.getTotalFrames();
-          if (notify) {
-            Notification.success("Confirm take successfully completed");
-          }
-          resolve();
-        }).catch((err) => {
-          console.error(err);
-          if (notify) {
-            Notification.error("Error renaming file with confirm take");
-          }
-          reject(err);
-        });
-      });
+        // Reset last export frame id
+        self.exportFrameId = self.getTotalFrames();
+
+        if (notify) {
+          Notification.success("Confirm take successfully completed");
+        }
+        response = true;
+
+      } catch (err) {
+        if (notify) {
+          Notification.error("Error renaming file with confirm take");
+        }
+        response = false;
+
+      } finally {
+        Loader.hide();
+        return response;
+      }
     }
 
     /**