Proposal for #46: Implementation of Image helper based on Imagine library #1669

Merged
merged 19 commits into from Dec 31, 2013

Conversation

Projects
None yet
7 participants
@tonydspaniard
Contributor

tonydspaniard commented Dec 28, 2013

Closes #46

Implements common used methods for Image manipulation:

  • crop
    Yii::$app->image->crop('/path/to/image.jpg', 200, 200, 720, 450);
  • thumb (keeping aspect ratio)
    Yii::$app->image->thumb('/path/to/image.jpg',120, 120);
  • text
    Yii::$app->image->text('/path/to/image.jpg', 'Yii-2 Image', ['font' => 'path/to/font.otf', 'size' => 12, 'color' =>'000']);
  • watermark
    Yii::$app->image->crop('/path/to/image.jpg', /path/to/watermark.jpg' );
  • frame
    Yii::$app->image->frame('/path/to/image.jpg', 5 );

It uses Imagine library. It also contains a method to access Imagine methods directly (ie when used for simple image manipulations):

Yii::$app->image->getImagine()
   ->open('/path/to/image.jpg')
   ->save('/path/to/image.jpg', array('quality' => 50));

Edit: This extension is on WIP, methods are proven to work, if accepted. Then will write the tests

tonydspaniard added some commits Dec 20, 2013

Merge remote-tracking branch 'upstream/master'
Conflicts:
	extensions/yii/jui/CHANGELOG.md
Merge remote-tracking branch 'origin/master'
* origin/master:
  Update change log
  Add active id to options if input widget has a model fixes #1550
Merge branch 'upstream' into 46-image-helper
* upstream:
  Fixed CSRF token masking issue.
  improved error message of calling invalid scope method.
  Fixed repo URL
  Fixes #1650: Added Connection::pdoClass.
  code style fix.
  added changelog
  codestyle fix
  improved checkIntegrity method
  Modified extension guidlines
  fix sphinx command signature
  fixed bug with forgotten param, fixed behavior for one table integrity
  fixed sequence reset
  added postgresql features to reset seq/check integrity
Merge branch 'upstream'
* upstream:
  Fixed CSRF token masking issue.
  improved error message of calling invalid scope method.
  Fixed repo URL
  Fixes #1650: Added Connection::pdoClass.
  code style fix.
  added changelog
  codestyle fix
  improved checkIntegrity method
  Modified extension guidlines
  fix sphinx command signature
  fixed bug with forgotten param, fixed behavior for one table integrity
  fixed sequence reset
  added postgresql features to reset seq/check integrity
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 28, 2013

Member

Looks good to me. @yiisoft/core-developers What's your opinion?

Do you plan to implement more commonly used methods?

Member

qiangxue commented Dec 28, 2013

Looks good to me. @yiisoft/core-developers What's your opinion?

Do you plan to implement more commonly used methods?

@tonydspaniard

This comment has been minimized.

Show comment
Hide comment
@tonydspaniard

tonydspaniard Dec 28, 2013

Contributor

@qiangxue the ones included are the most common used. If @yiisoft/core-developers have any suggestions that cannot be easily handled with Imagine would be great to hear it.

Contributor

tonydspaniard commented Dec 28, 2013

@qiangxue the ones included are the most common used. If @yiisoft/core-developers have any suggestions that cannot be easily handled with Imagine would be great to hear it.

+ /**
+ * @return array of available drivers.
+ */
+ public function getAvailableDrivers()

This comment has been minimized.

@creocoder

creocoder Dec 28, 2013

Contributor

@tonydspaniard About this method implementation. Why not just:

return [static::DRIVER_GD2, static::DRIVER_GMAGICK, static::DRIVER_IMAGICK];

For which goals you use excess/static variable

@creocoder

creocoder Dec 28, 2013

Contributor

@tonydspaniard About this method implementation. Why not just:

return [static::DRIVER_GD2, static::DRIVER_GMAGICK, static::DRIVER_IMAGICK];

For which goals you use excess/static variable

This comment has been minimized.

@tonydspaniard

tonydspaniard Dec 28, 2013

Contributor

@creocoder fast typing... fixing

Any other methods to implement?

@tonydspaniard

tonydspaniard Dec 28, 2013

Contributor

@creocoder fast typing... fixing

Any other methods to implement?

This comment has been minimized.

@creocoder

creocoder Dec 28, 2013

Contributor

@tonydspaniard

Any other methods to implement?

Will think about that.

@creocoder

creocoder Dec 28, 2013

Contributor

@tonydspaniard

Any other methods to implement?

Will think about that.

extensions/yii/imagine/Image.php
+ * @return ManipulatorInterface
+ */
+ public function watermark($filename, $watermarkFilename, Point $pos = null)
+ {

This comment has been minimized.

@klimov-paul

klimov-paul Dec 28, 2013

Member

Different methods interface looks inconstant: here you pass coordinates as an object, while in “crop()” you are using separated $startX, $startY.

@klimov-paul

klimov-paul Dec 28, 2013

Member

Different methods interface looks inconstant: here you pass coordinates as an object, while in “crop()” you are using separated $startX, $startY.

extensions/yii/imagine/Image.php
+ * @return \Imagine\Image\ImageInterface|ManipulatorInterface
+ */
+ public function thumb($filename, $width, $height, $mode = ManipulatorInterface::THUMBNAIL_OUTBOUND)
+ {

This comment has been minimized.

@klimov-paul

klimov-paul Dec 28, 2013

Member

I do not think short name is appropriate here. Better use full “thumbnail”, keeping consistency with the “ImageInterface”.

@klimov-paul

klimov-paul Dec 28, 2013

Member

I do not think short name is appropriate here. Better use full “thumbnail”, keeping consistency with the “ImageInterface”.

This comment has been minimized.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Dec 28, 2013

Member

Any other methods to implement?

Maybe “rotate()”.

Member

klimov-paul commented Dec 28, 2013

Any other methods to implement?

Maybe “rotate()”.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 28, 2013

Member

What about resize()? Does thumbnail() do the same thing?

Member

qiangxue commented Dec 28, 2013

What about resize()? Does thumbnail() do the same thing?

@tonydspaniard

This comment has been minimized.

Show comment
Hide comment
@tonydspaniard

tonydspaniard Dec 28, 2013

Contributor

@qiangxue resize() is something very simple to do with access to getImagine(), not worth a method here.

Maybe “rotate()”.

Same goes with rotation. Check results of the following code (frame it and then rotate -8 degrees):

Yii::$app->image->frame($imgPath, 5, '666', 0)
    ->rotate(-8)
    ->save($runtime . '/123456789.jpg', ['quality' => 50]);

123456789

Contributor

tonydspaniard commented Dec 28, 2013

@qiangxue resize() is something very simple to do with access to getImagine(), not worth a method here.

Maybe “rotate()”.

Same goes with rotation. Check results of the following code (frame it and then rotate -8 degrees):

Yii::$app->image->frame($imgPath, 5, '666', 0)
    ->rotate(-8)
    ->save($runtime . '/123456789.jpg', ['quality' => 50]);

123456789

extensions/yii/imagine/Image.php
+ private $_imagine;
+ /**
+ * @var string the driver to use. These can be:
+ * - gd2

This comment has been minimized.

@samdark

samdark Dec 28, 2013

Member

Better to provide constants in example or people will use strings directly.

@samdark

samdark Dec 28, 2013

Member

Better to provide constants in example or people will use strings directly.

+ {
+ if (!is_string($driver) || !in_array($driver, $this->getAvailableDrivers(), true)) {
+ throw new InvalidConfigException(
+ strtr('"{class}::driver" should be string of these possible options "{drivers}", "{driver}" given.', [

This comment has been minimized.

@samdark

samdark Dec 28, 2013

Member

"{drivers}" becomes "gd2, imagick". I think it should be "gd2", "imagick".

@samdark

samdark Dec 28, 2013

Member

"{drivers}" becomes "gd2, imagick". I think it should be "gd2", "imagick".

extensions/yii/imagine/composer.json
+ ],
+ "require": {
+ "yiisoft/yii2": "*",
+ "imagine/imagine": "v0.5.0"

This comment has been minimized.

@samdark

samdark Dec 28, 2013

Member

Formatting issue?

@samdark

samdark Dec 28, 2013

Member

Formatting issue?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 28, 2013

Member

Looks OK to me overall.

Member

samdark commented Dec 28, 2013

Looks OK to me overall.

extensions/yii/imagine/Image.php
+ * - imagick
+ * - gmagick
+ */
+ private $_driver = "gd2";

This comment has been minimized.

@creocoder

creocoder Dec 28, 2013

Contributor

"gd" => 'gd'

@creocoder

creocoder Dec 28, 2013

Contributor

"gd" => 'gd'

@kmergen

This comment has been minimized.

Show comment
Hide comment
@kmergen

kmergen Dec 29, 2013

How can I implement extension in my project. Can not find extension in extensions directory

kmergen commented Dec 29, 2013

How can I implement extension in my project. Can not find extension in extensions directory

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Dec 29, 2013

Contributor

@kmergen This is not merget yet.

Contributor

creocoder commented Dec 29, 2013

@kmergen This is not merget yet.

extensions/yii/imagine/Image.php
+ * @param integer $y position on image to apply watermark. Defaults to null
+ * @return ManipulatorInterface
+ */
+ public function watermark($filename, $watermarkFilename, $x = null, $y = null)

This comment has been minimized.

@klimov-paul

klimov-paul Dec 29, 2013

Member

Maybe it is better to pass coordinates as an array. Single argument will simplify the interface as well as cover possible pass of “Point” object:

public function watermark($filename, $watermarkFilename, $point = null)
{
    if (is_array($point)) {
        list($x, $y) = $point;
        $point = new Point($x, $y);
    } elseif ($point instanceof Point) {
        // do nothing  
    }
}

Same strategy can be applied for all methods.

@klimov-paul

klimov-paul Dec 29, 2013

Member

Maybe it is better to pass coordinates as an array. Single argument will simplify the interface as well as cover possible pass of “Point” object:

public function watermark($filename, $watermarkFilename, $point = null)
{
    if (is_array($point)) {
        list($x, $y) = $point;
        $point = new Point($x, $y);
    } elseif ($point instanceof Point) {
        // do nothing  
    }
}

Same strategy can be applied for all methods.

This comment has been minimized.

@cebe

cebe Dec 29, 2013

Member

👍

@cebe

cebe Dec 29, 2013

Member

👍

tonydspaniard added some commits Dec 29, 2013

Merge branch 'upstream' into 46-image-helper
* upstream: (35 commits)
  Fixes #1691: added “viewport” meta tag to layout views.
  Fixed the issue that query cache returns the same data for the same SQL but different query methods
  moved section
  subsection added
  typo [skip ci]
  docs about response
  Fixes #1586: `QueryBuilder::buildLikeCondition()` will now escape special characters and use percentage characters by default
  docs improved
  csrf docs added
  Fixes #1685: UrlManager::showScriptName should be set true for tests.
  Fixes #1688: ActiveForm is creating duplicated messages in error summary
  change back the visibility of findTableNames to protected.
  Typo fix.
  Fixes #1681: Added support for automatically adjusting the "for" attribute of label generated by `ActiveField::label()`
  Simplified tests.
  Fixes #1631: Charset is now explicitly set to UTF-8 when serving JSON
  Fixed typo
  added html layout for mail component in basic app
  CS fixes.
  Merge branch 'debug_module_improvements' of github.com:Ragazzo/yii2 into Ragazzo-debug_module_improvements
  ...

@qiangxue qiangxue merged commit 52ad66e into yiisoft:master Dec 31, 2013

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 31, 2013

Member

merged 2df7d0a

Member

qiangxue commented Dec 31, 2013

merged 2df7d0a

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 31, 2013

Member

I just review the code. It seems to me the only configurable option is which driver to use. For this reason, how about we turn this into a static helper instead of an application component? The helper can automatically choose which driver to use based on the installed PHP extensions.

Member

qiangxue commented Dec 31, 2013

I just review the code. It seems to me the only configurable option is which driver to use. For this reason, how about we turn this into a static helper instead of an application component? The helper can automatically choose which driver to use based on the installed PHP extensions.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jan 1, 2014

Member

Sounds good to me. We can have a static property for the driver that is null for autodetect and may be set explicitly.

Member

cebe commented Jan 1, 2014

Sounds good to me. We can have a static property for the driver that is null for autodetect and may be set explicitly.

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Jan 1, 2014

Contributor

@cebe Which order for autodetection:

1) Gmagick
2) Imagick
3) Gd2

or

1) Imagick
2) Gmagick
3) Gd2

?

Contributor

creocoder commented Jan 1, 2014

@cebe Which order for autodetection:

1) Gmagick
2) Imagick
3) Gd2

or

1) Imagick
2) Gmagick
3) Gd2

?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jan 1, 2014

Member

Gmagick, Imagick, Gd2 according to performance and efficiency.

Member

samdark commented Jan 1, 2014

Gmagick, Imagick, Gd2 according to performance and efficiency.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 1, 2014

Member

Finished refactoring.

Member

qiangxue commented Jan 1, 2014

Finished refactoring.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 1, 2014

Member

@tonydspaniard The frame() method doesn't look quite right to me. Currently, the frame is added to the outside of the original image. The $alpha value doesn't make much sense then. IMO, the frame should be added to the inside of the image with alpha value indicating the transparency of the frame. By doing so, we also keep the image size unchanged, which is probably preferable in practice. What do you think?

Member

qiangxue commented Jan 1, 2014

@tonydspaniard The frame() method doesn't look quite right to me. Currently, the frame is added to the outside of the original image. The $alpha value doesn't make much sense then. IMO, the frame should be added to the inside of the image with alpha value indicating the transparency of the frame. By doing so, we also keep the image size unchanged, which is probably preferable in practice. What do you think?

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jan 1, 2014

Member

margin could be positive for inside frame or negative for outside frame. alpha should have no effect on outside frame then.

For the text() method I think we should add a position parameter that can be a point or array as it is in the other methods. it is quite uncommon that the position will be default.

Member

cebe commented Jan 1, 2014

margin could be positive for inside frame or negative for outside frame. alpha should have no effect on outside frame then.

For the text() method I think we should add a position parameter that can be a point or array as it is in the other methods. it is quite uncommon that the position will be default.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jan 1, 2014

Member

rest of the code looks really cool to me :)

Member

cebe commented Jan 1, 2014

rest of the code looks really cool to me :)

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 2, 2014

Member

I have refactored the text() method as you suggested.

I like your suggestion about the frame margin. I don't know how to implement the inside frame though. @tonydspaniard Do you have any idea?

Member

qiangxue commented Jan 2, 2014

I have refactored the text() method as you suggested.

I like your suggestion about the frame margin. I don't know how to implement the inside frame though. @tonydspaniard Do you have any idea?

@tonydspaniard tonydspaniard deleted the tonydspaniard:46-image-helper branch Feb 5, 2014

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