Optimize computing dominant color and transparency for images by combining the two functions by pbearne · Pull Request #381 · WordPress/performance · GitHub

…or_has_transparency() functions into single function

Updated tests and improved the images
Introduced two new functions dominant_color_get_dominant_color() and dominant_color_has_transparency() that fetchs the date from image meta

Summary

Fixes #

Relevant technical choices

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

…or_has_transparency() functions into single function Updated tests and improved the images Introduced two new functions dominant_color_get_dominant_color() and dominant_color_has_transparency() that fetchs the date from image meta

Can you explain this change a little more.

we are getting color with the shorten form #fff back from the underlining PHP code

I think it better to always return a 6 character hex here.

Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>

@pbearne I think the changes related to the issue look good for the most part, but this PR includes a bunch of additional changes which I think are out of scope as they do not relate to the underlying issue.

I think the only changes that are needed here are those in the load.php file (plus related tests). What is the reasoning for modifying the implementation of the image classes here? That seems out of scope.

…ing_up_image_editor

… function fail

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

https://www.php.net/manual/en/function.imagecolorat.php#example-2657

I pushed up some changed to add some more unit test coverage.

@pbearne Pretty much LGTM now. The remaining comments are minor, would be great if you could address them.

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

@spacedmonkey Thanks for the quick iteration. LGTM! 🎉

pbearne requested a review from spacedmonkey as a code owner June 16, 2022 21:30

pbearne added this to the 1.3.0 milestone Jun 16, 2022

pbearne added [Focus] Images [Type] Enhancement A suggestion for improvement of an existing feature labels Jun 16, 2022

pbearne mentioned this pull request Jun 16, 2022

Avoid duplicate execution of setting up image editor when determining dominant color and transparency #373

Closed

pbearne linked an issue Jun 16, 2022 that may be closed by this pull request

Avoid duplicate execution of setting up image editor when determining dominant color and transparency #373

Closed

spacedmonkey requested a review from felixarntz June 20, 2022 12:47

spacedmonkey reviewed Jun 20, 2022

View reviewed changes

Comment thread modules/images/dominant-color/class-dominant-color-image-editor-imagick.php

spacedmonkey reviewed Jun 20, 2022

View reviewed changes

Comment thread modules/images/dominant-color/load.php

spacedmonkey reviewed Jun 20, 2022

View reviewed changes

Comment thread modules/images/dominant-color/load.php

Update modules/images/dominant-color/load.php

2158a17

Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>

spacedmonkey reviewed Jun 21, 2022

View reviewed changes

Comment thread tests/utils/TestCase/DominantColorTestCase.php Outdated

felixarntz reviewed Jun 22, 2022

View reviewed changes

felixarntz added the [Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) label Jun 22, 2022

felixarntz changed the title Combined the dominant_color_get_dominant_color() and the dominant_col… Optimize computing dominant color and transparency for images by combining the two functions Jun 22, 2022

spacedmonkey reviewed Jun 22, 2022

View reviewed changes

Comment thread modules/images/dominant-color/class-dominant-color-image-editor-gd.php Outdated

spacedmonkey and others added 10 commits June 22, 2022 22:55

Force hex to 6 characters

198532c

Merge branch 'trunk' into INITS-976-Avoid_duplicate_execution_of_sett…

a6ab609

…ing_up_image_editor

changed code to return WP_error in has_transparency or dominant_color…

37b3222

… function fail

Update modules/images/dominant-color/load.php

b01ba33

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update modules/images/dominant-color/load.php

c6f0f2b

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update modules/images/dominant-color/load.php

ec6e41a

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update modules/images/dominant-color/load.php

57ab283

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update modules/images/dominant-color/load.php

43e3dd1

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update modules/images/dominant-color/load.php

e4b2e03

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Removed magic code for simpler str_pad function

1d2e440

spacedmonkey reviewed Jun 27, 2022

View reviewed changes

Comment thread modules/images/dominant-color/class-dominant-color-image-editor-gd.php Outdated

spacedmonkey added 8 commits June 27, 2022 20:21

Improve unit tests.

43b5268

Add get_hex_from_rgb function.

e75e578

Remove test for now.

83d2cce

Return wp_error.

3018d7e

Change message.

4e130cb

Check to see if the method exists.

043feb2

Improve testing.

879a43f

Ignore on multisite.

a9acc67

spacedmonkey reviewed Jun 27, 2022

View reviewed changes

spacedmonkey approved these changes Jun 28, 2022

View reviewed changes

felixarntz reviewed Jun 28, 2022

View reviewed changes

Comment thread modules/images/dominant-color/load.php Outdated

Comment thread modules/images/dominant-color/load.php Outdated

Comment thread modules/images/dominant-color/load.php Outdated

spacedmonkey and others added 2 commits June 28, 2022 21:57

Change function name.

e5f2054

Apply suggestions from code review

14073b6

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

spacedmonkey requested a review from felixarntz June 28, 2022 21:03

felixarntz approved these changes Jun 28, 2022

View reviewed changes

felixarntz merged commit 6e0ff30 into WordPress:trunk Jun 28, 2022

-              			return new WP_Error( 'image_editor_dominant_color_error', __( 'Dominant color detection failed.', 'performance-lab' ) );
-              		}
-              		return $hex;
+              		return dechex( imagecolorat( $shorted_image, 0, 0 ) );

-              		if ( strlen( $hex ) < 6 ) {
+              		$rgb = imagecolorat( $shorted_image, 0, 0 );
+              		$r   = ( $rgb >> 16 ) & 0xFF;

Optimize computing dominant color and transparency for images by combining the two functions by pbearne · Pull Request #381 · WordPress/performance · GitHub

Uh oh!

Optimize computing dominant color and transparency for images by combining the two functions - #381

Conversation

pbearne commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

spacedmonkey Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

pbearne Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

spacedmonkey Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacedmonkey Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

spacedmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pbearne commented Jun 16, 2022 •
edited
Loading