Check filesize before uploading file to server and display alert if file is too big #7778

NicolasBuquet · 2024-04-17T17:55:09Z

Check filesize before uploading file to server and display alert if file is too big

This test is present in the Android version of Element, but curiously not in the iOS version.

Pull Request Checklist

I read the contributing guide
UI change has been tested on both light and dark themes, in portrait and landscape orientations and on iPhone and iPad simulators
Accessibility has been taken into account.

Pull request is based on the develop branch

Pull request contains a changelog file in ./changelog.d
You've made a self review of your PR
Pull request includes screenshots or videos of UI changes
Pull request includes a sign off

NicolasBuquet · 2024-04-17T17:56:13Z

@pixlwave
Can you have a look at this PR ?

pixlwave

Thanks for this PR, it's a nice idea. There's a bit more work needed before we can merge it, comments inline.

pixlwave · 2024-05-07T15:59:40Z

Riot/Modules/Room/RoomViewController.m

+        [self showAlertWithTitle:@"The file you want to upload is too big."
+                         message:[NSString stringWithFormat:@"\nThe file you want to upload is too big.\n\nIt mustn't weight more than %ldMB", maxUploadFileSize/(1024*1024)]];


A few initial thoughts:

These strings should be added to Vector.strings rather than hard coded.

We should use NSByteCountFormatter for file sizes so it is formatted properly for all locales and follows the system behaviour of reporting 1000 bytes = 1MB.

The message repeats the title. Something simple like File too large and Maximum supported file size: 100MB would probably be better here.

pixlwave · 2024-05-07T16:01:30Z

Riot/Modules/Room/RoomViewController.m

+    // Check maxUploadSize accepted by the home server before trying to upload.
+    NSUInteger maxUploadFileSize = self.roomDataSource.mxSession.maxUploadSize;
+    NSDictionary *fileAttributes = [NSFileManager.defaultManager attributesOfItemAtPath:url.path error:nil];
+    if (fileAttributes && fileAttributes.fileSize > maxUploadFileSize) {


Have you checked if this takes any encryption overhead into account? Might be worth building a bit of leniency into here.

pixlwave · 2024-05-07T16:07:35Z

Riot/Modules/Room/RoomViewController.m

+    if (fileAttributes && fileAttributes.fileSize > maxUploadFileSize) {
+        [self showAlertWithTitle:@"The file you want to upload is too big."
+                         message:[NSString stringWithFormat:@"\nThe file you want to upload is too big.\n\nIt mustn't weight more than %ldMB", maxUploadFileSize/(1024*1024)]];
+        return;


This whole check is in the wrong place, it doesn't account for videos and images to be processed, meaning something that would previously send, will now be rejected. It should either happen after the respective processing has taken place, or only happen for files inside the else if (fileUTI.isFile).

changelog.d/check_filesize_before_upload.change

…ile is too big. Signed-off-by: Nicolas Buquet <[email protected]>

codecov · 2024-05-15T15:36:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.32%. Comparing base (a5d4d1c) to head (f16203a).
Report is 41 commits behind head on develop.

❗ Current head f16203a differs from pull request most recent head 90f1122

Please upload reports for the commit 90f1122 to get more accurate results.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #7778      +/-   ##
===========================================
+ Coverage    12.27%   12.32%   +0.05%     
===========================================
  Files         1651     1651              
  Lines       164167   163958     -209     
  Branches     67499    67364     -135     
===========================================
+ Hits         20148    20209      +61     
+ Misses      143346   143112     -234     
+ Partials       673      637      -36

Flag	Coverage Δ
uitests	`55.67% <ø> (+0.65%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pixlwave

Thanks for the updates, some more comments added inline.

pixlwave · 2024-06-06T13:59:17Z

Riot/Modules/MatrixKit/Models/Room/MXKRoomDataSource.m

+     if( ![self isFilesizeOkToBeSentForLocalAVAsset:videoAsset] )
+     {
+         failure([NSError errorWithDomain:MXKRoomDataSourceErrorDomain code:MXKRoomDataSourceErrorCantSendFileToBig userInfo:nil]);
+         return;
+     }
+
    __block MXEvent *localEchoEvent = nil;

    [_room sendVideoAsset:videoAsset withThumbnail:videoThumbnail threadId:self.threadId localEcho:&localEchoEvent success:success failure:failure];


Hi @NicolasBuquet, thanks for the update. To my eyes this one still appears to be too early: MXRoom.sendVideoAsset will convert the video meaning that the check above would block some files from sending that would have been fine.

pixlwave · 2024-06-06T14:01:05Z

Riot/Modules/MatrixKit/Models/Room/MXKRoomDataSource.m

+    if (filesize > maxUploadFileSize)
+    {
+        return NO;
+    }


Same question again here:

Have you checked if this takes any encryption overhead into account? Might be worth building a bit of leniency into here.

pixlwave · 2024-06-06T14:02:28Z

Riot/Modules/MatrixKit/Models/Room/MXKRoomDataSource.m

+- (BOOL)isFilesizeOkToBeSentForLocalAVAsset:(AVAsset *)asset
+{
+    // Check if asset points to a local file
+    if( ![asset isKindOfClass:AVURLAsset.class] )


Our objective-c style should look like so:

Suggested change

if( ![asset isKindOfClass:AVURLAsset.class] )

if (![asset isKindOfClass:AVURLAsset.class])

There's a few of these in here.

pixlwave · 2024-06-06T14:08:44Z

Riot/Assets/en.lproj/Vector.strings

@@ -1715,6 +1715,8 @@ Tap the + to start adding people.";
 // MARK: File upload
 "file_upload_error_title" = "File upload";
 "file_upload_error_unsupported_file_type_message" = "File type not supported.";
+"file_upload_error_too_large_title" = "File too large";
+"file_upload_error_too_large_message" = "Maximum supported file size is %@MB";


Have you checked what this looks like? Now that there's an NSByteCountFormatter feeding the alert, it should already contain the appropriate unit formatted appropriately in the string.

pixlwave · 2024-06-06T14:08:51Z

Riot/Assets/fr.lproj/Vector.strings

We can't make changes to the translations directly in the project - these should be added on https://translate.element.io once the PR is merged, otherwise we run the risk of handling conflicts.

pixlwave · 2024-07-17T12:12:44Z

@NicolasBuquet Also, are you planning on coming back to this PR too?

NicolasBuquet · 2024-08-14T09:15:38Z

@NicolasBuquet Also, are you planning on coming back to this PR too?

Hello @pixlwave No, I don't push anymore to this PR.

Too complicated. IMO, it needs changes in matrix-ios-sdk to be properly implemented. Thank you for your involvement.

pixlwave · 2024-08-14T09:26:16Z

Ok, thank you for your time looking into this anyway :)

NicolasBuquet mentioned this pull request Apr 17, 2024

Afficher un message d'alerte avant envoi d'une pièce jointe trop lourde tchapgouv/tchap-ios#1016

Merged

pixlwave reviewed May 7, 2024

View reviewed changes

Check filesize before uploading file to server and display alert if f…

d24c084

…ile is too big. Signed-off-by: Nicolas Buquet <[email protected]>

NicolasBuquet force-pushed the develop branch from 3d4409f to d24c084 Compare May 8, 2024 09:46

Move hard-coded strings to strings files (only EN and FR)

f16203a

NicolasBuquet requested a review from pixlwave June 5, 2024 12:25

Check filesize limit before sending Image, Video or File

90f1122

NicolasBuquet force-pushed the develop branch from 6c7c555 to 90f1122 Compare June 5, 2024 12:29

pixlwave approved these changes Jun 6, 2024

View reviewed changes

pixlwave closed this Aug 14, 2024

This was referenced Aug 14, 2024

Check power level before starting live sharing location #7832

Merged

Check power level before starting live sharing location #7808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check filesize before uploading file to server and display alert if file is too big #7778

Check filesize before uploading file to server and display alert if file is too big #7778

NicolasBuquet commented Apr 17, 2024 •

edited

Loading

NicolasBuquet commented Apr 17, 2024

pixlwave left a comment

pixlwave May 7, 2024 •

edited

Loading

pixlwave May 7, 2024 •

edited

Loading

pixlwave May 7, 2024

codecov bot commented May 15, 2024 •

edited

Loading

pixlwave left a comment

pixlwave Jun 6, 2024

pixlwave Jun 6, 2024

pixlwave Jun 6, 2024

pixlwave Jun 6, 2024

pixlwave Jun 6, 2024

pixlwave commented Jul 17, 2024

NicolasBuquet commented Aug 14, 2024 •

edited

Loading

pixlwave commented Aug 14, 2024

		[self showAlertWithTitle:@"The file you want to upload is too big."
		message:[NSString stringWithFormat:@"\nThe file you want to upload is too big.\n\nIt mustn't weight more than %ldMB", maxUploadFileSize/(1024*1024)]];

	if( ![asset isKindOfClass:AVURLAsset.class] )
	if (![asset isKindOfClass:AVURLAsset.class])

Check filesize before uploading file to server and display alert if file is too big #7778

Check filesize before uploading file to server and display alert if file is too big #7778

Conversation

NicolasBuquet commented Apr 17, 2024 • edited Loading

Pull Request Checklist

NicolasBuquet commented Apr 17, 2024

pixlwave left a comment

Choose a reason for hiding this comment

pixlwave May 7, 2024 • edited Loading

Choose a reason for hiding this comment

pixlwave May 7, 2024 • edited Loading

Choose a reason for hiding this comment

pixlwave May 7, 2024

Choose a reason for hiding this comment

codecov bot commented May 15, 2024 • edited Loading

Codecov Report

pixlwave left a comment

Choose a reason for hiding this comment

pixlwave Jun 6, 2024

Choose a reason for hiding this comment

pixlwave Jun 6, 2024

Choose a reason for hiding this comment

pixlwave Jun 6, 2024

Choose a reason for hiding this comment

pixlwave Jun 6, 2024

Choose a reason for hiding this comment

pixlwave Jun 6, 2024

Choose a reason for hiding this comment

pixlwave commented Jul 17, 2024

NicolasBuquet commented Aug 14, 2024 • edited Loading

pixlwave commented Aug 14, 2024

NicolasBuquet commented Apr 17, 2024 •

edited

Loading

pixlwave May 7, 2024 •

edited

Loading

pixlwave May 7, 2024 •

edited

Loading

codecov bot commented May 15, 2024 •

edited

Loading

NicolasBuquet commented Aug 14, 2024 •

edited

Loading