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

Add correct MIME type "image/jpeg" on image capture for file uploads #259

Closed
wants to merge 2 commits into from
Closed

Add correct MIME type "image/jpeg" on image capture for file uploads #259

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 28, 2023

#141 is an awesome feature but was not working out of the gate. I was not using "image/*" and saw that "image/jpg" was also set for accept but since I had "image/jpeg" it didn't work right away. There is no MIME type "image/jpg" and the actual type is "image/jpeg". This commit adds the correct MIME type while keeping "image/jpg" for anyone that used that with this feature. It'd probably be good to add more common image types in the future.

Also changed to .contains instead of == so that if the string matches anywhere in accept it will be enabled. This will streamline using the feature as it is common to have multiple types such as accept="image/jpeg, image/png, image/webp".

Connor Bey 13™ added 2 commits February 27, 2023 19:34
Copy link
Collaborator

@jayohms jayohms left a comment

Choose a reason for hiding this comment

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

Thanks for noticing! Agree that we should support both jpeg mime types 👍

@@ -56,7 +56,7 @@ internal class TurboCameraCaptureDelegate(val context: Context) {
private fun FileChooserParams.allowsCameraCapture(): Boolean {
val accept = defaultAcceptType()
val acceptsAny = accept == "*/*"
val acceptsImages = accept == "image/*" || accept == "image/jpg"
val acceptsImages = accept.contains("image/*") || accept.contains("image/jpg") || accept.contains("image/jpeg")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer that we're a bit more strict here. We extract the default accept type, but we can also check the array of acceptTypes for the jpeg mime types.

This feels like a good balance to check that the default accept type is a wildcard type or the acceptTypes array supports jpegs. Here's what it'd look like:

val defaultAccept = defaultAcceptType()
val acceptsAny = defaultAccept == "*/*"
val acceptsImages = defaultAccept == "image/*" || acceptTypes.any {
    it == "image/jpg" || it == "image/jpeg"
}

Copy link
Author

Choose a reason for hiding this comment

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

Looks good!

@@ -56,7 +56,7 @@ internal class TurboCameraCaptureDelegate(val context: Context) {
private fun FileChooserParams.allowsCameraCapture(): Boolean {
val accept = defaultAcceptType()
val acceptsAny = accept == "*/*"
val acceptsImages = accept == "image/*" || accept == "image/jpg"
val acceptsImages = accept.contains("image/*") || accept.contains("image/jpg") || accept.contains("image/jpeg")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The TurboCameraCaptureDelegateTest test class should be improved to handle cover these mime types:

  • image/*
  • image/jpg (as the first and non-first acceptType)
  • image/jpeg (as the first and non-first acceptType)

Copy link
Author

Choose a reason for hiding this comment

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

Don't have a lot of experience in Kotlin and tests. I did try making some adding in new params and used assertThat(intent).isNotNull() but that made the buildIntent fail. Would need some help here.

@olliekav
Copy link

Any chance of getting this branch merged?

@jayohms
Copy link
Collaborator

jayohms commented Aug 29, 2023

Closing in favor of #282

@jayohms jayohms closed this Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants