From 2fc70c7a5b8effbbe88007b164a2137cb5ae50f1 Mon Sep 17 00:00:00 2001 From: Theo Gravity Date: Thu, 21 Mar 2024 20:26:07 -0700 Subject: [PATCH] Fix "Save as" downloads not completing --- CHANGELOG.md | 5 ++ src/ElectronDownloadManager.ts | 110 ++++++++++++++------------- test/ElectronDownloadManager.test.ts | 17 +++-- 3 files changed, 73 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40292f5..ca30061 100644 --- 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 index cb2e2b8..3eb33e6 100644 --- 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 => { 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 index 970aef7..6d122bf 100644 --- 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({