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

Media size selection #4721

Merged
merged 19 commits into from Sep 7, 2021
Merged

Media size selection #4721

merged 19 commits into from Sep 7, 2021

Conversation

pixlwave
Copy link
Contributor

@pixlwave pixlwave commented Aug 17, 2021

Depends on matrix-org/matrix-ios-kit#880.

The video prompt shows as

Small: 480p
Medium: 720p
Large: 1080p

One caveat here, introduced by matrix-org/matrix-ios-sdk#1145 is sending large videos to encrypted rooms from the share extension. As the videos are larger now, the 120MB memory limit can be hit when encrypting the video. A few potential solutions would be:

  • Limit the resolution that can be sent from the share extension.
  • Artificially lower the maxUploadSize in the share extension.
  • Add a notice when sending a video that larger videos may fail and to use the app instead.

IMG_0077

IMG_B8C12CF2A6BB-1

@github-actions
Copy link

github-actions bot commented Aug 17, 2021

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/7aGANA

@pixlwave
Copy link
Contributor Author

Following feedback, the separate image and video settings have been combined into a single toggle for media:

IMG_573D68B1CBE0-1

@ShadowJonathan
Copy link
Contributor

This does not present approximate sizes, is that intended?

Copy link

@niquewoodhouse niquewoodhouse left a comment

Choose a reason for hiding this comment

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

Some (I think) small changes:

Settings

  • Change Media title to Sending images and videos (just media could be interpreted to mean media in timelines that you haven't sent)
  • Change confirm media size before sending to Confirm size when sending
  • Add supporting copy underneath that says When this is on, you’ll be asked to confirm what size images and videos will be sent as.. This helps reassure and set expectations.

Settings - confirm@2x

Image

  • Change intro copy. Confirm size to send. You can turn this off in settings. This way we inform people they can turn this off somehow.
  • Change styling of options, with actual sizes in brackets. I looked at Mail on iOS and wanted to use the same formatting, on the assumption it's potentially familiar.

image@2x

Video

  • Change intro copy
  • Include size in details, as people using these options are mostly concerned about bandwidth

video@2x

@pixlwave
Copy link
Contributor Author

pixlwave commented Aug 27, 2021

@niquewoodhouse I've used a title and message for the confirmation sheets as it looks a bit cleaner and gives two different font weights. Wondering whether we should now include a sentence to say that file sizes are approximate?

Simulator Screen Shot - iPhone 12 mini - 2021-08-27 at 11 15 44

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Aug 27, 2021

Wondering whether we should now include a sentence to say that file sizes are approximate?

my 2c: use ~ for this, e.g. Large (~100.00 KB). But not on actual size, to contrast; Actual Size (1.77 MB)

@ShadowJonathan
Copy link
Contributor

@niquewoodhouse one change you forgot to note is that the button group is now separated from the cancel button, that's not the behaviour today (as seen with what @pixlwave is showing), so maybe mark that as a requested change as well? (i really like how the seperated buttons look)

@pixlwave
Copy link
Contributor Author

@niquewoodhouse one change you forgot to note is that the button group is now separated from the cancel button, that's not the behaviour today (as seen with what @pixlwave is showing), so maybe mark that as a requested change as well? (i really like how the seperated buttons look)

Yep, sorted:
Simulator Screen Shot - iPhone 12 mini - 2021-08-27 at 12 32 40

@niquewoodhouse
Copy link

@niquewoodhouse I've used a title and message for the confirmation sheets as it looks a bit cleaner and gives two different font weights. Wondering whether we should now include a sentence to say that file sizes are approximate?

Font weights look really good, thanks. I'm not sure what to do about approximate sizes, to be honest. How approximate are they? I'm not sure if people would be that bothered if we're out by a few KB? I guess it'll matter to some cohort, I'm just not sure what % of all the people using this it would be.

my 2c: use ~ for this, e.g. Large (~100.00 KB). But not on actual size, to contrast; Actual Size (1.77 MB)

Thanks, that's very nice, I think that's the most elegant way of doing it, but I'm not sure how known ~ is as a way of meaning approximately but I prefer it to have more text, and it's so subtle that if you don't know what it means you might just not see it anyway. @pixlwave could we go ahead with @ShadowJonathan's suggestion and just test it out?

@pixlwave
Copy link
Contributor Author

pixlwave commented Aug 27, 2021

How approximate are they?

Fairly - we're guessing a file size based upon the image dimensions, but the compression also varies depending on the image contents. After a couple of quick tests I've seen one image come out at 264KB from an estimate of 102KB. And conversely another come out at 170KB from an estimate of 200KB. Like you say though, how much this matters anyway given they're all pretty small files.

File sizes using a ~ before:
Simulator Screen Shot - iPhone 12 mini - 2021-08-27 at 14 18 40

@pixlwave
Copy link
Contributor Author

pixlwave commented Aug 31, 2021

@niquewoodhouse All changes made and I've just refreshed the alpha build for you to try. Screenshots of image size selection above.

Video size:
Simulator Screen Shot - iPhone 12 mini - 2021-08-27 at 17 27 14

Settings. Note - I haven't used a footer style text here as the rest of the settings screen shows all information like this instead. I think this will change when the settings screen gets an overhaul, so seemed more sensible to let that happen there than have 1 item that looks different:
Simulator Screen Shot - iPhone 12 mini - 2021-08-31 at 09 41 44

@ShadowJonathan
Copy link
Contributor

Note - I haven't used a footer style text here as the rest of the settings screen shows all information like this instead. I think this will change when the settings screen gets an overhaul

(maybe a seperate issue for this? so the previous style doesn't get lost?)

@pixlwave pixlwave merged commit c6e7df5 into develop Sep 7, 2021
@pixlwave pixlwave deleted the doug/4479_media_size_selection branch September 7, 2021 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ability to select the size of video you're sending Remove resolution selection when sending image
4 participants