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

feat(web): include timestamp in download filename #5878

Merged
merged 14 commits into from
Jan 25, 2024

Conversation

MohamedFBoussaid
Copy link
Contributor

@MohamedFBoussaid MohamedFBoussaid commented Dec 20, 2023

This is a suggestion to fix #5874.

~~ The current implementation block any multiple download for the same time (asset or archive), until the current download is done~~
My PR will block any multiple download for the same time (asset or archive), until the current download is done.

User can download an archive and an asset at the same time, but never the same kind of download at the same time.

This is can be improved if needed and you think is better, to block the download only based on file name. So users can download as many files as they want in parallel, as long they are not the same files.

Here is how the current changes will looks like :

https://github.com/immich-app/immich/assets/52571029/0875ff50-4dd7-4416-a232-a09a7f26a1e6

This PR will :

  1. Block multiple download for the same files while the initial download is not yet done.

For example :

  • if the user try to download file1, and before it is done a new download for file1 is requested this will be blocked
  • if the user try to download file1, and before it is done a new download for file1 and file is requested this will NOT be blocked

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we accept multiple download requests for the same type?

@MohamedFBoussaid
Copy link
Contributor Author

Why can't we accept multiple download requests for the same type?

To avoid this behaviour : #5874
And also most of the time when a user click again the download for the same file while the download is not yet done, is by mistake (double click).

@MohamedFBoussaid
Copy link
Contributor Author

Why can't we accept multiple download requests for the same type?

I can make it block based on the file name and not the download type

@MohamedFBoussaid
Copy link
Contributor Author

How it will behave with 1 file

1.file.mov

@MohamedFBoussaid
Copy link
Contributor Author

MohamedFBoussaid commented Dec 20, 2023

How it will behave with multiple files:

Multiple.files.mov

@MohamedFBoussaid
Copy link
Contributor Author

How it will behave with Album Download:

Album.download.mov

@MohamedFBoussaid
Copy link
Contributor Author

NOTE:
The only place where this will not behave as expected by this PR, is in the album when selecting multiple assets, it will always download with the same zip file name. I am trying to understand the code to propose an alternative way of naming.

@MohamedFBoussaid
Copy link
Contributor Author

NOTE: The only place where this will not behave as expected by this PR, is in the album when selecting multiple assets, it will always download with the same zip file name. I am trying to understand the code to propose an alternative way of naming.

This is now covered :

Screen.Recording.2023-12-20.at.18.10.02.mov

@alextran1502 alextran1502 marked this pull request as draft December 21, 2023 15:30
@grishy
Copy link

grishy commented Dec 23, 2023

Maybe as an option there is make save interface like for uploading?
You can upload a batch of photos and you can see them all + status above.

@MohamedFBoussaid
Copy link
Contributor Author

Hey @alextran1502 you have set this PR as draft. I am not planning to do any extra change. Shall it pass to ready for review ?
Thanks.

@jrasm91
Copy link
Contributor

jrasm91 commented Jan 24, 2024

This is trying to solve/fix some of the usability issues around file downloads, but we should probably try to address them directly.

  1. It's too easy to click the download button twice
    We can make the download button temporarily disabled (2-3 seconds) while the download starts

  2. Download names aren't unique enough
    We can make the download filename template use something with a timestamp by default

  3. No user feedback after clicking "Download"
    We can add an info notification in the top right, that the download has started.

@MohamedFBoussaid
Copy link
Contributor Author

This is trying to solve/fix some of the usability issues around file downloads, but we should probably try to address them directly.

Thanks @jrasm91 for your comment.

  1. It's too easy to click the download button twice
    We can make the download button temporarily disabled (2-3 seconds) while the download starts

In which scenario do you think a user will trigger download twice for the same exact files ? (Before the current download is done)

  1. Download names aren't unique enough
    We can make the download filename template use something with a timestamp by default

The change that I am proposing is adding date to the download for example: DownloadSuffix-2024-01-24-18-42-59.zip

  1. No user feedback after clicking "Download"
    We can add an info notification in the top right, that the download has started.

The current change will block downloading twice the same files while the download is not done, and if the user try to download s(he) will get a warning to let her/him know: Please wait, the selected assets are currently being downloaded. You can start another download once this is complete.

@jrasm91
Copy link
Contributor

jrasm91 commented Jan 24, 2024

In general, I'm for (1) improving user experience and (2) doing so with minimal code. I am good with the default archive name change as this helps with (1). I don't like the idea of showing a duplicate download message at all. It's an arbitrary limitation. So what if the user wants to start a second download immediately? If the user did not intend to download it again then you have a usability issue that should be fixed. Assuming it is fixed, the need for code to track duplicate downloads just becomes a maintenance burden or something that frustrates the user which has a legitimate reason for clicking it a second time.

@MohamedFBoussaid
Copy link
Contributor Author

In general, I'm for (1) improving user experience and (2) doing so with minimal code. I am good with the default archive name change as this helps with (1). I don't like the idea of showing a duplicate download message at all. It's an arbitrary limitation. So what if the user wants to start a second download immediately? If the user did not intend to download it again then you have a usability issue that should be fixed. Assuming it is fixed, the need for code to track duplicate downloads just becomes a maintenance burden or something that frustrates the user which has a legitimate reason for clicking it a second time.

Hey @jrasm91,
I see you point, I will update the PR to handle just the archive name.

@MohamedFBoussaid
Copy link
Contributor Author

@jrasm91, I have updated the PR.
But the Mobile tests are failing, after rebasing my branch from main.

@MohamedFBoussaid MohamedFBoussaid marked this pull request as ready for review January 25, 2024 12:40
@jrasm91
Copy link
Contributor

jrasm91 commented Jan 25, 2024

The mobile tests have been a bit flaky lately so don't worry about that one.

@jrasm91
Copy link
Contributor

jrasm91 commented Jan 25, 2024

What do you think of adding a notification when the user clicks download?

@jrasm91 jrasm91 changed the title fix(web): Blocking multiple downloads feat(web): include timestamp in download filename Jan 25, 2024
@jrasm91 jrasm91 merged commit 4eca2b0 into immich-app:main Jan 25, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Download notification with multiple download
4 participants