-
Notifications
You must be signed in to change notification settings - Fork 818
Changed compressionQuality by default to 0 #1744
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
Changed compressionQuality by default to 0 #1744
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vicajilau Does this fix the compression quality bug?
Maybe we should just have allowCompression
be deprecated (and removed in the Java class).
I think we can look at the value of compressionQuality
to determine if we should compress the image? Like, if compressionQuality is 0 (which would be the new default), we do not compress.
@vicajilau @navaronbracke many thanks for your fast resolution, i appreciate that, for me the resolution its perfect! i would add only one think about that and its instead create the compressed image into pictures folder (when the user set any compression level), create the image into app folder to not create a lot of images into a shared folder |
@navaronbracke and @daniJimen, We have to keep in mind that only Android and iOS uses these two properties ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small remarks, but otherwise LGTM.
Sadly changing the default compression quality could be seen as a breaking change? (Even though it is used to fix a bug)
if (onFileLoading != null) { | ||
_eventSubscription = _eventChannel.receiveBroadcastStream().listen( | ||
(data) => onFileLoading((data as bool) | ||
(data) => onFileLoading((data is bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its still broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String timeStamp = new SimpleDateFormat("yyyyMMdd_HHmmss", Locale.getDefault()).format(new Date()); | ||
String imageFileName = "JPEG_" + timeStamp + "_"; | ||
File storageDir = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_PICTURES); | ||
File storageDir = context.getCacheDir(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is to fix #1743 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES!! And not only because this will fix #1742 too, also aligning android and iOS flows
private static File createImageFile() throws IOException { | ||
String timeStamp = new SimpleDateFormat("yyyyMMdd_HHmmss").format(new Date()); | ||
private static File createImageFile(Context context) throws IOException { | ||
String timeStamp = new SimpleDateFormat("yyyyMMdd_HHmmss", Locale.getDefault()).format(new Date()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch for the locale!
Co-authored-by: Navaron Bracke <[email protected]>
Co-authored-by: Navaron Bracke <[email protected]>
Everything ready @navaronbracke 🚀 |
Conflicts resolved @navaronbracke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update the major version, since this changes the default value of the compressionQuality
, so people might be surprised if the compression breaks when updating. (going from a default of 30 to 0)
Co-authored-by: Navaron Bracke <[email protected]>
Co-authored-by: Navaron Bracke <[email protected]>
CHANGELOG.md
Outdated
@@ -1,10 +1,18 @@ | |||
## 10.0.0 | |||
### General | |||
- The `compressionQuality` property in the `pickFiles` method now defaults to `0`, and `allowCompression` is set to `false`. [@vicajilau](https://github.com/vicajilau). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you removed the wrong duplicated section here? Now the breaking change label is gone.
Everything ready as you suggested @navaronbracke 🚀 |
Updated the OP so that the mentioned issues will be closed when this is merged. |
Fixes #1742
Fixes #1743