Skip to content

Exact Size #123

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.

Sign up for GitHub

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

Merged

adityapatadia merged 2 commits into gumlet:master from felipetnh:gumlet-save-exact-size

Nov 30, 2018

Merged

Exact Size #123

adityapatadia merged 2 commits into gumlet:master from felipetnh:gumlet-save-exact-size

Nov 30, 2018

Conversation

Copy link

Contributor

@felipetnh felipetnh commented Sep 24, 2018


 Exact Size

Copy link

Contributor Author

felipetnh commented Sep 24, 2018

peter279k reviewed

View reviewed changes

lib/ImageResize.php

imagecolortransparent($dest_image, $background);

imagefill($dest_image, 0, 0, $background);

if( !empty($exact_size) && is_array($exact_size) ){

$dest_image = imagecreate($exact_size[0], $exact_size[1]);

Copy link

Contributor

@peter279k peter279k Sep 26, 2018

Choose a reason for hiding this comment

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

This seems to have the additional space.

peter279k reviewed

View reviewed changes

lib/ImageResize.php

if( !empty($exact_size) && is_array($exact_size) ){

$dest_image = imagecreate($exact_size[0], $exact_size[1]);

} else{

$dest_image = imagecreate($this->getDestWidth(), $this->getDestHeight());

Copy link

Contributor

@peter279k peter279k Sep 26, 2018

Choose a reason for hiding this comment

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

This seems to have the additional space.

Copy link

Contributor Author

@felipetnh felipetnh Sep 26, 2018

Choose a reason for hiding this comment

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

What do you mean by "the additional space"?

Copy link

Contributor

@peter279k peter279k Sep 26, 2018

Choose a reason for hiding this comment

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

The white space seems to be existed before the $dest_image variable :).

peter279k reviewed

View reviewed changes

lib/ImageResize.php Outdated

define('IMAGETYPE_WEBP', 18);

}

if ($filename === null || empty($filename) || (substr($filename, 0, 5) !== 'data:' && !is_file($filename))) {

if ($filename === null || empty($filename) || (substr($filename, 0, 7) !== 'data://' && !is_file($filename))) {

Copy link

Contributor

@peter279k peter279k Sep 26, 2018

Choose a reason for hiding this comment

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

The Travis CI build is broken because this image file checking codition is modified.
I think this should revert the original condition.

Copy link

Contributor Author

@felipetnh felipetnh Sep 26, 2018

Choose a reason for hiding this comment

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

This substr($filename, 0, 7) was on the master branch. The version I downloaded was the previous one. So I wasn't sure what to do.

Copy link

Contributor

@peter279k peter279k Sep 26, 2018

Choose a reason for hiding this comment

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

Well. Checking the latest merged PR seems that it's different.

Copy link

Contributor

@peter279k peter279k Sep 26, 2018

Choose a reason for hiding this comment

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

The master branch is ($filename, 0, 5). Please check that :-).

peter279k suggested changes

View reviewed changes

Copy link

Contributor

@peter279k peter279k left a comment

Choose a reason for hiding this comment

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

Hi @felipetnh, thank you for your reply.
The Travis CI build work is broken and it should be fixed :).
Thank you for your PR.


 Changed back

203a046

Copy link

coveralls commented Sep 26, 2018 •

edited

Loading

Pull Request Test Coverage Report for Build 195

12 of 33 (36.36%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-3.7%) to 81.507%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/ImageResize.php	12	33	36.36%

Files with Coverage Reduction	New Missed Lines	%
lib/ImageResize.php	2	72.34%

Totals	Coverage Status
Change from base Build 193:	-3.7%
Covered Lines:	238
Relevant Lines:	292

💛 - Coveralls

1 similar comment

Copy link

coveralls commented Sep 26, 2018

Pull Request Test Coverage Report for Build 195

12 of 33 (36.36%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-3.7%) to 81.507%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/ImageResize.php	12	33	36.36%

Files with Coverage Reduction	New Missed Lines	%
lib/ImageResize.php	2	72.34%

Totals	Coverage Status
Change from base Build 193:	-3.7%
Covered Lines:	238
Relevant Lines:	292

💛 - Coveralls

Copy link

Contributor

adityapatadia commented Oct 4, 2018

@felipetnh if you can fix the CI error, we can merge this.

Copy link

Contributor

peter279k commented Oct 4, 2018

@adityapatadia, it seems that the CI work is successful, but the coveralls is failed.

It looks like the code coverage is decreased and I think we need to add more tests to recover the code coverage...

Copy link

Contributor Author

felipetnh commented Oct 4, 2018 via email

Guys, I have no idea what you're talking about. CI errors? Coveralls? Those things aren't really my thing. Tell me what I need to do and I'll gladly update the PR. Thanks! Em qui, 4 de out de 2018 04:33, peter279k <notifications@github.com> escreveu:

@adityapatadia <https://github.com/adityapatadia>, it seems that the CI work is successful, but the coveralls is failed. It looks like the code coverage is decreased and I think we need to add more tests to recover the code coverage... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEEkMyYyqnltoPncs7C2MAWKOzFsUbAfks5uhbnDgaJpZM4W3UaV> .

Copy link

Contributor

peter279k commented Oct 4, 2018

Hi @felipetnh, do you see this captured image?
image

The coverage/coveralls is failed because the coverage decreased. You need to do is add more tests to recover the code coverage back 😄.

Copy link

Contributor Author

felipetnh commented Oct 4, 2018 via email

And how do I do that? I've never worked with test cases. Em qui, 4 de out de 2018 às 07:48, peter279k <notifications@github.com> escreveu:

Hi @felipetnh <https://github.com/felipetnh>, do you see this captured image? [image: image] <https://user-images.githubusercontent.com/9021747/46469397-d17fb780-c805-11e8-89e8-cdcad0d2cd80.png> The coverage/coveralls is failed because the coverage decreased. You need to do is add more tests to recover the code coverage back 😄. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEEkM9rfKkk9RYyn-AAr9zMFHWER6xFsks5uhedkgaJpZM4W3UaV> .

Copy link

Contributor

peter279k commented Oct 4, 2018

@felipetnh, thank you for your reply. Let me take a look at your forked repo and suggest the recommendations to increase code coverage :).

Copy link

Contributor

peter279k commented Oct 5, 2018

After tracing the source code, it looks like that we don't have to increase code coverage because this decreased percent is small.

@adityapatadia, I think it's time to be merged now.

Copy link

Contributor

adityapatadia commented Oct 16, 2018

It would be great if we can also add details about this in Readme.

Copy link

Contributor

peter279k commented Oct 16, 2018 •

edited

Loading

Hi @adityapatadia, how about adding some details for this issue in released draft when releasing the latest version?

Copy link

ekinhbayar commented Nov 12, 2018

Any update on this?

Copy link

Contributor

peter279k commented Nov 13, 2018

Hi @ekinhbayar, thank you for your concern.

Just wait for @adityapatadia review this PR.

@adityapatadia adityapatadia merged commit e7a349b into gumlet:master

peter279k reviewed

View reviewed changes

lib/ImageResize.php

imagegammacorrect($this->source_image, 2.2, 1.0);

if( !empty($exact_size) && is_array($exact_size) ) {

Copy link

Contributor

@peter279k peter279k Jun 6, 2019 •

edited

Loading

Choose a reason for hiding this comment

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

It's mentioned on issue #142.

Copy link

Contributor

@peter279k peter279k Jun 25, 2019

Choose a reason for hiding this comment

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

@adityapatadia, this codes will effect and cause on the issue #142.

The line 291 to 300.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants

@felipetnh @coveralls @adityapatadia @peter279k @ekinhbayar