download: Show in-app notification on downlaoding a file. #1466
Conversation
| "show-download-success", | ||
| notificationTitle, | ||
| notificationBody, | ||
| ); |
There was a problem hiding this comment.
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.
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.
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.
So possibly we'd suppress the native notification when showing the in-app one.
There was a problem hiding this comment.
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.
Is there a way to check if the native notification actually sent?
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?
shubham-padia
force-pushed
the
show-feedback-widget
branch
from
a5ad948 to
55867de
Compare
| title: notificationTitle, | ||
| body: notificationBody, | ||
| silent: true, // We'll play our own sound - ding.ogg | ||
| }); |
There was a problem hiding this comment.
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.
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.
shubham-padia
force-pushed
the
show-feedback-widget
branch
from
55867de to
390933b
Compare
Fixes zulip#1371. We do not accept a filePath from the web app when recieving the event to show the file in a folder, since that could introduce securithy vulnerabilies. We instead keep the list of latest 50 downloaded files in memory and give each filePath a unique downloadId that can be passed around between the webapp and the desktop app.
shubham-padia
force-pushed
the
show-feedback-widget
branch
from
390933b to
a593cc1
Compare
|
Updated the PR to store the last 50 downloaded files in memory. I tested the oldest file removal logic by printing the downloadedFiles map in console. The PR shows both native and in-app notification right now since it is not possible to know when the user has disabled notifications from system-side. |
4 participants
Fixes #1371.
We do not accept a filePath from the web app when recieving the event to show the file in a folder, since that could introduce securithy vulnerabilies. We store the latest downloaded file path instead in a variable. Only one in-app notification is active at a time, so storing the latest file path is enough for out use case.
To test this PR, run a zulip server that runs the branch on zulip/zulip#36408 and add it to the zulip desktop app running on this PR. Upload a file and download it to test.
Screenshots and screen captures:
Platforms this PR was tested on:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: