Version: 7.x
Sandbox: http://drupal.org/sandbox/OxideInteractive/1817208
Git clone: git clone --recursive --branch 7.x-1.x OxideInteractive@git.drupal.org:sandbox/OxideInteractive/1817208.git pointcrop
The pointcrop module is for smart cropping of images using drupal image styles.
The module provides 2 things:
- The ability to select a single point on an image field image to use as the focal point when cropping.
- "Pointcrop scale and crop" and "Pointcrop crop" image effects, which use that focal point.
The reason this module was created was to be able to have images that display in vastly different sizes and in different orientations, but always keep the subject in the frame.
There are a number of other image cropping modules which were reviewed before creating this one.
- Smart crop: Tries to focus automatically based on entropy. This doesn't work well in practice.
- Imagefield crop, Imagefield focus, Manual crop, Image javascript crop: These modules are based around the idea of the user being able to select a rectangle/crop area for uploaded images, which is then used for cropping. This is a more complex solution that doesn't give as much flexibility in terms of the size and orientation of the final image.
There is also a jQuery plugin (https://github.com/oxideinteractive/pointcrop), that is able to use the focus points this module provides to do dynamic cropping on the front end.
This means image size & orientation can change dynamically and the images will still also focus on the focal point.
Integration with this plugin will be provided out of the box in future, most likely in the form of a views plugin.
A demo site will very likely be set up for the plugin.
Screenshots will also soon be added to the project page, demonstrating pointcropped images vs regular cropped images.
Review bonus
#1907552: PHP Execute Extended Tools (rooby)
Comments
Comment #1
ramsonkr CreditAttribution: ramsonkr commentedquick review:
There are many issues related to indentations, extra spaces etc
http://ventral.org/pareview/httpgitdrupalorgsandboxoxideinteractive18172...
Focus point description,The point consists of x and y values of between 0 and 1
provide an example value.
Comment #2
bripatand CreditAttribution: bripatand commentedManual review:
Nice module. See to work as expected after some basic testing.
Beside the few errors reported before by pareview, the code is well written and make appropriate use of the Drupal hooks according to me.
You may want to use two fields instead of one field with two values separated by a comma.
Comment #3
OxideInteractive CreditAttribution: OxideInteractive commentedThanks for the reviews.
I have pushed the coding standards changes from #1 in now.
I have run it through drush dcs now and it got no errors.
- As an aside, there is a lot of mention of using the coder module so that you don't have these kinds of time wasting issues during this process. However the current recommended release of coder does not catch these things, so it is a bit misleading.
In regards to one DB field vs two, I will have a think about that.
I was thinking seeing as they are coordinates that come as a set, they are fine as a single value, however there could be a chance that someday someone finds it useful to have the numbers stored in the database as decimals instead of varchar, so that they can do math with the numbers more easily.
I guess if it is going to change now would be the best time for such a change.
Comment #4
sittard CreditAttribution: sittard commentedNice module, well coded and documented. I tested it on a image library site that currently uses Imagefield focus and found that it produces good results. The functionality is really simple and I would imagine even a non technical content manager would have no problem quickly setting the focus point.
Some mainly non technical feedback:
Overall a really nice clean module and I'd be keen to see this module get released. Good work!
Comment #5
rooby CreditAttribution: rooby commentedI am posting as a committer on the pointcrop project.
Thanks for the review. I will action the code related items in the next couple of days, but for now:
1. That was considered, however it was decided against for a couple of reasons. One being that, while similar, they are both doing different things, and the other is that integration with the jQuery plugin is planned, at which point it really is a module of it's own, and there would be no need to have the imagefield_focus functionality.
2. There is already some instructions in the README (which is also linked to from the documentation link on the project page). Detailed instructions are not necessary on the project page itself, that is what the documentation is for.
3. It is wrong to include a version number in the .info file. Drupal.org handles that automatically.
4. I will look into fixing that.
5. I see your point. It might be that it changes to two separate fields, as per #2 & #3, in which case it would be easier to follow.
6. I will add something to make that clear.
7. The field is visible currently due to the fact that you can also set the point manually in the field if you want to be more precise. Maybe it could be an option of the field settings form to show or hide it. I might do that.
8. It is noted that there is no out of the box implementation of the plugin yet (it will be coming), however I will try to make it more clear.
Thanks again for the review and I will fix the actionable items soon.
Comment #6
willvincent CreditAttribution: willvincent commentedPlease also consider: #1831570: Compatibility with media module?. The available cropping modules that work with media (as far as I can tell) are few & far between. Given that the implementation is fairly straightforward, and huge popularity of the media module, including support for it would be beneficial, both for users of the media module, and the success of this module.
Comment #7
anrikun CreditAttribution: anrikun commentedImageField Focus does this already.
Comment #8
rooby CreditAttribution: rooby commented@anrikun:
Based on that quote, I see now that it does provide a very similar thing. If you ignore the crop rectangle.
In fact, the real #1 reason it was created was to be able to more easily achieve this - http://www.canberra.edu.au/alumni/photo-archive
Where images of any dimensions can be uploaded by anonymous users.
They are then shown on that page, with a complex, semi date based sort, in a way that they could appear anywhere, at almost any size (depending on where they appear and how many images are around them).
Navigation, cropping, placement, etc. is all on the front end and images must be pre-image-cached, before they are pointcropped in the front end.
When you filter by searching, the same images can appear in different places, at different sizes.
So there are some differences between the two modules:
1. The module is a drupal end for the PointCrop plugin. https://github.com/oxideinteractive/pointcrop
Basically to make it easy to get the data required by the plugin out from drupal.
It would be possible to get the same data out of imagefield focus, with a little bit more work.
The jQuery plugin was the #1 purpose, when this module was originally created.
I see that there is an imagefield focus plugin coming soon also, which will be another way these modules differ.
It is common to have different drupal modules that provide integration with different jQuery plugins for similar purposes.
2. This module has a simpler interface. Both for admins (field settings) and end users (focus selection).
For the purpose of crop focusing, you only need a single point, and it is preferred to keep the interface as simple as possible for users to avoid confusion. An option to hide the text field will also make this even more streamlined.
3. When the user is able to select a rectangle, they are given influence over the scaling & cropping.
If they can only possibly select a single point they cannot affect the result.
There are two sides to this:
One is that when selecting a rectangle, they can select a vastly different rectangle than where the image will appear elsewhere on the site. In this case the image might have to be compromised when it is made to fit in the target space.
For example, on a 1000x1000 image, a portion 1000x100 is selected in the middle. If that selection has to be placed into an area 200x1000, you cannot easily get that selection to fit into that area.
The other side is that when selecting a single point, in the case of the 1000x1000 original, the user has to select a single point, which can then easily be placed into the 1000x200 space. The tradeoff is that if the focus point is a bunch of people in a line all the way across that image, the user is forced to select a single point and people outside that point could be cropped out, however there will still be a reasonable focus point.
So in that way the functionality is actually different.
4. Storage is different. This module stores a ratio, not actual x,y coords.
This means that when the jQuery plugin comes to the image, even if it is not the same size it was when the focus point was selected, the ratio will still be valid.
If the original image is 1000x1000; and the focus point is 500,500; and then you have to pointcrop a 100x100 version of the image on the front end; the 500,500 coordinate has just become useless.
And in this case you don't want to be outputting the original 1000x1000, because in this case it is guaranteed that no image will be over 100x100, so pushing out a hundred 1000x1000 images when you don't need to is unnecessary slowness.
Another bonus to storing as a ratio, is that on the node edit form, images can be scaled down to fit in the width of the form, without affecting the ratio.
So although these modules on the surface have a very similar purpose, there are a number of important differences.
It would be possible to merge the two modules, however due to these differences you would either have to change one of the modules functionality somewhat to fit nicely in with the other one, or else just be smooshing two different modules together into one.
I'm all for not duplicating efforts, however if there is difference enough I don't see the harm. Especially if bugs are getting fixed.
Also, these are not modules that require a huge amount of man hours to keep maintained, so I can't see there being a situation where both modules have hundreds of issues and everyone would be better off if everyone was working on a single issue queue.
For example, we have TAC, TAC Lite, content access, node access, etc.
These modules all provide access control, but in different ways.
These two modules provide focused cropping, but in different ways.
Comment #9
anrikun CreditAttribution: anrikun commentedI didn't mean that this was a duplicate effort. I just wanted to point out that there is already a module that what made for the same purpose, even if the way the 2 modules handle the task may be different.
Everybody is free to make modules, and if you think that yours might help users, you are welcome. Users are then free to pick up the one that best suit their needs :-)
Comment #10
adnasa CreditAttribution: adnasa commentedjust a general question because I need to learn this also
The following line
Shouldn't be enough to add the
pointcrop.effects.inc
in .info file along with the files[] arrayso that it is automatically included?
Comment #11
willvincent CreditAttribution: willvincent commentedMy understanding is that the autoloader is for classes, the contents of the pointcrop.effects.inc file is procedural code.
I could be wrong, but that's been my understanding on how registering files with the files[] array in the .info file works.
Comment #12
anrikun CreditAttribution: anrikun commented@adnasa: willvincent is right, the files[] array in the .info file is for classes only.
Comment #13
adnasa CreditAttribution: adnasa commented#11: @willvincent, thanks for pointing that out! this is good info :-)
Comment #14
rooby CreditAttribution: rooby commented@anrikun:
No worries.
I guess it is good to have the differences listed any way as people will wonder.
Comment #15
klausiClosing due to lack of activity. Feel free to reopen if you are still working on this application.
If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)
Comment #16
rooby CreditAttribution: rooby commentedThis is definitely still being worked on.
Will try to do some reviews too.
Comment #17
PA robot CreditAttribution: PA robot commentedClosing due to lack of activity. Feel free to reopen if you are still working on this application.
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #18
rooby CreditAttribution: rooby commentedStill going on this...
Comment #19
PA robot CreditAttribution: PA robot commentedClosing due to lack of activity. Feel free to reopen if you are still working on this application.
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #20
rooby CreditAttribution: rooby commentedThis still isn't dead.
Comment #21
willvincent CreditAttribution: willvincent commentedI really enjoy using this module, and while it is still only a sandbox project that hasn't been granted permission for promotion to a full project, I use it quite regularly on client sites. It's been my experience that its simple interface provides all the necessary power for focusing the subject of an image to be cropped, and as it's based on a single point is the most flexible of all the other options that aim to provide similar functionality currently on D.O.
For these reasons, and as per #3 indicating that all specified coding standard issues have been addressed, I'm going to mark this RTBC, and recommend it be accepted.
It works as advertised, is lightweight, and much more flexible than existing options.
Comment #22
rooby CreditAttribution: rooby commentedThanks Will for the review, it's good to see that people are using this.
I also use it on a couple of sites that have been live for a long time.
The more I think about the remaining tasks in this issue the more I think they don't belong here.
Point 4 from comment #4 is a bug that needs fixing.
The rest are pretty much just feature requests and potential feature requests, which is not what this process is about.
I also added a project I reviewed.
I know it's only 1 so far and the project has gone cold but it was a thorough review.
I actually really still want to see that module so I will try to push the maintainer to get it out.
I was so close to RTBCing it too.
Comment #23
rooby CreditAttribution: rooby commentedI have added a bug report to the pointcrop issue queue for point 4 of #4.
I will add issues for the feature requests soon.
Comment #24
abhitesh.das CreditAttribution: abhitesh.das commentedChanged Issue title. FYI: http://drupal.org/node/1011698
Comment #25
stBorchertHi.
There are still several errors you need to fix before this can be flagged as "reviewed and tested":
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please change your account information and enter your realname.
Please keep in mind that we are currently quite busy with all the project applications and we can only review projects with a review bonus. Please help us reviewing and we'll take a look at your project right away.
Comment #26
PA robot CreditAttribution: PA robot commentedClosing due to lack of activity. Feel free to reopen if you are still working on this application.
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #26.0
PA robot CreditAttribution: PA robot commentedAdded a link to a project that was reviewed.