-
-
Notifications
You must be signed in to change notification settings - Fork 485
download: Show in-app notification on downlaoding a file. #1466
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import { | |
| type WebContents, | ||
| app, | ||
| } from "electron/main"; | ||
| import {randomBytes} from "node:crypto"; | ||
| import fs from "node:fs"; | ||
| import path from "node:path"; | ||
|
|
||
|
|
@@ -15,6 +16,35 @@ import * as t from "../common/translation-util.ts"; | |
|
|
||
| import {send} from "./typed-ipc-main.ts"; | ||
|
|
||
| const maxTrackedDownloads = 50; | ||
|
|
||
| type DownloadedFile = { | ||
| id: string; | ||
| filePath: string; | ||
| }; | ||
|
|
||
| const downloadedFiles = new Map<string, DownloadedFile>(); | ||
|
|
||
| function trackDownloadedFile(filePath: string): DownloadedFile { | ||
| const downloadedFile: DownloadedFile = { | ||
| id: randomBytes(16).toString("hex"), | ||
| filePath, | ||
| }; | ||
|
|
||
| downloadedFiles.set(downloadedFile.id, downloadedFile); | ||
|
|
||
| if (downloadedFiles.size > maxTrackedDownloads) { | ||
| const oldestDownloadedFileId = [...downloadedFiles.keys()][0]; | ||
| downloadedFiles.delete(oldestDownloadedFileId); | ||
| } | ||
|
|
||
| return downloadedFile; | ||
| } | ||
|
|
||
| export function getDownloadedFilePath(downloadId: string): string | undefined { | ||
| return downloadedFiles.get(downloadId)?.filePath; | ||
| } | ||
|
|
||
| function isUploadsUrl(server: string, url: URL): boolean { | ||
| return url.origin === server && url.pathname.startsWith("/user_uploads/"); | ||
| } | ||
|
|
@@ -125,16 +155,31 @@ export default function handleExternalLink( | |
| url: url.href, | ||
| downloadPath, | ||
| async completed(filePath: string, fileName: string) { | ||
| const notificationTitle = t.__("Downloaded {{{fileName}}}.", { | ||
| fileName, | ||
| }); | ||
| const notificationBody = t.__("Click to open downloads folder."); | ||
| // Show native notification | ||
| const downloadNotification = new Notification({ | ||
| title: t.__("Download Complete"), | ||
| body: t.__("Click to show {{{fileName}}} in folder", {fileName}), | ||
| title: notificationTitle, | ||
| body: notificationBody, | ||
| silent: true, // We'll play our own sound - ding.ogg | ||
| }); | ||
| downloadNotification.on("click", () => { | ||
| // Reveal file in download folder | ||
| shell.showItemInFolder(filePath); | ||
| }); | ||
| downloadNotification.show(); | ||
| const {id: downloadId} = trackDownloadedFile(filePath); | ||
| // Event to show in-app notification in addition to the native | ||
| // notification. | ||
| send( | ||
| contents, | ||
| "show-download-success", | ||
| notificationTitle, | ||
| notificationBody, | ||
| downloadId, | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to do both the old notification mechanism and the new one -- is that intendedd? It is important for backwards-compatibility to make sure we do the old thing on existing web app versions. But I don't know if doing both would be problematic, and in any case, I would expect some sort of detection logic and comments explaining the reasoning behind what we're doing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is intended. My understanding of the requirements is that we want to show both in-app and native notifications. @alya let me know if that is not the case. Added comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, I was imagining we'd have both, but I'm not sure if it's silly to do it that way. If you'd be up for it, it might be good to check how a few other apps handle it. Trying Discord, Slack, and WhatsApp, it seems like they pick one or the other, unless my desktop notifications aren't working properly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So possibly we'd suppress the native notification when showing the in-app one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we don't want to send both. There are some potential advantages to the native notification; It appears even if your desktop app is not focused -- say if you started downloading a long video and then tabbed away. Is there a way to check if the native notification actually sent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I checked this and there's not for all OS. Only windows emits a failed event: https://www.electronjs.org/docs/latest/api/notification#event-failed-windows. Might be ok having both notifications in that case? |
||
|
|
||
| // Play sound to indicate download complete | ||
| if (!ConfigUtil.getConfigItem("silent", false)) { | ||
|
|
@@ -150,7 +195,7 @@ export default function handleExternalLink( | |
| if (state !== "cancelled") { | ||
| if (ConfigUtil.getConfigItem("promptDownload", false)) { | ||
| new Notification({ | ||
| title: t.__("Download Complete"), | ||
| title: t.__("Download complete"), | ||
| body: t.__("Download failed"), | ||
| }).show(); | ||
| } else { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the original issue is a complaint about the native notifications not working on Windows. Do we understand how that's happening? I feel like papering over a bug like that without understanding it is risky -- have you tried arranging an interactive debugging session with someone experiencing the bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user didn't confirm whether they had desktop notifications enabled or not.

The original intention was to solve #desktop > several clicks to download a file, I can unlink the issue from the PR if we think it's a windows bug (which I highly doubt). Reply from the issue poster felt incoherent.