Hello and thanks for the distribution, I am learning a lot :)

I have an Open Atrium installation and I was using Manualcrop on images styles. I got this error https://www.drupal.org/node/2092911 that now is solved in the last dev. Also about the patches that you apply on Manualcrop

- http://drupal.org/files/issues/manualcrop-file_entity-settings-1665130-1... //it is commited in last dev
- http://drupal.org/files/issues/manualcrop-two-crop-tools-2237835.patch //needed as @dsnopek said in #3
- http://drupal.org/files/issues/manualcrop-make-file-2248587.patch //it is commited in last dev

This is also also needed
- https://www.drupal.org/files/issues/manual_crop-fix-for-array-flip-22924... //needed it is commited in last dev

Thanks again :)

Comments

dsnopek’s picture

Category: Feature request » Task
Status: Active » Needs review
Issue tags: +Needs manual testing
StatusFileSize
new964 bytes

Here is a patch that make the changes described in the issue summary. But I haven't had a chance to actually do any manual testing of the new manualcrop, which will be necessary before this can be commited.

candelas’s picture

@dsnopek I found a bug in manualcrop-two-crop-tools-2237835.patch that makes that when you add a new image the manualcrop styles are lost. I reported here. Thanks for your work.

dsnopek’s picture

StatusFileSize
new963 bytes

Hmm. That patch was added to support #2255095: Add "Crop" tool to Spotlight widget which was reverted. So, we could actually remove that patch until that issue is merged! Here's a new patch that does that...

candelas’s picture

@dsnopek could be that in that patch you made an array and manual crop has not that option when creating a new image crop? Thanks to be so fast :)

dsnopek’s picture

I'm really not sure what could be causing the problem, I haven't had any time to look into it. But since we reverted the functionality that actually needs that patch, we may as well worry about it later when adding back that functionality. :-)

candelas’s picture

I just saw that they change last dev. The one I checked and I am using is 7.x-1.4+121-dev
I check the new dev and the only feature added is this
Add configure link on modules page
what I think doesn't interference.

dsnopek’s picture

Issue tags: +sprint
dsnopek’s picture

candelas’s picture

candelas’s picture

Issue summary: View changes

@dsnopek The patch that I mention in issue summary

- https://www.drupal.org/files/issues/manual_crop-fix-for-array-flip-22924... //needed

has been commited. https://www.drupal.org/node/2292491

The Manual Crop mantainer has said in #2306801-7: Upcoming 1.5 release that at the end of next week he will be willing to release 1.3. Also he says that if there is any tested patch that we, users, want to be committed, that we say in that issue.
Since I know little, maybe it can be interesting for you to take a look on their patches that need review, so in the future you don't need to patch it.

dsnopek’s picture

It'd be very welcome if a new stable version of manualcrop was released! All but one of our patches has been merged upstream already and the remaining one is kind of unfinished anyway. So, I don't think there's any patches we really need to push for to be included! If you see that they make a new manualcrop release, please let us know on this issue. Thanks!

candelas’s picture

Mainteiner said:
Now that #1828178: Media 2.x UI suggestion in the Media file selector widget has been fixed I'll release a new version next saturday! So please give
the lastest dev version a test drive to prevent major issues in the next
release

dsnopek’s picture

StatusFileSize
new845 bytes

@candelas: Thanks for the update! Here's a quick re-roll of the patch with the now latest manualcrop -dev so that I can try this on Travis-CI. Then I'm going to do some manual testing.

EDIT: Here is the Travis-CI build: https://travis-ci.org/panopoly/panopoly/builds/57945062

dsnopek’s picture

Works great in my manual testing! Assuming Travis-CI is ok with it, I think this is committable. :-) I guess then it'll be a question of whether we commit with -dev or wait for the release next Saturday.

dsnopek’s picture

@candelas: Can you do some manual testing with the latest manualcrop -dev in Panopoly too to see if you can find any issues that I didn't find? Thanks!

candelas’s picture

@dsnopek I will try during the weekend or begining of next week. Now I am learning about features because I have made many changes to Open Atrium and I am looking for a method to keep my changes when I update version.

I would wait until the stable ManualCrop release is done, wouldn't I? Like that you will not have to update later :)

candelas’s picture

Title: Update to last ManualCrop dev » Update to last ManualCrop 7.x-1.5

I change the subject and if I report any problem, I will make it related :)

candelas’s picture

Status: Needs review » Active
jiv_e’s picture

Can I help with this? I have problems that are fixed in the latest stable manualcrop (1.5). By the way what is the recommended way to update a single module in panopoly for my own project?

dsnopek’s picture

Can I help with this?

Yes! Doing manual testing with Panopoly and the latest Manualcrop, making sure that everything works as it should, and reporting back here would help. Even more helpful (although, a lot harder to do) would be creating some Behat tests for the Manualcrop functionality as exposed in Panopoly so we can merge in updates quicker (since we won't need to manually test as much).

By the way what is the recommended way to update a single module in panopoly for my own project?

If we're just talking about a single site (as opposed to a child distribution) and this is just for testing purposes, here's what I'd recommend:

  1. Download the module to sites/all/modules (as opposed to profiles/panopoly/modules)
  2. Look at the panopoly_images.make file and try to apply any Manualcrop patches that weren't committed yet
  3. Clear cache and run module updates (ie. drush updb -y or update.php)

But once Panopoly does this update, be sure to remove your extra copy from sites/all/modules - otherwise you'll miss out on future updates from Panopoly.

Thanks!

jiv_e’s picture

Great! Thanks for the answer! Panopoly_test has crop mentioned only in two places. Current tests check that the crop button is available on media media browser and when adding a spotlight.

What kind of tests for cropping would be most critical in your opinion? I could try to look into that if I get some free time in the future.

dsnopek’s picture

Ideally, it would actually use Manualcrop to crop an image and then verify that an image with the correct dimensions was generated. I think that'd be enough! However, it will be a bit challenging because so much of this will need to be done with Javascript.

dsnopek’s picture

Here is a new version of this patch which updates to manualcrop 1.5, and includes #2499647: Update to jquery.imgareaselect v1.0.0-rc1 (so we can download from GitHub) to try and fix #2499595: Build failing because host of jquery.imgareaselect (odyniec.net) has gone offline

I've done a little manual testing, I'm going to do some more in a bit. I think I'll probably end up making a new issue for writing the Behat tests for manualcrop, because I want to get this merged soon!

dsnopek’s picture

Title: Update to ManualCrop 1.5 » Update to latest ManualCrop 7.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new1.75 KB

Alright, that patch was short lived. :-) The maintainer of manualcrop made a commit to pull the 0.9.11-rc1 release from GitHub:

http://cgit.drupalcode.org/manualcrop/commit/?id=d6c449d68826b034492460b...

So, here's a newer patch which updates to that version!

EDIT: Here is a Travis build of this patch: https://travis-ci.org/panopoly/panopoly/builds/65247524

dsnopek’s picture

Here is a follow-up issue to write the Behat tests: #2499699: Write Behat tests for ManualCrop

My manual testing is looking great! If the automated tests pass on Travis, I'm going to commit this.

I'd feel better if we had more testing/review, but since this is breaking our build and essentially preventing us from being able to release, I'm going to push it through.

  • dsnopek committed 344caa9 on 7.x-1.x
    Update Panopoly Images for Issue #2401473 by dsnopek: Update to latest...
dsnopek’s picture

Status: Needs review » Fixed

Committed! Thanks to everyone who helped to push this issue forward :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.