Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@katout
Copy link

@katout katout commented Oct 17, 2018

The image size is very large in the current implementation.
I added items that can set jpeg's quality

e.g.

ImagePicker.pickImage(
  source: ImageSource.camera,
  maxWidth: maxWidth,
  maxHeight: maxHeight,
  resizeQuality: resizeQuality,
)

@katout katout changed the title add resize quality [image_picker] add resize quality Oct 18, 2018
@katout
Copy link
Author

katout commented Oct 19, 2018

I'm sorry
The quality setting of ios is incorrect.

I will fix it when I get home.

@katout
Copy link
Author

katout commented Oct 19, 2018

ok!

@nosmirck
Copy link

nosmirck commented Oct 26, 2018

hello @katout

I'm about to make a PR on your branch, please take a look, I didn't change anything really, mostly got rid of the unnecessary changes (gradle and other android files) plus one minor change in the ImageResizer to get the quality before passing it to the method and another in the dart file to validate that the resizeQuality passed is between 0.0 and 1.0

[EDIT]
Apparently I can't make a PR into your branch. Any suggestions? my branch is this one:

https://github.com/nosmirck/plugins/tree/add_quality

}

allprojects {
rootProject.allprojects {

Choose a reason for hiding this comment

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

unnecessary change

Copy link
Contributor

Choose a reason for hiding this comment

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

We prob don't need this.

Copy link

@nosmirck nosmirck left a comment

Choose a reason for hiding this comment

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

I added some comments requesting a few changes.

repositories {
google()
jcenter()
mavenLocal()

Choose a reason for hiding this comment

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

unnecessary change

Copy link
Contributor

Choose a reason for hiding this comment

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

don't need this


dependencies {
classpath 'com.android.tools.build:gradle:3.1.2'
classpath 'com.android.tools.build:gradle:3.2.0'

Choose a reason for hiding this comment

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

unnecessary change

repositories {
google()
jcenter()
mavenLocal()

Choose a reason for hiding this comment

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

unnecessary change

boolean shouldDownscaleHeight = hasMaxHeight && maxHeight < originalHeight;
boolean shouldDownscale = shouldDownscaleWidth || shouldDownscaleHeight;

resizeQuality = resizeQuality != null ? resizeQuality : new Double(1.0);

Choose a reason for hiding this comment

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

int quality = resizeQuality != null ? ((int) resizeQuality * 100.0) : 100;

Bitmap scaledBmp = Bitmap.createScaledBitmap(bmp, width.intValue(), height.intValue(), false);
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
scaledBmp.compress(Bitmap.CompressFormat.JPEG, 100, outputStream);
scaledBmp.compress(Bitmap.CompressFormat.JPEG, (int) (resizeQuality * 100), outputStream);

Choose a reason for hiding this comment

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

 scaledBmp.compress(Bitmap.CompressFormat.JPEG, quality, outputStream);

repositories {
google()
jcenter()
mavenLocal()

Choose a reason for hiding this comment

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

unnecessary change


dependencies {
classpath 'com.android.tools.build:gradle:3.1.2'
classpath 'com.android.tools.build:gradle:3.2.0'

Choose a reason for hiding this comment

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

unnecessary change

repositories {
google()
jcenter()
mavenLocal()

Choose a reason for hiding this comment

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

unnecessary change

zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.4-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip

Choose a reason for hiding this comment

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

unnecessary change

double resizeQuality,
}) async {
assert(source != null);

Choose a reason for hiding this comment

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

check for value between 0.0 and 1.0

if (resizeQuality != null && (resizeQuality < 0 || resizeQuality > 1.0)) {
  throw ArgumentError.value(resizeQuality, 'resizeQuality must be between 0.0 and 1.0');
}

@katout
Copy link
Author

katout commented Oct 28, 2018

Hi @nosmirck !
Thank you for checking 👍

check for value between 0.0 and 1.0

I thought that libraries of android and ios would check that, but I think so too.

unnecessary change

Certainly, it should be fixed with another PR rather than this PR.
I compiled it into one for the local test to be successful...
I would like the maintainer to decide whether it is acceptable.

@katout
Copy link
Author

katout commented Oct 28, 2018

If @nosmirck threw a pull request to katout, please create another branch from https: //github.com/katout/plugins/tree/add_quality (different from add_quality )and request katout / plugins.

I am sorry, git and English are not good 🤕

@nosmirck
Copy link

If @nosmirck threw a pull request to katout, please create another branch from https: //github.com/katout/plugins/tree/add_quality (different from add_quality )and request katout / plugins.

I am sorry, git and English are not good 🤕

I tried creating a PR to your repo, but apparently your repo is not accepting PRs (or maybe I did something wrong).

You might as well try to pull my branch, delete your image_picker folder inside your branch and replace with mine (is the same thing plus the fixes I added).

Also, if you give me access to your fork, I might just push my changes too.

@katout
Copy link
Author

katout commented Oct 30, 2018

I think that it is because you are running fork with flutter/plugins instead of katout/plugins.
and I see Your branch (nosmirck/plugins) history derived, not a derived branch from katout/plugins.
Sorry if different 😢

Please do your best because the modifications should remain in the history

@nosmirck
Copy link

@katout I managed to do it! I had to delete my old fork and fork again from yours, branch from your branch and add my changes.

The PR is there and awaiting :)

It has been a pleasure working with you XD

# Conflicts:
#	packages/image_picker/example/android/gradle/wrapper/gradle-wrapper.properties
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Sorry for the late response.
The image_picker has been updated a lot since this PR and there seems to be conflicts, do you mind to rebase and resolve the conflicts? Note that we added a type check for the images; so on IOS, you might need to handle the case that PNG and GIF doesn't support qualities.

}

allprojects {
rootProject.allprojects {
Copy link
Contributor

Choose a reason for hiding this comment

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

We prob don't need this.

repositories {
google()
jcenter()
mavenLocal()
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need this

@katout
Copy link
Author

katout commented Jul 12, 2019

Sorry, I have no motivation to maintain it because it is too old.
Please close this issue as it looks like a little while reading the code.

@cyanglaz
Copy link
Contributor

@katout Thank you for the quick response. I do think what the PR is trying to do is valuable. It isn't trivial to merge right now. I'm following the initial PR review policy, I'm labeling it with "backlog" and we will prioritize according to the issue's priority.

The issue is created here flutter/flutter#36072

@cyanglaz
Copy link
Contributor

cyanglaz commented Aug 5, 2019

This feature has been added in #1896
Closing this PR.

@cyanglaz cyanglaz closed this Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants