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

[image-picker][ios] Fix unexpected behaviour of quality option on iOS #21361

Merged
merged 12 commits into from
Feb 24, 2023

Conversation

behenate
Copy link
Member

Why

On iOS when using ImagePicker with .bmp files the file would be converted to .png if any quality is selected, even though the option should be ignored. Therefore setting quality to 0.2 (default value) would yield different results than for not setting it at all #20299

How

Updated the ImagePickerModule to ignore the quality option with .bmp files.

When opening a .bmp image with allowsEditing set to true file has to be converted to .png, now the fileName is also updated to have .png file extension. Docs have been updated to make it more clear when the conversion happens.

Test Plan

Tested on a physical device with opening images from gallery and the camera.

@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing links in changelog entries


I've added some suggestions below, you can just apply and commit them 😉

❌ Error: SwiftLint Violations


Found 3 violations, 1 serious. See the review comments below for more insights.


Generated by ExpoBot 🤖 against 3ea3075

packages/expo-image-picker/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
@expo-bot expo-bot added the bot: needs changes ExpoBot found things that don't meet our guidelines label Feb 23, 2023
Copy link
Contributor

@barthap barthap left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! The previous behaviour was incorrect indeed.
The actual fix code (switch cases for BMP) looks good! 🎉

But please try doing the filename replacement the Swift way. Personally, I'd create a helper function, which looks sth like this (written by hand, probably wouldn't compile):

func replaceFileExtension(fileName: String?, targetExtension: String) -> String? {
  guard let fileName = fileName else { return nil }
  if !fileName.lowercased.contains(targetExtension.lowercased()) {
    return fileName.deletingPathExtension + targetExtension
  }
  return fileName
}

Also, please use Swift-specific methods for that, as linter suggests.

If you need more help with this, feel free to ping me

Edit: One more thing. You need to run yarn build from inside the packages/expo-image-picker directory and commit the generated build/* files for CI to pass

packages/expo-image-picker/src/ImagePicker.types.ts Outdated Show resolved Hide resolved
@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing links in changelog entries


I've added some suggestions below, you can just apply and commit them 😉

⚠️ Suggestion: SwiftLint Violations


Found 4 violations, 0 serious. See the review comments below for more insights.


Generated by ExpoBot 🤖 against 39880f3

packages/expo-image-picker/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: SwiftLint Violations


Found 4 violations, 0 serious. See the review comments below for more insights.


Generated by ExpoBot 🤖 against fb8016b

packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
packages/expo-image-picker/ios/MediaHandler.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@barthap barthap left a comment

Choose a reason for hiding this comment

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

LGTM

packages/expo-image-picker/src/ImagePicker.types.ts Outdated Show resolved Hide resolved
@lukmccall lukmccall merged commit 9772ed8 into main Feb 24, 2023
@lukmccall lukmccall deleted the @behenate/image-picker-quality-fix branch February 24, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: needs changes ExpoBot found things that don't meet our guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants