Skip to content

QuickEditor: Take photo action in the media picker#266

Merged
AdamGrzybkowski merged 7 commits intotrunkfrom
adam/225_take_photo
Aug 28, 2024
Merged

QuickEditor: Take photo action in the media picker#266
AdamGrzybkowski merged 7 commits intotrunkfrom
adam/225_take_photo

Conversation

@AdamGrzybkowski
Copy link
Copy Markdown
Contributor

Closes #225

Description

This PR implements the Take photo option from the media picker popup.

An important bit here is the FileProvider config - this needs to be custom so that it will work when the app implementing our library has its own FileProvider setup. You can read about it here.
I've also added a config to our DemoApp to make sure this works (that's how I discovered this needs to be handled).

take_photo.mp4

Testing Steps

Important Note: The backend is currently returning the wrong format for the updated date field. As a workaround, you can apply this patch to comment that field from the Avatar object.
Gravatar-Android-13-04-48.patch

  1. Make sure you have at least two avatars uploaded on Gravatar
  2. Run the DemoApp
  3. Go to Avatar Update tab
  4. Tap Update Avatar button
  5. Follow the steps to log in (if necessary)
  6. Tap the Upload Image button
  7. Tap the Take photo
  8. Take photo with camera
  9. Crop if you want and confirm
  10. Confirm you see the photo in the list with the progress indicator
  11. Confirm the photo was uploaded and can be selected

@AdamGrzybkowski AdamGrzybkowski added this to the Quick Editor milestone Aug 14, 2024
appcompat = "1.7.0"
material = "1.12.0"
ucrop = "2.2.9"
ucrop = "2.2.9-native"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had to switch to the native version because the compression wouldn't work for me and the uploaded avatar would be the same size as the original image despite the compression.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From uCrop's documentation:

implementation 'com.github.yalantis:ucrop:2.2.8-native' - get power of the native code to preserve image quality (+ about 1.5 MB to an apk size)

1.5 MB is a lot. Is there any workaround?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I haven't found one in this library. We can handle the compression ourselves on top of it though, but that will be tricky as we have to be careful not to duplicate the compression when the one from the library works (I imagine this might be dependent on the system version for example).

Using the -native variant seemed like a better pick for now to not spend too much time on it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@maxme I'm curious to know what you think of this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding 1.5Mb is not ideal, but what worries me most is adding native code to our library dependencies. For deployments on the app store, it's going to be extra steps, and might be limiting for our users. They might be surprised they have native code and might look for the culprit dependency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had to switch to the native version because the compression wouldn't work for me and the uploaded avatar would be the same size as the original image despite the compression.

Do you get some logs or is this a silent error (they skip the compression steps maybe)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I haven't seen any errors (I haven't looked for them though at that point) just that the final image had the exact same size as the photo taken with the camera. I will take another look at it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm testing this on different devices/emulators and the image size is always the same. The compression seems to work in a way that you can clearly see the drop in the image quality and the width/height sizes. Yet the file size remains unchanged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed the -native version for now.


private const val UCROP_COMPRESSION_QUALITY = 100
private const val UCROP_COMPRESSION_QUALITY = 90
private const val UCROP_MAX_IMAGE_SIZE = 1080
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Set this as the max size for now - seems enough for an Avatar though happy to hear your opinions.

@AdamGrzybkowski AdamGrzybkowski force-pushed the adam/226_pick_photo_from_library branch from d486a81 to b60791e Compare August 19, 2024 12:21
@AdamGrzybkowski AdamGrzybkowski force-pushed the adam/226_pick_photo_from_library branch from b60791e to 7423491 Compare August 20, 2024 20:06
Base automatically changed from adam/226_pick_photo_from_library to trunk August 21, 2024 13:08
@AdamGrzybkowski AdamGrzybkowski force-pushed the adam/225_take_photo branch 5 times, most recently from 4189b9c to 1be8d2f Compare August 28, 2024 06:42
@maxme
Copy link
Copy Markdown
Contributor

maxme commented Aug 28, 2024

Note: we're currently waiting for a reply/merge to fix a bug in uCrop - Yalantis/uCrop#926

@AdamGrzybkowski
Copy link
Copy Markdown
Contributor Author

Note: we're currently waiting for a reply/merge to fix a bug in uCrop - Yalantis/uCrop#926

I don't think it makes sense to block this PR because of this. We can fix this later by either updating the UCrop version or switching to our fork.

@maxme maxme self-requested a review August 28, 2024 11:43
@AdamGrzybkowski AdamGrzybkowski merged commit fb32d47 into trunk Aug 28, 2024
@AdamGrzybkowski AdamGrzybkowski deleted the adam/225_take_photo branch August 28, 2024 12:18
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.

QuickEditor: Option to use camera to upload a new avatar

3 participants