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

Should icon service always save 'original' image size? #9970

Closed
juho-jaakkola opened this Issue Jul 11, 2016 · 9 comments

Comments

Projects
None yet
3 participants
@juho-jaakkola
Member

juho-jaakkola commented Jul 11, 2016

The EntityIconService has a minor problem. Every time an icon is cropped, the cropping affects all image sizes including the largest 'master' size. This causes the image quality of the master icon to deteriorate every time the icon is cropped.

Perhaps we should always save also the original image with the "size" original. It would be created regardless if it were included in the image size configuration (returned by the icon size hook). Sites could optionally define a size for it, but it would not be affected by cropping. This would make sure there's always a high-quality version of each image available.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Jul 11, 2016

Member

i think it should... this would also help if bigger resolutions are added in a future config and you need to recreate the icons

Member

jdalsem commented Jul 11, 2016

i think it should... this would also help if bigger resolutions are added in a future config and you need to recreate the icons

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 11, 2016

Contributor

I think my original design had that, but I eventually got rid of it. Can't remember why.

Contributor

hypeJunction commented Jul 11, 2016

I think my original design had that, but I eventually got rid of it. Can't remember why.

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Jul 11, 2016

Member

Well, there's always the risk that someone uploads a ridiculously large image.

Member

juho-jaakkola commented Jul 11, 2016

Well, there's always the risk that someone uploads a ridiculously large image.

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Jul 11, 2016

Member

Perhaps we could allow passing in an image size config that would skip the resize/crop part and save the image without modification.

That way the original image could get saved only for the image types that need for it.

Member

juho-jaakkola commented Jul 11, 2016

Perhaps we could allow passing in an image size config that would skip the resize/crop part and save the image without modification.

That way the original image could get saved only for the image types that need for it.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 11, 2016

Contributor

One option is to calculate if cropping coordinates are a square and decide
based on that which sizes to crop
On Jul 11, 2016 8:03 PM, "Juho Jaakkola" notifications@github.com wrote:

Perhaps we could allow passing in an image size config that would skip the
resize/crop part and save the image without modification.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#9970 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABJaSdsrYww74RRlP-LU9uLJ-4Z4pbdeks5qUoWMgaJpZM4JJP0u
.

Contributor

hypeJunction commented Jul 11, 2016

One option is to calculate if cropping coordinates are a square and decide
based on that which sizes to crop
On Jul 11, 2016 8:03 PM, "Juho Jaakkola" notifications@github.com wrote:

Perhaps we could allow passing in an image size config that would skip the
resize/crop part and save the image without modification.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#9970 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABJaSdsrYww74RRlP-LU9uLJ-4Z4pbdeks5qUoWMgaJpZM4JJP0u
.

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Jul 12, 2016

Member

One option is to calculate if cropping coordinates are a square and decide
based on that which sizes to crop

That won't help with custom image types. They could have any aspect ratio.

How about skipping crop/resize if the configuration value is e.g. an empty array:

array(
    'medium' => array('w' => 100, 'h' => 100, 'square' => true, 'upscale' => true),
    'large' => array('w' => 200, 'h' => 200, 'square' => false, 'upscale' => false),
    'original' => array()
);
Member

juho-jaakkola commented Jul 12, 2016

One option is to calculate if cropping coordinates are a square and decide
based on that which sizes to crop

That won't help with custom image types. They could have any aspect ratio.

How about skipping crop/resize if the configuration value is e.g. an empty array:

array(
    'medium' => array('w' => 100, 'h' => 100, 'square' => true, 'upscale' => true),
    'large' => array('w' => 200, 'h' => 200, 'square' => false, 'upscale' => false),
    'original' => array()
);
@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 12, 2016

Contributor

Fine by me. I just don't want duplicate files on the filestore.

Contributor

hypeJunction commented Jul 12, 2016

Fine by me. I just don't want duplicate files on the filestore.

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Jul 13, 2016

Member

What are the main use cases for creating an image with saveIconFromElggFile() so that it creates a dupe? Something like you're browsing an image album and use a "Set this as a profile icon" feature?

I would guess in most cases it is appropriate to make a duplicate. At least I personally would like to have a copy in case the original source image gets removed.

Member

juho-jaakkola commented Jul 13, 2016

What are the main use cases for creating an image with saveIconFromElggFile() so that it creates a dupe? Something like you're browsing an image album and use a "Set this as a profile icon" feature?

I would guess in most cases it is appropriate to make a duplicate. At least I personally would like to have a copy in case the original source image gets removed.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 13, 2016

Contributor

Give me some code to read ;)

Contributor

hypeJunction commented Jul 13, 2016

Give me some code to read ;)

juho-jaakkola added a commit to juho-jaakkola/Elgg that referenced this issue Jul 14, 2016

feature(iconservice): it is possible to save unaltered version of an …
…image

The EntityIconService now saves an unaltered version (no cropping or resizing)
of the image if the configuration for the image size is an empty array. This
makes it possible to always have access to a high quality version of the image.

Fixes #9970

juho-jaakkola added a commit to juho-jaakkola/Elgg that referenced this issue Jul 14, 2016

feature(iconservice): it is possible to save unaltered version of an …
…image

The EntityIconService now saves an unaltered version (no cropping or resizing)
of the image if the configuration for the image size is an empty array. This
makes it possible to always have access to a high quality version of the image.

Fixes #9970

juho-jaakkola added a commit to juho-jaakkola/Elgg that referenced this issue Jul 14, 2016

feature(iconservice): it is possible to save unaltered version of an …
…image

The EntityIconService now saves an unaltered version (no cropping or resizing)
of the image if the configuration for the image size is an empty array. This
makes it possible to always have access to a high quality version of the image.

Fixes #9970

juho-jaakkola added a commit to juho-jaakkola/Elgg that referenced this issue Jul 18, 2016

feature(iconservice): it is possible to save unaltered version of an …
…image

The EntityIconService now saves an unaltered version (no cropping or resizing)
of the image if the configuration for the image size is an empty array. This
makes it possible to always have access to a high quality version of the image.

Fixes #9970

@mrclay mrclay closed this in 7157a33 Jul 19, 2016

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