[#3568] - [#33765] Add fast check for image orientation
- Closed
- 28 May 2015
- Medium
- Build: staging
- # 3568
- Diff
- itbra:patch-15
- Foreign ID 33765
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Detecting an image's orientation (landscape, portrait, etc) may become a repetitive task when is comes to decide which values to set for width and height to the resize() method. I found my self in need to know if the images i process are in landscape or portrait or (rather rarly) in square format. As a first step i coded this check whereever i needed it which i found pretty unhandy. Querying this information from the JImage class statically like is possible with the getImageFileProperties() method was a very helpful faster solution.
At another stage (when preparing my view) i found myself again require to know an image's orientation to apply a CSS class. As can be seen the need for this information is there which is why i add this pull request including the 3 new methods: isCubic(), isLandscape(), isPortrait().
Test Instructions (mabe somebody experienced is willing to adopt it into the image test class?
1. Create the helper class file from this PR under JPATH_LIBRARIES . '/joomla/image/helper.php'
2. In JPATH_LIBRARIES . '/joomla/image/image.php' method `getImageFileProperties()` replace
// Build the response object.
$properties = (object) array(
'width' => $info[0],
'height' => $info[1],
'type' => $info[2],
'attributes' => $info[3],
'bits' => isset($info['bits']) ? $info['bits'] : null,
'channels' => isset($info['channels']) ? $info['channels'] : null,
'mime' => $info['mime']
);
return $properties;
with
return JImageHelper::getImageFileProperties($path);
as wished by @dongilbert.
3. Take any file e.g. the currently active template's index.php and at the very top
right below `defined('_JEXEC') or die;` paste this code:
// Define the file to use.
// NOTE: Make sure this file exists or use another path to an existing image file!
$file = JPATH_BASE . '/images/powered_by.png';
echo '<pre>file: ' . print_r($file, true) . '</pre>';
echo '<pre>' . print_r(str_repeat('_', 51), true) . '</pre>';
// Get instance of JImage.
$image = new JImage($file); // $image must be instance of JImage
// Get image properties calling static method from JImageHelper.
$props = JImageHelper::getImageFileProperties($file); // $props must be an object
echo '<pre>props: ' . print_r($props, true) . '</pre>';
echo '<pre>' . print_r(str_repeat('_', 51), true) . '</pre>';
echo '<pre>' . print_r('Check for every property separately:', true) . '</pre>';
echo '<pre>' . print_r(str_repeat('_', 51), true) . '</pre>';
// Check the returned data contains all of these properties.
foreach (array(
'width',
'height',
'type',
'attributes',
'bits',
'channels',
'mime',
'filesize',
'orientation') as $p
)
{
if (array_key_exists($p, $props))
{
echo "<pre>{$p} is: " . print_r($props->$p, true) . '</pre>';
}
else
{
echo '<pre>' . print_r("\$props does not contain {$p}-data", true) . '</pre>';
}
}
echo '<pre>' . print_r(str_repeat('_', 51), true) . '</pre>';
jexit('Done');
The expected output should be:
file: /path/to/joomla/images/powered_by.png
___________________________________________________
props: stdClass Object
(
[width] => 150
[height] => 35
[type] => 3
[attributes] => width="150" height="35"
[bits] => 8
[channels] =>
[mime] => image/png
[filesize] => 2301
[orientation] => landscape
)
___________________________________________________
Check for every property separately:
___________________________________________________
width is: 150
height is: 35
type is: 3
attributes is: width="150" height="35"
bits is: 8
channels is:
mime is: image/png
filesize is: 2301
orientation is: landscape
___________________________________________________
Done
Any feedback is welcome.
| Labels | |||
| Labels |
Added:
?
|
||
Agree. The generic method indeed provides more flexibility.
What do you think, should i replace my previous implementation or should we wait for more feedback?
I'd wait a day and see if someone else has an issue with it.
Agree. The generic method indeed provides more flexibility.
Shouldn't that be "square" rather than "cubic"? I don't think we support 3D printers yet. ;-)
Shouldn't that be "square" rather than "cubic"? I don't think we support 3D printers yet. ;-)
See, that's why it's good to have native english speakers looking over it
Perhaps something like that would be an improvement:
https://github.com/itbra/joomla-cms/pull/2/files
I moved all the exception stuff to a helper method.
What about using just one method, so we don't have to run all three to check the orientation.
Simpler usage and shorter commit
const ORIENTATION_LANDSCAPE = 1;
const ORIENTATION_PORTAIT = 2;
const ORIENTATION_SQUARE = 4;
/**
* @return integer Orientation constant
*/
public static function getOrientation($path)
{
$info = getimagesize($path);
if ($info[0] > $info[1])
{
return static::ORIENTATION_LANDSCAPE;
}
else if ($info[0] < $Info[1])
{
return static::LANDSCAPE_PORTAIT;
}
else
{
return static::LOCATION_SQUARE;
}
}
one method looks better
I absolutely agree to the one-method-solution, which is why i replaced my previous implementation by a single generic method.
However, consider another situation where a user already created an instance of JImage and already has a linked resource waiting for processing (resize, crop, etc.). The user now wants to get the desired information from this resource. When implementing the getOrientation() method statically the user has no access to this method since the class doesn't inherit another class that might provide a static orientation check method. And calling our statically implemented getOrientation() method from within the instance would result in an error, because it is not possible to call a static method from within an instance via $this->getOrientation().
So it seems we either have to add a parent class (containing the static methods) which JImage inherits from or the getOrientation() method must not be implemented statically requiring to create a new JImage instance passing in the resource of the instance the user is currently working with everytime a he wants to get the desired information.
What do you think?
@piotr-cz I wonder if it may be required to have constants for the orientation as i cannot imagine any other situation where these contants might be helpful. Having numeric values for the orientation appears to be useless to me.
| Description | <p>Detecting an image's orientation (landscape, portrait, etc) is a repetitive task when is comes to decide which values to set for width and height to the resize() method. I found my self in need to know if the images i process are landscape or portrait or rather rare in cubic format. As a first step i coded this check where i needed it which i found pretty unhandy. Querying this information statically like is possible with the getImageFileProperties() method was a very helpful fast accessor for this information.</p> <p>At another stage (when preparing my view) i am again required to know an image's orientation to apply a CSS class. As can be seen the need for this information is there which is why i add this pull request including the 3 new methods: isCubic(), isLandscape(), isPortrait().</p> <p>Any feedback is welcome.</p> | ⇒ | <p>Detecting an image's orientation (landscape, portrait, etc) may become a repetitive task when is comes to decide which values to set for width and height to the resize() method. I found my self in need to know if the images i process are in landscape or portrait or (rather rarly) in square format. As a first step i coded this check whereever i needed it which i found pretty unhandy. Querying this information from the JImage class statically like is possible with the getImageFileProperties() method was a very helpful faster solution.</p> <p>At another stage (when preparing my view) i found myself again require to know an image's orientation to apply a CSS class. As can be seen the need for this information is there which is why i add this pull request including the 3 new methods: isCubic(), isLandscape(), isPortrait().</p> <p>Any feedback is welcome.</p> |
| Labels |
Added:
?
?
|
||
@itbra Yes, I'm not sure whether to use static method or not.
I'd say the convention is to have all static method in an helper class like JImageHelper but in JImage we already have static getImageFileProperties.
Maybe static methods might be useful for processing images in bulk when creating a JImage instance for every file would be waste of resources.
Well, at the end we can have both :) Let's see what @dongilbert thinks.
As for the constants, the goal is to not use it's values, just names
switch (JImage::getOrientation($imgPath))
{
// No integeres, but constant name
case JImage::ORIENTATION_LANDSCAPE:
echo "It's landscape image";
break;
// Landscape and portait gives square, this is when using integers might be handy
case JImage::ORIENTATION_LANDSCAPE & JImage::ORIENTATION_PORTAIT:
echo "Square";
}
I think i figured a good solution. Let me explain referring to my current situation (multi-upload of several images the same time)
Uploading an image includes several tasks like path sanitasation, input validation, upload processing.
Assuming all sanitasation succeeded i now have a bunch of tmp files waiting to be processed in $_FILES. What i now do is:
- Create a new instance of JImage not passing in anything to the constructor and assign it to
$jimage. - Iteratively save every uploaded file into a tmp folder via
JFile::move($input, $output). Then call$jimage->loadFile($tmpfile). This loads the uploaded file ready for processing (resice, scale, crop, etc.). - Call
$jimage->getOrientation()method not passing in a parameter. This will cause the method to use the loaded resource and fetch the orientation from it. - Evaluate the returned value like:
$isLandscape = $jimage->getOrientation() == JImage::ORIENTATION_LANDSCAPE;
$isPortrait = $jimage->getOrientation() == JImage::ORIENTATION_PORTRAIT;
$isSquare = $jimage->getOrientation() == JImage::ORIENTATION_SQUARE;
- Process the resource and save it.
if ($isLandscape)
{
$jimage->cropResize($fullsize_width, null);
}
elseif ($isPortrait)
{
$jimage->cropResize(null, $fullsize_height);
}
elseif ($isSquare)
{
$jimage->cropResize($fullsize_width, $fullsize_height);
}
$jimage->toFile($targetFile);
Then, when it comes to prepare the view and assign classes for CSS access:
- Create a new instance of JImage not passing in anything to the constructor and assign it to
$jimage. - Iteratively check whether the file exists and call
$jimage->loadFile($file). This loads the uploaded file ready for processing (resice, scale, crop, etc.). - Dump the orientation
$orientation = $jimage->getOrientation() - Assign the orientation to the class attribute which will then be output.
This way i found the non-static implemenation seems to be the better solution. Also, as in either case there is only one JImage instance doing the job, there is no waste of resource as mentioned by piotr-cz above.
Your comments and arguments are highly welcome.
Let's use non-static method then.
We are already using constants for scale methods and IMHO this is a standard in programming when values are not dynamic. I could not find any resources to support this, but consider that
Imagick::getImageOrientation is using constants, and SilverStripe::Image uses same constants as the one I suggested (the API is very similar to our JImage).
As for orientation checking in your example, I guess your code would look cleaner using switch control structure.
May I suggest the following code:
$orientation = $jimage->getOrientation();
$width = ($orientation == 'portrait') ? null : $fullsize_width;
$height = ($orientation == 'landscape') ? null: $fullsize_height;
$jimage->cropResize($width, $height);
$jimage->toFile($targetFile);
I would use static class constants, so I can check:
[...]
if ($orientation == JImage::ORIENTATION_LANDSCAPE)
[...]
(and the method itself returns one of the constants)
So it doesn't matter what value (= string) the methods return or if you use "portrait" or "-1" etc. (or other stuff in future versions) it will always works
You convinced me. I updated my previous comment and the PR.
I think we are good to go for testing and merging.
Hi @itbra, good work on this so far. I really like your concept here and great job taking in feedback. I'd like to suggest a couple more improvements though, if I could.
IMO, the current getImageFileProperties() static method is not very useful, because it just re-maps the results of getimagesize($imgPath). I wonder if it would be more useful if we also added an orientation property to the retuned data from getImageFileProperties(). That would provide you with the static access to the info that you originally wanted (by providing a file path), and also remove the code duplication from the getOrientation() method. To accomplish this, I would move the logic for determining orientation to a JImageHelper static method. That method would look like (notice I moved the constants to the helper as well):
/**
* Compare width and height integers to determine image orientation.
*
* @param integer $width
* @param integer $height
*
* @return mixed Orientation string or null.
*
* @since __DEPLOY_VERSION__
*/
public function getOrientation($width, $height)
{
switch (true)
{
case ($width > $height) :
return self::ORIENTATION_LANDSCAPE;
case ($width < $height) :
return self::ORIENTATION_PORTRAIT;
case ($width == $height) :
return self::ORIENTATION_SQUARE;
default :
return null;
}
}
And then change getOrientation() on the JImage class to:
/**
* Method to detect an whether image's orientation is landscape, portrait or square.
* The orientation will be returned as string.
*
* @return mixed Orientation string or null.
*
* @since __DEPLOY_VERSION__
*/
public function getOrientation()
{
if ($this->isLoaded())
{
$width = $this->getWidth();
$height = $this->getHeight();
return JImageHelper::getOrientation($width, $height);
}
return null;
}
Then, in the getImageFileProperties() method, I would add to the $properties object:
$properties = (object) array(
//...snip
'orientation' => JImageHelper::getOrientation((int) $info[0], (int) $info[1]);
);
Perfect! This solution looks smart and satifies both requirements - static access by passing in a path only and access on the loaded resource.
What do you suggest how to proceed? Do you want me to create the JImageHelper file and integrate your suggested solution or do you wanna keep your baby in your hands and integrate was we elaborated?
Add the static getOrientation to JImageHelper, move the class constants over, and make the suggested modifications to JImage->getOrientation() and JImage->getImageFileProperties().
Ok. I searched my current Joomla! installation (3.2.3) for a class JImageHelper and didn't find any, which is why i assume i have to create it. So far i never created new core files and feel little unsure. Please tell me, where you want me to create the helper file? In libraries/cms/helper as class JHelperImage ?
Sorry - I didn't realize the class did not already exist. You would create it at libraries/joomla/image/helper.php.
While you're creating that, can you also move the getImageFileProperties() method to it, then proxy that method in JImage. The JImage method would look like this now: (Leave the other doc-block things, but add the @deprecated as well.
/**
* @deprecated 4.0 Use JImageHelper::getImageFileProperties instead.
*/
public static function getImageFileProperties($path)
{
return JImageHelper::getImageFileProperties($path);
}
Ok, i'm done for the moment. Please have a look and let me know if there's something missing or wrong.
Also, what about the DEPLOY_VERSION string in the docblocks? Is this kind of a placeholder for the deploy script or would i have to substitute it by the current platform version?
Yes, that is a placeholder for the deployment script.
Works like charme now. :-)
Don, we actually don't have that deploy version placeholder setup in the
CMS.
On Wednesday, May 7, 2014, Don Gilbert notifications@github.com wrote:
Yes, that is a placeholder for the deployment script.
—
Reply to this email directly or view it on GitHub#3568 (comment)
.
We should fix that. :)
Since the release commit isn't done by CI, adding a script to local
workflow isn't a big deal. Just remember that the release workflow in the
CMS is more manual work than the Framework :-)
On Wednesday, May 7, 2014, Don Gilbert notifications@github.com wrote:
We should fix that. :)
—
Reply to this email directly or view it on GitHub#3568 (comment)
.
Good solution, should be integrated into the core.
Would you mind to make the fill color for cropped images (not only PNGs) configurable via constructor option and thus customisable? It looks pretty ugly when the page presenting images has a white background color but the cropped image(s) have a black fill color.
I can extend the helper with a method converting color strings like used with CSS into an RGB-array if you like.
@mbabker
Do you want me to replace the placeholders by the currently used numerical platform number?
@itbra Check out my Pull Request #1562, JImageFilterBackgroundfill. Similar has been merged in Joomla-Framework, but CMS is still waiting.
There's a method sanitizeColor for color conversions. I remember Don suggested it may could be moved to a helper class which we didn't have at that moment
Nice. Wonder, why this hasn't found its way into the CMS yet. Processing images is an important functionality to CMS and within the last 10 months it could have been integrated and deployed i think.
I think it just have been forgotten, as the other pull request adding SCALE_FIT got in easily.
I added the filesize information to the returned object of JImageHelper::getImageFileProperties($path) as i found this information quite useful and required.
I guess this could go in 3.4.0. Can we have a new Feature tracker on joomlacode and precise test instructions? Thanks.
| Title |
|
||||||
I created the tracker item and updated this PR's title to reflect the associated tracker item id.
However, i'm afraid i can't provide with any test instructions. I don't know how to do this and depend on the help of our participants. Mabe @dongilbert can help out?
@dongilbert What is your opinion on using abstract class for helpers, like here?
I know it's a convention in the CMS (perhaps to avoid instantiation of such classes) but IMHO it's an misuse of abstract type.
brianteeman
- | Labels | |||
| Labels | |||
nicksavov
- | Labels |
Removed:
?
|
||
| Labels | |||
Can we merge this to 3.4-dev? I have another PR for the helper.php in the pipe but cannot add it as long as this PR is not merged because the file doesn't exist.
Yes, if it's submitted against that branch we can test and merge into 3.4-dev
I'm not sure what you're telling me. Must i repeat all the steps above with a new PR against the 3.4-dev branch? If so, i wonder why since i created a couple other PRs against 3.3 and they've been merged into 3.4-dev. I'm not very experienced in the usage of git and am little confused now.
This PR against the staging branch is fine. We can always commit it to the 3.4-dev branch when merging.
I didn't see at least two good tests yet, but I may be blind.
I have another PR for the helper.php in the pipe but cannot add it as long as this PR is not merged because the file doesn't exist.
You can actually create the class with another PR as well. Once one of the PRs is merged, the other just needs to be updated as it will create a conflict. But it's not a hard thing to do.
Thanks Thomas for your "translation". I wonder nobody tested this yet. Can you start the game and test it soon? I'd like to see this deployed with 3.4.
I think it can be tested easily enough if you want to outline the steps an admin user would take to make sure nothing breaks. It's new code so there's no concern about backwards compatibility.
Maybe just a quick comment outlining what a user should do and what they can expect to happen after they do it. Easy!
I Updated the opening post and added some testing instructions. Note, these are no Unit-Tests. I have no knowledge in Unit-testing and no experience in writing such tests. For the sake of seeing this PR merged to 3.4-dev i ask you guys to test it the quick 'n' dirty way and report your feedback, please.
Anybody experienced in adopting this test into a Unit test is highly welcome to contribute a proper test class to the framework. I suppose this should go into a JImageHelperTest class into this folder.
| Status | New | ⇒ | Pending |
Anyone to take some minutes to test this feature? The testing procedure is well documented at the associated github item and really takes very less time.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3568.
Sorry for the wrong link. Here is the correct one.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3568.
@itbra sorry for the delay.
I have just tested it but i'm not exactly sure if it is a successful test?
I have add your code form here: #3568 (comment)
The result is:
``file: [Joomla Root]/images/powered_by.png orientation from JImage: landscape fileProperties contains width: 1 fileProperties contains height: 1 fileProperties contains type: 1 fileProperties contains attributes: 1 fileProperties contains bits: 1 fileProperties contains channels: 1 fileProperties contains mime: 1 fileProperties contains filesize: 1 fileProperties contains orientation: 1 orientation from JImageHelper: landscape
Is this the correct one?
@test The orientation is returned as expected but I also wonder like @zero-24 if the rest of the info is correct, it looks wrong to me.
On another note, I don't understand why the code is in a helper file and not just in JImage? Is it just because so you can't get the static value of the orientation? To me, it would be nicer to just have one class handling the image information.
@zero-24 and @roland-d thanks for testing. I updated the test instructions to report clearly what's going on. I forgot to update them while discussing and working out all the changes.
@roland-d If you read the whole discussion you'll notice that i initially added my solution to class JImage. However, on 7 May 2014 the original author @dongilbert asked me to move it over to JImageHelper.
@itbra Thanks for the explanation, I saw that @dongilbert asked you to move it to a helper class. I was just asking what the reason behind it is. I don't see the advantage of it.
Neither do I. But since I just wanted to see this feature getting merged soon I just did what I was asked for. Unfortunately it isn't yet merged.
@itbra It wasn't merged because there were no tests, now there are 2 tests by myself and @zero-24 so that is OK. However I do question the usage of the helper file. Either we make it 1 file and merge it like that, otherwise I will consult with the PLT to see if it needs to be a single file or needs a helper file.
Whatever fits and suits the merge requirements.
I would be fine with the helper class. +1
As a rule of thumb, helpers should contain functionality that is common but has no special designation under the overall architecture of the application.
- Suffix the classname with Helper
- Use static methods whenever possible
http://stackoverflow.com/questions/7064867/helper-class-in-php
Developers could easily use the helper classes in their own extensions without having to instantiate a JImage object.
I don't care either way.
JImage is more used to actually change the image in some way. While this proposed code only reads information from the image.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-05-28 14:56:21 |
| Closed_By | ⇒ | roland-d |
well... I don't know and it is now surely too late, but my 2 cents are that a new helper class for 2 methods that could had fit in JImage without breaking anything is... over-engineering
Think it over.... we are creating a legacy...
... and I think all those @since __DEPLOY_VERSION__ are wrong...
Just wondering if it may make more sense to have something like
getOrientation()which returnslandscape,portraitorcubic.In case of the CSS class, you could even directly use the output if you want.