fix(web): uploading directly to album spams notification feed

[Art051]

Fix for #6102

• Removed multiple invocations of notification.
• Added count to final notification which appears at the end of upload process.
• Retained the (in my opinion) more useful popup bottom right which displays individual item progress.

Used a few assets from here for the gif below.

Gif shows adding assets both during new gallery creation and adding assets to existing album.

[danieldietzler]

So now there isn't a notification for adding asset(s) to an album anymore, correct?

[Art051]

So now there isn't a notification for adding asset(s) to an album anymore, correct?

The gif in the original comment is what the result is.

So rather than a toast/notification popup for every file uploaded, you still get the progress being displayed (which was being covered previously bby the notifications. Then at the end the user is still presented with a single, and final, notification, which states the total processed.

PR has been updated to factor in single/multi asset upload count in the notification as mentioned previously.

[zackpollard]

Hey, thanks for the PR, one comment as this doesn't seem to align with the intended fix we had for this problem as Daniel mentioned previously :)

[zackpollard]

As Daniel mentioned, this seems to remove the current functionality where we send a notification when assets already existing on the server are added to an album (not uploaded to an album). We would like to retain that notification but suppress (or group it) during the upload case.

[Art051]

Ah I get you, sorry I thought he was referring to uploading, apologies. Will rectify this evening.

[Art051]

I've just double checked, and the cummulative total assets notification shown in my gif appears for me when adding pre-existing assets to an album. It appears both when adding them during creation of an album, and when added after the album has been created.

[danieldietzler]

Wtf?

[Art051]

?

[zackpollard]

I've just double checked, and the cummulative total assets notification shown in my gif appears for me when adding pre-existing assets to an album. It appears both when adding them during creation of an album, and when added after the album has been created.

It seems this is going through some other mechanism then, hmmm. The messaging is not the same as the message being removed or the new one that was added.

[Art051]

It's here:

immich/web/src/routes/(user)/albums/[albumId=id]/[[photos=photos]]/[[assetId=id]]/+page.svelte

Line 294 in 540e568

message: `Added ${count} asset${count === 1 ? '' : 's'}`,

[Art051]

I think probably needs refactoring wherever the notifications are used so that the Notification[] is at most 3 items long, with each of the NotificationType enums being one of the 3 indexes in the array.

Then have it display the count of each NotificationType in their own notification.

if that makes sense?

[jrasm91]

You can drag and drop directly into the album and that should trigger it.

[Art051]

Yes, that's what's shown in the GIFs.

[zackpollard]

@Art051 Doing some testing this does now seem to only produce a single notification for each case, but the behavior is a little weird. When dragging assets to be uploaded into the album, it'll say "Added x assets to album" before they are even uploaded, followed by a "Uploaded 4 assets successfully, refresh the page to see new upload assets." The other thing is that we specify the need to reload, but that doesn't actually seem necessary in the cases I tested.

[Art051]

Yeh to be honest the functions are quite large and there's lots going on in them that I didn't want to change too much and split up. It seems to have been originally designed for one approach and retrospectively taken another.

I'll close this PR and circle back around to it so if someone more familiar with the codebase wants to have a go they can, without feeling they're overstepping/jumping in.

[alextran1502]

We will need to use throttle function from lodash library I think

[zackpollard]

Yeh to be honest the functions are quite large and there's lots going on in them that I didn't want to change too much and split up. It seems to have been originally designed for one approach and retrospectively taken another.

I'll close this PR and circle back around to it so if someone more familiar with the codebase wants to have a go they can, without feeling they're overstepping/jumping in.

Alright no worries, don't want you to feel like your attempts aren't appreciated, as you say it's a little complicated to unpick this to be completely correct, just discussing if we want to take these changes in regardless as the outcome is better than what we have now, might just need a further rework later

[jrasm91]

I've actually looked at this. The problem like @Art051 mentioned is that there upload and add to album utility functions in asset-utils are all intertwined in a way that makes it hard to do what we want to do, which is show a single notification after the upload queue is done. What should happen, is all those methods get cleaned up and refactored so they are more usable to begin with.

[Art051]

Yeh to be honest the functions are quite large and there's lots going on in them that I didn't want to change too much and split up. It seems to have been originally designed for one approach and retrospectively taken another.

I'll close this PR and circle back around to it so if someone more familiar with the codebase wants to have a go they can, without feeling they're overstepping/jumping in.

Alright no worries, don't want you to feel like your attempts aren't appreciated, as you say it's a little complicated to unpick this to be completely correct, just discussing if we want to take these changes in regardless as the outcome is better than what we have now, might just need a further rework later

Cheers, yeh I just picked it up because I'd encountered it a few weeks back. It's perfectly doable with some decent refactoring of existing code. But I don't want to go shifting stuff all over the place on a repo I'm not overly familiar with without some time spent learning a bit more of it.