Skip to content
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

[#33765] Add fast check for image orientation #3568

Closed
wants to merge 28 commits into from

Conversation

Projects
None yet
@itbra
Copy link
Contributor

commented May 6, 2014

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.

Add fast check for image orientation
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.

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().

Any feedback is welcome.
@Bakual

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

Just wondering if it may make more sense to have something like getOrientation() which returns landscape, portrait or cubic.
In case of the CSS class, you could even directly use the output if you want.

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2014

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?

@Bakual

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

I'd wait a day and see if someone else has an issue with it.

@betweenbrain

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

Agree. The generic method indeed provides more flexibility.

👍

@chrisdavenport

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

Shouldn't that be "square" rather than "cubic"? I don't think we support 3D printers yet. ;-)

@Bakual

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

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 👍

@bembelimen

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

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.

@piotr-cz

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

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;
    }
}
@Fedik

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

one method looks better 👍

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

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.

itbra added some commits May 7, 2014

Update image.php
Replace 3 separate methods (isCubic, isPortrait, isLandscape) by one generic method getOrientation.
Update image.php
Added note
@piotr-cz

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

@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";
}
Update image.php
Remove keyword static from method signature and replaced the check for a loaded resource.
@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

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:

  1. Create a new instance of JImage not passing in anything to the constructor and assign it to $jimage.
  2. 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.).
  3. 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.
  4. Evaluate the returned value like:
$isLandscape = $jimage->getOrientation() == JImage::ORIENTATION_LANDSCAPE;
$isPortrait = $jimage->getOrientation() == JImage::ORIENTATION_PORTRAIT;
$isSquare = $jimage->getOrientation() == JImage::ORIENTATION_SQUARE;
  1. 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:

  1. Create a new instance of JImage not passing in anything to the constructor and assign it to $jimage.
  2. Iteratively check whether the file exists and call $jimage->loadFile($file). This loads the uploaded file ready for processing (resice, scale, crop, etc.).
  3. Dump the orientation $orientation = $jimage->getOrientation()
  4. 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.

@piotr-cz

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

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.

@sovainfo

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

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);

@bembelimen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

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

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

You convinced me. I updated my previous comment and the PR.

I think we are good to go for testing and merging.

itbra added some commits May 7, 2014

Update image.php
Added new constants reflecting an image's orientation. These can also be used to easily assign the orientation as a string literal to e.g. a CSS class.
Replaced the return values in getOrientation() by the new constants.

// @codeCoverageIgnoreEnd
}

This comment has been minimized.

Copy link
@piotr-cz

piotr-cz May 7, 2014

Contributor

I'd use now

    $width = (int) $info[0];
    $height = (int) $info[1];
}
else if ($this->isLoaded)
{
    $width = $this->getWidth();
    $height = $this->getHeight();
}
else
{
    return null;
}

// Determine format
switch (true)
//...

so we are not repeating logic

This comment has been minimized.

Copy link
@itbra

itbra May 7, 2014

Author Contributor

@piotr-cz
I see your point, but there is good reason for the implementation variance between the if-block and the ifelse-block. Note, that within the if-block the JImage-instance has not loaded any file and thus no pointer to a gd-resource. Within this block the file-information is directly accessed from the related file via getimagezie(), while in the ifelse-block the information is fetches from the loaded resource.
Ofcourse one could load the file passed in and fetch the data from the loaded resource. But i considered to no create a resource when there is no need for it. But i'm open for your arguments.

Regarding switch/case:
I also considered to use a it but found it harder to read and impossible to wrap in my editor whereas with the if/ifelse/else block each part can be wrapped making it easier to hide parts of the code and focus on a specific portion.

If i got you completely wrong, let me know.

This comment has been minimized.

Copy link
@dongilbert

dongilbert May 7, 2014

Contributor

If you take the suggestion I added in my comment, it would resolve the issues and remove the duplication.

@dongilbert

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

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]);
);
@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

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?

@dongilbert

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

Add the static getOrientation to JImageHelper, move the class constants over, and make the suggested modifications to JImage->getOrientation() and JImage->getImageFileProperties().

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

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 ?

@dongilbert

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

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);
}
Update image.php
Slight changes to the function descriptions
@itbra

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2014

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.

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

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.

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

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.

@zero-24

This comment has been minimized.

Copy link
Contributor

commented May 9, 2015

@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?
@roland-d

This comment has been minimized.

Copy link
Contributor

commented May 14, 2015

@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.

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2015

@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.

@zero-24

This comment has been minimized.

Copy link
Contributor

commented May 19, 2015

@test successful with the last stat of the test instructions. Thanks @itbra

@roland-d

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

@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.

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2015

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.

@roland-d

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

@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.

@itbra

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2015

Whatever fits and suits the merge requirements.

@roland-d

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

@wilsonge @Bakual Can you guys please answer if the change should be in a helper file or the main JImage file? My preference goes to the main JImage file.

@Kubik-Rubik

This comment has been minimized.

Copy link
Member

commented May 28, 2015

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.

@Bakual

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

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.

@roland-d

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

Thanks guys, merged in 449b907

@roland-d roland-d closed this May 28, 2015

@smz

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

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...

@smz

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

... and I think all those @since __DEPLOY_VERSION__ are wrong...

@Bakual

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

@smz Can you do a PR please? It's never to late to change something as long as it isn't released yet.
The __DEPLAY_VERSION__ stuff sure is wrong.

@smz

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

@Bakual Of course! Will do ASAP (later today...)

@smz

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

New PR fixing some issues in this: #7060

@zero-24 zero-24 added this to the Joomla! 3.4.2 milestone May 29, 2015

Bakual added a commit that referenced this pull request Jun 3, 2015

johanjanssens added a commit to joomlatools/joomla-fork that referenced this pull request Jun 19, 2015

mbabker added a commit to joomla-framework/image that referenced this pull request Aug 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.