Fix "Save as" downloads not completing

theogravity · Mar 22, 2024 · 2fc70c7 · 2fc70c7
1 parent 2fb2411
commit 2fc70c7
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 59 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,8 @@
+# 1.2.0 (2024-03-21)
+
+- Fixes a major issue where a download would not complete if using the save as dialog
+- Fixes internal static method `disableThrottle()` where it was not working / throwing
+
 # 1.1.1 (2024-03-21)
 
 - Fix issues around downloading smaller files where the download would complete before the progress / completed event was emitted

diff --git a/src/ElectronDownloadManager.ts b/src/ElectronDownloadManager.ts
@@ -218,32 +218,27 @@ export class ElectronDownloadManager implements IElectronDownloadManager {
    */
   protected onWillDownload(
     id: string,
-    { window, directory, overwrite, saveAsFilename, callbacks, saveDialogOptions, showBadge }: DownloadParams
+    { directory, overwrite, saveAsFilename, callbacks, saveDialogOptions, showBadge }: DownloadParams
   ) {
     return async (event: Event, item: DownloadItem, webContents: WebContents): Promise<void> => {
       item.pause()
-
-      const cancel = async () => {
-        item.cancel()
-        if (callbacks.onDownloadCancelled) {
-          this.log(`[${id}] Calling onDownloadCancelled`)
-
+      const start = async () => {
+        if (callbacks.onDownloadStarted) {
+          this.log(`[${id}] Calling onDownloadStarted`)
           try {
-            await callbacks.onDownloadCancelled({
-              id,
+            await callbacks.onDownloadStarted({
+              ...downloadData,
               item,
               event,
               webContents,
-              percentCompleted: 0,
-              resolvedFilename: item.getFilename(),
-              cancelledFromSaveAsDialog: true,
             })
           } catch (e) {
-            this.log(`[${id}] Error during onDownloadCancelled: ${e}`)
-            this.handleError(callbacks, e as Error, { id, item, event, webContents, cancelledFromSaveAsDialog: true })
+            this.log(`[${id}] Error during onDownloadStarted: ${e}`)
+            this.handleError(callbacks, e as Error, { id, item, event, webContents })
           }
         }
       }
+
       // Begin adapted code from
       // https://github.com/sindresorhus/electron-dl/blob/main/index.js#L73
 
@@ -265,24 +260,10 @@ export class ElectronDownloadManager implements IElectronDownloadManager {
 
       if (saveDialogOptions) {
         this.log(`[${id}] Prompting save as dialog`)
-        let result
-
-        try {
-          result = await dialog.showSaveDialog(window, { defaultPath: filePath, ...saveDialogOptions })
-        } catch (e) {
-          this.log(`[${id}] Error while showing save dialog: ${e}`)
-          this.handleError(callbacks, e as Error, { item, event, webContents })
-          await cancel()
-          return
-        }
-
-        if (result.canceled) {
-          this.log(`[${id}] User cancelled save as dialog`)
-          await cancel()
-          return
-        } else {
-          item.setSavePath(result.filePath!)
-        }
+        // This actually isn't what shows the save dialog
+        // If item.setSavePath() isn't called at all after some tiny period of time,
+        // then the save dialog will show up, and it will use the options we set it to here
+        item.setSaveDialogOptions({ ...saveDialogOptions, defaultPath: filePath })
       } else {
         this.log(`Setting save path to ${filePath}`)
         item.setSavePath(filePath)
@@ -293,7 +274,6 @@ export class ElectronDownloadManager implements IElectronDownloadManager {
       const resolvedFilename = path.basename(item.getSavePath()) || item.getFilename()
 
       this.log(`[${id}] Associating ${id} to ${resolvedFilename}`)
-      this.log(`[${id}] Initiating download item handlers`)
 
       const downloadData = {
         id,
@@ -305,21 +285,6 @@ export class ElectronDownloadManager implements IElectronDownloadManager {
 
       this.idToDownloadItems[id] = item
 
-      if (callbacks.onDownloadStarted) {
-        this.log(`[${id}] Calling onDownloadStarted`)
-        try {
-          await callbacks.onDownloadStarted({
-            ...downloadData,
-            item,
-            event,
-            webContents,
-          })
-        } catch (e) {
-          this.log(`[${id}] Error during onDownloadStarted: ${e}`)
-          this.handleError(callbacks, e as Error, { id, item, event, webContents })
-        }
-      }
-
       const handlerConfig = {
         id,
         event,
@@ -338,9 +303,52 @@ export class ElectronDownloadManager implements IElectronDownloadManager {
         updated: updatedHandler,
       })
 
-      item.on('updated', updatedHandler)
-      item.once('done', doneHandler)
-      item.resume()
+      if (saveDialogOptions) {
+        // Because the download happens concurrently as the user is choosing a save location
+        // we need to wait for the save location to be chosen before we can start to fire out events
+        // there's no good way to listen for this, so we need to poll
+        const interval = setInterval(async () => {
+          if (item.getSavePath()) {
+            this.log(`User selected save path to ${item.getSavePath()}`)
+            this.log(`[${id}] Initiating download item handlers`)
+            clearInterval(interval)
+            await start()
+            item.on('updated', updatedHandler)
+            item.once('done', doneHandler)
+            item.resume()
+          } else if (item.getState() === 'cancelled') {
+            clearInterval(interval)
+            this.log(`[${id}] Download was cancelled by user`)
+            if (callbacks.onDownloadCancelled) {
+              this.log(`[${id}] Calling onDownloadCancelled`)
+
+              try {
+                await callbacks.onDownloadCancelled({
+                  id,
+                  item,
+                  event,
+                  webContents,
+                  cancelledFromSaveAsDialog: true,
+                  percentCompleted: 0,
+                  resolvedFilename: '',
+                })
+              } catch (e) {
+                this.log(`[${id}] Error during onDownloadCancelled: ${e}`)
+                this.handleError(callbacks, e as Error, { item, event, webContents })
+              }
+            }
+          } else {
+            this.log(`[${id}] Waiting for save path to be chosen by user`)
+          }
+        }, 500)
+      } else {
+        this.log(`[${id}] Initiating download item handlers`)
+
+        await start()
+        item.on('updated', updatedHandler)
+        item.once('done', doneHandler)
+        item.resume()
+      }
     }
   }
 

diff --git a/test/ElectronDownloadManager.test.ts b/test/ElectronDownloadManager.test.ts
@@ -1,7 +1,7 @@
 // ElectronMultiDownloader.test.ts
 import { DownloadParams, DownloadManagerCallbacks, ElectronDownloadManager } from '../src'
 import EventEmitter from 'events'
-import { BrowserWindow, DownloadItem, dialog } from 'electron'
+import { BrowserWindow, DownloadItem } from 'electron'
 
 jest.mock('electron', () => {
   const originalModule = jest.requireActual('electron');
@@ -11,12 +11,6 @@ jest.mock('electron', () => {
     app: {
       getPath: jest.fn().mockReturnValue('/default/path'),
     },
-    dialog: {
-      showSaveDialog: jest.fn().mockReturnValue({
-        canceled: false,
-        filePath: '/test/path/test.txt'
-      }),
-    },
     BrowserWindow: jest.fn().mockImplementation(() => ({
       webContents: {
         downloadURL: jest.fn(),
@@ -80,6 +74,7 @@ describe('ElectronMultiDownloader', () => {
       cancel: jest.fn(),
       pause: jest.fn(),
       resume: jest.fn(),
+      setSaveDialogOptions: jest.fn(),
       getFilename: jest.fn().mockImplementation(() => {
         return downloadItemData.filename
       }),
@@ -130,7 +125,7 @@ describe('ElectronMultiDownloader', () => {
       }
     };
     instance.download(params);
-    expect(dialog.showSaveDialog).toHaveBeenCalled()
+    expect(downloadItem.setSaveDialogOptions).toHaveBeenCalled()
     expect(window.webContents.downloadURL).toHaveBeenCalledWith(params.url, undefined);
   });
 
@@ -209,6 +204,8 @@ describe('ElectronMultiDownloader', () => {
       const params: DownloadParams = { window, url: 'http://example.com/file.txt', callbacks, saveAsFilename: '/tmp/testFile.txt', };
 
       instance.download(params);
+      // Need this because of the await start() when setting up the handlers
+      await new Promise((r) => setTimeout(r, 0));
       emitter.emit('updated', null, 'progressing')
 
       expect(mockOnDownloadProgress).toHaveBeenCalledWith({
@@ -229,6 +226,8 @@ describe('ElectronMultiDownloader', () => {
       const params: DownloadParams = { window, url: 'http://example.com/file.txt', callbacks, saveAsFilename: '/tmp/testFile.txt', };
 
       instance.download(params);
+      // Need this because of the await start() when setting up the handlers
+      await new Promise((r) => setTimeout(r, 0));
       emitter.emit('done', null, 'completed')
 
       expect(mockOnDownloadCompleted).toHaveBeenCalledWith({
@@ -248,6 +247,8 @@ describe('ElectronMultiDownloader', () => {
       };
       const params: DownloadParams = { window, url: 'http://example.com/file.txt', callbacks, saveAsFilename: '/tmp/testFile.txt', };
       instance.download(params);
+      // Need this because of the await start() when setting up the handlers
+      await new Promise((r) => setTimeout(r, 0));
       emitter.emit('done', null, 'cancelled')
 
       expect(mockOnDownloadCancelled).toHaveBeenCalledWith({