feat(mobile): Manual asset upload #3445

shenlong-tanwen · 2023-07-28T03:27:10Z

In this PR, I've added a basic support for #1684

Changes made in the PR

User can use the upload button from the Asset viewer to manually upload the single asset to the server or they can select at-most 30 assets from the timeline

DraggableScrollbarSheet in homepage is made dynamic to show actions related to the type of assets selected. If it contains only local assets, only the Share, Delete and Upload are displayed. If it contains both local and remote assets, favorite, add to album and other similar options are displayed.

Local Only Selection

Local + Remote Selection

While testing, It seemed extremely hard to trigger force refresh from the homepage. Going through the code, found that the current interval between two successive refresh to force it was set to 2 seconds. I've bumped it to 4 seconds to make it a little bit more easier to trigger force refresh if required.

How Has This Been Tested?

✅ Multi asset upload from homepage
✅ Single asset upload from asset view

Potential Improvements

Currently, Favorite and other options to be performed on Remote only assets from the bottom sheet in the homepage are ignoring the local assets with a toast stating that they are not supported. The user can be presented with a prompt to upload them before performing the required action.
Local Notification handling for Asset upload progress, failure and error instead of showing a Toast
Need to analyze the performance impact of the app because of the change to make the homepage bottom sheet dynamic. From my basic testing, it did not seem to impact much in Performance. But it would be best to test it out once with large number of asset selections.

vercel · 2023-07-28T03:27:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
immich	⬜️ Ignored (Inspect)	Visit Preview		Aug 6, 2023 2:37am

alextran1502 · 2023-07-28T04:38:48Z

Hello, what is the limitation of 30 uploads at once?

shenlong-tanwen · 2023-07-28T05:04:49Z

Hello, what is the limitation of 30 uploads at once?

It is a not a limitation of the stack but rather, a one set by us to easily handle cases such as:

App going into the background and thereby possibly killing off the upload in-between
Platform specific battery optimisation killing the app, etc.

I just felt it would be easier to handle such cases by asking the user to upload the assets in batches so that we can be sure that all the assets will be uploaded before the app is killed prematurely. Because with the current implementation, one can upload assets even without background / foreground sync enabled.

We can remove the hard limit, but we might need to improve the current implementation to make it resilient to above cases.

Even with the hard limit, it might be possible that the upload might be interrupted if a user is uploading a huge file and the app gets terminated somehow.

martyfuhry

Looks cool! I didn't pull it down to try it out, but I left some initial thoughts above. I'm not super sure what the use case for this is, since I use the auto backup feature. I suppose there could be some confusion about uploading images manually versus automatically with this feature, but I doubt it's a big deal.

mobile/lib/modules/backup/providers/backup.provider.dart

martyfuhry · 2023-07-28T13:02:32Z

mobile/lib/modules/home/ui/control_bottom_app_bar.dart

+      initialChildSize: hasRemote ? 0.30 : 0.15,
      minChildSize: 0.15,
-      maxChildSize: 0.57,
+      maxChildSize: hasRemote ? 0.57 : 0.15,


Why are these changed?

Those are for the DraggableScrollbarSheet related change where if all the selections are only local assets, we shouldn't let the user drag the bottom sheet and only present them with the options available for local only assets. Setting the initial and maximum values to the same seemed like an easier way to achieve it

mobile/lib/modules/home/views/home_page.dart

waclaw66 · 2023-08-03T06:45:21Z

Looks cool! I didn't pull it down to try it out, but I left some initial thoughts above. I'm not super sure what the use case for this is, since I use the auto backup feature. I suppose there could be some confusion about uploading images manually versus automatically with this feature, but I doubt it's a big deal.

I'm happy for this feature. I don't/wan't to use automatic backup, manual selective upload of photos is my use case.

alextran1502 · 2023-08-03T14:45:59Z

Hello, I've finished the first round of testing of this PR

We don't have an indication of upload progress when uploading from the asset detail view. It can be confusing to the user if they are on a slower network, and it takes some time to upload the asset. What do you think?
After uploading a video, the state isn't updated correctly. Therefore you cannot upload other assets unless going to the backup page and tap on the Cancel button at the bottom of the page.

shenlong-tanwen · 2023-08-03T15:58:09Z

We don't have an indication of upload progress when uploading from the asset detail view. It can be confusing to the user if they are on a slower network, and it takes some time to upload the asset. What do you think?

True. I'll check if we can reuse the notification handling used for background upload for manual uploads as well. It should be possible with a little refactoring.

…ntly

…bleSheet

This is to prevent the buttons in the bottom sheet getting clipped behind the 3 way navigation buttons in the default density of Android devices

shenlong-tanwen · 2023-08-05T11:47:19Z

Refactored the PR to handle upload progress update using local notifications. New dependency flutter_local_notifications is added to make it easier to handle platform specific local notifications.

The notification works the same way as the change made in this PR - #781 except that the progress notifications are displayed always for manual uploads. Detailed notifications are displayed only when the user explicitly enables it under Notification settings. However, if a user is trying to upload only a single asset, we will display detailed notification for the single asset so as to provide better visibility when a user uploads a single large asset file. If the notification permission is not granted, the user will not be notified of the upload progress nor the final status of the upload.

The older platform specific code for handling background service upload notification can be refactored to use this plugin in a separate PR.

The other issue with App State not updating properly when uploading a video asset was due to us trying to show a toast when the context is unmounted. Since most part of them are refactored to use local notifications now, that issue should be fixed as well.

waclaw66 · 2023-09-11T06:35:25Z

Could you please make the limit much higher or rather remove it completely?
I think it makes no different when the app is killed after 31 or 300 uploads.
Look at following usecase as I described above. I don't want to use automatic backup for many reasons. I want to upload daily shots (e.g. 300) in the evening. Now I have to select batch of 30 manually and let them upload and do the same 9times again. It's exhausting. When 31+ assets is selected to upload it refuses to start.
My proposal is to remove that limit completely or if you don't agree with my proposal upload first 30 of all selected (300) assets at least. Then I would need to select all day and click to upload 10times instead of manually selecting 300 images manually. It would be less painfull. Or couldn't be the list of selected assets handed to the automatic backup procedure?
Thanks in advance for considering the enhancement.

daniele-athome · 2023-09-11T07:07:48Z

I second that. Battery optimizations don't kick in if there is a foreground service notification (on Android, I don't know about iOS though).
Random app kills due to low resources could happen anyway, I don't think we can blame the app for that (OOM killer is called on rare, very extreme circumstances).

shenlong-tanwen · 2023-09-11T07:30:41Z

Could you please make the limit much higher or rather remove it completely? I think it makes no different when the app is killed after 31 or 300 uploads.

We can remove the limit for now and can re-add it in future if there are reports of the app getting killed. I'll handle this in the near future or you guys can feel free to open a PR to remove the hard limit. It is handled in the home_page.dart file

immich/mobile/lib/modules/home/views/home_page.dart

Lines 153 to 159 in 5d1011b

    
           if (assets.length > 30) { 
        
             ImmichToast.show( 
        
               context: context, 
        
               msg: 'home_page_upload_err_limit'.tr(), 
        
               gravity: ToastGravity.BOTTOM, 
        
             ); 
        
           } else {

banto6 · 2023-12-22T14:46:08Z

Great feature, but currently the interaction is rather strange, will it be optimized for interaction in the future? For example, add an extra button to each album or homepage to realize the upload function

shenlong-tanwen · 2023-12-25T06:41:37Z

Great feature, but currently the interaction is rather strange, will it be optimized for interaction in the future? For example, add an extra button to each album or homepage to realize the upload function

I am planning to make this a bit more easier, yes. Adding an option to the local album page sounds like a great feature. Can you raise that as a separate FR in discussions?

banto6 · 2023-12-25T13:31:46Z

Sure.

alextran1502 requested review from fyfrey and martyfuhry July 28, 2023 04:03

martyfuhry reviewed Jul 28, 2023

View reviewed changes

shenlong-tanwen marked this pull request as ready for review July 29, 2023 07:40

shenlong-tanwen added 10 commits August 5, 2023 17:01

fix: exclude albums filter in backup provider

d1b6918

refactor: Separate builder methods for Top Control App Bar buttons

a590661

fix: Show download button only for Remote only assets

72d516c

fix(mobile): Force Refresh duration is too low to trigger it consiste…

99ada1b

…ntly

feat(mobile): Make Buttons dynamic in Home Selection DraggableScrolla…

bd476c5

…bleSheet

feat(mobile): Manual Asset upload

f3f26dc

refactor(mobile): Replace _showToast with ImmichToast calls

7818406

refactor(mobile): home_page selectionAssetState handling

1bbded2

chore(mobile): min and initial size of DraggableScrollState increased

9c69cae

This is to prevent the buttons in the bottom sheet getting clipped behind the 3 way navigation buttons in the default density of Android devices

feat(mobile): notifications for manual upload progress

b0b5ba2

wording

79038e2

alextran1502 approved these changes Aug 6, 2023

View reviewed changes

alextran1502 enabled auto-merge (squash) August 6, 2023 02:39

alextran1502 merged commit deaf81e into immich-app:main Aug 6, 2023
21 checks passed

shenlong-tanwen deleted the feat/mobile-single-upload branch August 6, 2023 11:14

shenlong-tanwen mentioned this pull request Oct 7, 2023

Fix/mobile remove upload soft limit #4380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mobile): Manual asset upload #3445

feat(mobile): Manual asset upload #3445

shenlong-tanwen commented Jul 28, 2023

vercel bot commented Jul 28, 2023 •

edited

alextran1502 commented Jul 28, 2023

shenlong-tanwen commented Jul 28, 2023 •

edited

martyfuhry left a comment

martyfuhry Jul 28, 2023

shenlong-tanwen Jul 28, 2023

waclaw66 commented Aug 3, 2023

alextran1502 commented Aug 3, 2023

shenlong-tanwen commented Aug 3, 2023

shenlong-tanwen commented Aug 5, 2023

waclaw66 commented Sep 11, 2023 •

edited

daniele-athome commented Sep 11, 2023 •

edited

shenlong-tanwen commented Sep 11, 2023

banto6 commented Dec 22, 2023

shenlong-tanwen commented Dec 25, 2023 •

edited

banto6 commented Dec 25, 2023

feat(mobile): Manual asset upload #3445

feat(mobile): Manual asset upload #3445

Conversation

shenlong-tanwen commented Jul 28, 2023

Changes made in the PR

How Has This Been Tested?

Potential Improvements

vercel bot commented Jul 28, 2023 • edited

alextran1502 commented Jul 28, 2023

shenlong-tanwen commented Jul 28, 2023 • edited

martyfuhry left a comment

Choose a reason for hiding this comment

martyfuhry Jul 28, 2023

Choose a reason for hiding this comment

shenlong-tanwen Jul 28, 2023

Choose a reason for hiding this comment

waclaw66 commented Aug 3, 2023

alextran1502 commented Aug 3, 2023

shenlong-tanwen commented Aug 3, 2023

shenlong-tanwen commented Aug 5, 2023

waclaw66 commented Sep 11, 2023 • edited

daniele-athome commented Sep 11, 2023 • edited

shenlong-tanwen commented Sep 11, 2023

banto6 commented Dec 22, 2023

shenlong-tanwen commented Dec 25, 2023 • edited

banto6 commented Dec 25, 2023

vercel bot commented Jul 28, 2023 •

edited

shenlong-tanwen commented Jul 28, 2023 •

edited

waclaw66 commented Sep 11, 2023 •

edited

daniele-athome commented Sep 11, 2023 •

edited

shenlong-tanwen commented Dec 25, 2023 •

edited