Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request]: "blocking" version of setSaveDialogOptions #41640

Open
3 tasks done
theogravity opened this issue Mar 20, 2024 · 6 comments
Open
3 tasks done

[Feature Request]: "blocking" version of setSaveDialogOptions #41640

theogravity opened this issue Mar 20, 2024 · 6 comments

Comments

@theogravity
Copy link
Contributor

theogravity commented Mar 20, 2024

Preflight Checklist

Problem Description

I'm writing a file downloader which makes use of DownloadItem.setSaveDialogOptions(), and the behavior around it is unintuitive.

  • The download / event handlers start during the dialog, when I would expect it to trigger only after the user has confirmed / canceled the dialog
  • item.getFilename() doesn't update with the filename the user has selected from the dialog
    • item.getSavePath() will however
  • No way of knowing when the dialog action has completed

Proposed Solution

This might be difficult due to how setSaveDialogOptions() is coupled to the DownloadItem, but I'd like to see the following:

  • A "blocking" version of setSaveDialogOptions() - asyncSetSaveDialogOptions()?
  • When this version is used, the download is "paused" (since it seems the nature of DownloadItem is to just immediately download when created), and "resumed" if the user decides to save the file; DownloadItem.cancel() is called if the user cancels the dialog. The value returned from asyncSetSaveDialogOptions() would be a boolean to indicate if the user has confirmed or denied save.

Alternatives Considered

Edit: The below does not work. See comments on why and an actual fix

Discovered a workaround: don't use setSaveDialogOptions() at all, but dialog.showSaveDialog() instead:

// item from will-download handler
    item.pause()
    const result = await dialog.showSaveDialog(window, { defaultPath: filePath, ...saveDialogOptions })

    if (result.canceled) {
      item.cancel()
      return
    } else {
      item.setSavePath(result.filePath!)
      item.resume()
    }

Additional Information

No response

@theogravity
Copy link
Contributor Author

theogravity commented Mar 21, 2024

For those who stumbled across this issue, I built a library that resolves a lot of issues that electron-dl has (including this one):

https://github.com/theogravity/electron-dl-manager

I use the alternative technique I described in the ticket.

@theogravity
Copy link
Contributor Author

@codebytere Can we ban the above poster? It's the second time I've seen this person comment in my issue tickets with junk. It seems to be generated from AI.

@theogravity

This comment was marked as off-topic.

@electron electron deleted a comment from Parv-gugnani Mar 21, 2024
@VerteDinde
Copy link
Member

@theogravity Banned - thanks for the call out, and sorry for the trouble 🙇‍♀️

@theogravity
Copy link
Contributor Author

theogravity commented Mar 22, 2024

Update: My workaround didn't actually work, and part of it was due to my lack of knowledge at the time around the lifecycle of a download in Electron.

The missing part in the story is: setSaveDialogOptions doesn't actually bring up the save dialog. If you read the description for downloadItem.setSavePath(path), it mentions the following:

If user doesn't set the save path via the API, Electron will use the original routine to determine the save path; this usually prompts a save dialog.

What this means is if you did not manually do a DownloadItem.setSavePath(), then it will eventually trigger a save as dialog using the setSaveDialogOptions() given. Eventually is pretty fast, within the same tick from what I've seen.

So what happens if you use the workaround I describe above where you use dialog. showSaveDialog() and then try to use DownloadItem. setSavePath() after? The download will never go into a completed state (as in, the DownloadItem.on('done') event never fires). I believe this is because the save dialog from dialog "overrides" the one that is attached to DownloadItem.

Because DownloadItem is expecting a file path from its own dialog, the download stalls as the routine to grab that file path never happens since that dialog never shows up (and you can't set DownloadItem.setSavePath() to "complete" the download in this situation either).

Summary:

In a scenario where you are using setSaveDialogOptions():

  • The download process is separate from the file selection process
  • setSaveDialogOptions() is not what triggers a dialog, but defining it will tell Electron to open a dialog if DownloadItem.setSavePath() isn't used in some tiny amount of time
    • You can't use setSaveDialogOptions and setSavePath() together. Although setSavePath() will save the path internally, it won't actually get used by the download process in the end in a setSaveDialogOptions scenario
  • The download process will only trigger the done event when the user has finished the file selection process where the result from the save as dialog is internally passed to DownloadItem.setSavePath()

So we must use DownloadItem.setSaveDialogOptions() if we want to save a download using a dialog.

The solution

I have posted a new version of my library that no longer uses the above suggestion, but a different one instead:

  // item from will-download handler
  // Pause the download because if we don't pause and the download finishes before the user
  // makes their selection, events won't get fired in a save as dialog situation
  item.pause();

  if (saveDialogOptions) {
    // we'll eventually show a save as dialog
    item.setSaveDialogOptions(window, {
      defaultPath: filePath,
      ...saveDialogOptions,
    });
  } else {
    // do not use a save as dialog, just save immediately after download to the specified path
    item.setSavePath("");
  }

  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 () => {
      // Check if the user has specified a save location
      if (item.getSavePath()) {
        // Save location specified, now register events and resume the download
        clearInterval(interval);
        item.on("updated", updatedHandler);
        item.once("done", doneHandler);
        item.resume();
      } else if (item.getState() === "cancelled") {
        clearInterval(interval);
        // Your cancel dialog logic
      }
    }, 500);
  } else {
    // Not showing dialog, execute immediately
    item.on("updated", updatedHandler);
    item.once("done", doneHandler);
    item.resume();
  }

@theogravity
Copy link
Contributor Author

That being said, it's clear that the workflow for handling downloads when you want to use a save as dialog is really clunky. It definitely needs some kind of overhaul.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@theogravity @VerteDinde and others