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.

Sign up for GitHub

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

Jump to bottom

Impossible to zoom out after rotating photo in the cropper #1538

Closed
teolemon opened this issue Apr 11, 2022 · 15 comments · Fixed by #2872
Closed

Impossible to zoom out after rotating photo in the cropper #1538

teolemon opened this issue Apr 11, 2022 · 15 comments · Fixed by #2872
Assignees
@monsieurtanuki
Labels
🐛 bug Something isn't working 🎯 P1 🖼️ Photos - Cropping
Milestone
Week 34 and 35

Comments

@teolemon
Copy link
Member

What

  • Impossible to zoom out after rotating photo in the cropper

Steps to reproduce the behavior

  1. Go to Product Addition
  2. Take a photo
  3. Rotate it
  4. Get a zoom in
  5. Try and fail to zoom out

Expected behavior

Just a rotation without zoom ?

Part of

Screenshot/Mockup/Before-After

screen-20220411-113914.mp4
@teolemon teolemon added this to the V2 milestone Apr 11, 2022
@M123-dev
Copy link
Member

Don't think this is possible with our current implementation

@monsieurtanuki
Copy link
Contributor

Again, if you need a simple implementation with just the full image, a 90-180-270 rotation and a crop tool, I can dig up what I coded in another project.

@teolemon teolemon added 🐛 bug Something isn't working 🎯 P1 labels Aug 7, 2022
@teolemon
Copy link
Member Author

@alexgarel showed me the issue again yesterday. I grumbled, saying we should probably go back to the older cropper, which was a 90-180-270 rotation tool, until we can figure out how to make that work properly.
Also note that non-destructive crop remains on my mind ( #2484 ), especially since we have the openfoodfacts-dart tooling to do it.

@monsieurtanuki
Copy link
Contributor

@teolemon The older cropper you're referring to was in the V1, or already in Smoothie?

There are currently 4 image croppers in pub.dev beyond 90%:

  • image_crop (saw the demo: I don't trust them)
  • crop_image (looks more like what I had in mind - not sure if they support 90 rotations)
  • crop_your_image (looks ok; doesn't support tilt that must be done upstream)
  • image_cropper (the one we're using) (has probably different UI/UX because it relies on existing native libraries)

And of course we can code it from scratch, like I did in another project.

At least we should test the first 3 of them. Is there a project for experiments or should we create one? Maybe I can do that with my aborted fast food project?

The requirements I see are:

  • as an input, an image (file?)
  • we always display the full image on the screen (probably zoomed out)
  • the image never changes (no zoom)
  • one "left rotation" button
  • handles on top that display the crop area - they can be moved/resized (no fix aspect ratio)
  • one "crop!" button, that either returns the cropped image, or the crop parameters (rotation, crop left, crop top, crop width, crop height)

@g123k
Copy link
Collaborator

g123k commented Aug 18, 2022

After discussing with @AshAman999 and @teolemon at the community point today, here are the minimum requirements for replacing the current cropper:

  • Have a Flutter implementation, allowing us to easily tweak it
  • Don't crop the (file) image, but only provide the coordinates/area

That way, we can ask the server to do the resizing stuff for us.

@monsieurtanuki monsieurtanuki self-assigned this Aug 18, 2022
@monsieurtanuki
Copy link
Contributor

Looks like it's going to be crop_image then.
I'll test that tomorrow.

@monsieurtanuki
Copy link
Contributor

That will be as simple as this; is that ok?

x +0° +180°
+0° Capture d’écran 2022-08-19 à 14 32 54 Capture d’écran 2022-08-19 à 14 33 41
+90° Capture d’écran 2022-08-19 à 14 34 17 Capture d’écran 2022-08-19 à 14 34 48

@teolemon teolemon modified the milestones: V2, Week 34 and 35 Aug 25, 2022
@teolemon teolemon mentioned this issue Aug 25, 2022
Closed
@monsieurtanuki
Copy link
Contributor

That will be as simple as this; is that ok?

ping

@M123-dev
Copy link
Member

Looks good to me, it's no gorgeous UI (as before), it has no extraordinary features (like before), it stays the same with simpler UX so I like it

@monsieurtanuki
Copy link
Contributor

Still working on it, but close to the end.
Probably pushed by Sunday evening.
Unfortunately the initial package (crop_image) did not provide the rotation: I had to code it and will push my code here and suggest it to crop_image.

@monsieurtanuki
Copy link
Contributor

For the record I've just remarked that the crop tool is significantly different in Android and iOS - which makes sense as we're talking about different native packages.
I cannot reproduce the issue neither in Android nor in iOS, though I must say the interface is confusing on both systems (both different and both confusing).
From my personal experience with flutter and image manipulations, we should rather be very sure of what we're doing, because flutter is notoriously slow for that (native packages should be used especially for the "save as file" part).

That said, I'm about to PR a refactoring. That's a first step. I'm already working on the next steps.

monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Aug 28, 2022
@monsieurtanuki
fix: openfoodfacts#1538 - refactoring of image cropper
a928d40 
Impacted files:
* `confirm_and_upload_picture.dart`: minor refactoring.
* `image_crop_page.dart`: rather heavy refactoring needed to move forward.
monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Aug 29, 2022
@monsieurtanuki
fix: openfoodfacts#1538 - refactoring of image cropper
f94e5c8 
Impacted files:
* `confirm_and_upload_picture.dart`: minor refactoring.
* `image_crop_page.dart`: rather heavy refactoring needed to move forward.
@monsieurtanuki monsieurtanuki mentioned this issue Aug 29, 2022
Merged
monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Aug 29, 2022
@monsieurtanuki
fix: openfoodfacts#1538 - refactoring of image cropper
f839692 
Impacted files:
* `confirm_and_upload_picture.dart`: minor refactoring.
* `image_crop_page.dart`: rather heavy refactoring needed to move forward.
monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Aug 29, 2022
@monsieurtanuki
fix: openfoodfacts#1538 - refactoring of image cropper
1ff4c62 
Impacted files:
* `image_crop_page.dart`: vertical buttons for gallery/camera dialog
* `smooth_alert_dialog.dart`: fixed comment
@github-actions github-actions bot mentioned this issue Aug 30, 2022
Merged
@monsieurtanuki
Copy link
Contributor

Actually #2858 was just a first step.

monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Aug 30, 2022
@monsieurtanuki
fix: openfoodfacts#1538 - new crop tool (cf. dev mode)
33ba5d1 
New files:
* `crop_grid.dart`: heavily inspired from package `crop_image` - ideally we should put it back there.
* `crop_helper.dart`: Crop Helper - which crop tool do we use, and the method to use it.
* `new_crop_page.dart`: Page dedicated to image cropping. Pops the resulting file path if relevant.
* `rotated_crop_controller.dart`: heavily inspired from package `crop_image` BUT with the rotation feature - ideally we should put it back there.
* `rotated_crop_image.dart`: heavily inspired from package `crop_image` BUT with the rotation feature - ideally we should put it back there.
* `rotation.dart`: 90 degree rotations - ideally we should put it back in package `crop_image`

Impacted files:
* `image_crop_page.dart`: now relying on new class `CropHelper` in order to get the appropriate crop tool (e.g. old or new)
* `pubspec.lock`: wtf
* `pubspec.yaml`: added package `image` for good performances regarding image encoding
* `user_preferences_dev_mode.dart`: added a "Use new crop tool" switch (default is `false`)
@monsieurtanuki monsieurtanuki mentioned this issue Aug 30, 2022
Merged
monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Sep 4, 2022
@monsieurtanuki
Merge branch 'develop' into fix_2/openfoodfacts#1538
6c4200b
monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Sep 6, 2022
@monsieurtanuki
Merge branch 'develop' into fix_2/openfoodfacts#1538
e03f062
@github-actions github-actions bot mentioned this issue Sep 6, 2022
Merged
monsieurtanuki added a commit that referenced this issue Sep 7, 2022
@monsieurtanuki
fix: #1538 - new crop tool (cf. dev mode) (#2872)
535cddc 
New files:
* `crop_grid.dart`: heavily inspired from package `crop_image` - ideally we should put it back there.
* `crop_helper.dart`: Crop Helper - which crop tool do we use, and the method to use it.
* `new_crop_page.dart`: Page dedicated to image cropping. Pops the resulting file path if relevant.
* `rotated_crop_controller.dart`: heavily inspired from package `crop_image` BUT with the rotation feature - ideally we should put it back there.
* `rotated_crop_image.dart`: heavily inspired from package `crop_image` BUT with the rotation feature - ideally we should put it back there.
* `rotation.dart`: 90 degree rotations - ideally we should put it back in package `crop_image`

Impacted files:
* `image_crop_page.dart`: now relying on new class `CropHelper` in order to get the appropriate crop tool (e.g. old or new)
* `pubspec.lock`: wtf
* `pubspec.yaml`: added package `image` for good performances regarding image encoding
* `user_preferences_dev_mode.dart`: added a "Use new crop tool" switch (default is `false`)
@monsieurtanuki
Copy link
Contributor

@teolemon @M123-dev @g123k @VaiTon I merged #2872 more than a month ago: have you tested the new (lighter) crop tool? As a reminder, you need to go to dev mode and click on the "yes I want the new crop tool" switch.

@M123-dev
Copy link
Member

M123-dev commented Oct 8, 2022

Yes I have been using it as my main for this time, actually remembered about it yesterday again but haven't had the time to create a issue yet.

I think we can switch, but I haven't tested on iOS

@AshAman999
Copy link
Member

I was using this for the past week, works fine for iOS,

Can't rotate to other angles other than 90 degrees, anyways it's light and does the job for me ✅

@monsieurtanuki monsieurtanuki mentioned this issue Oct 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@monsieurtanuki monsieurtanuki

Labels
🐛 bug Something isn't working 🎯 P1 🖼️ Photos - Cropping
Projects
🤳🥫 The Open Food Facts mobile app (Android & iOS)
Archived in project
Milestone
Week 34 and 35
Development

Successfully merging a pull request may close this issue.

fix: #1538 - new crop tool (cf. dev mode) monsieurtanuki/smooth-app
5 participants
@g123k @teolemon @monsieurtanuki @M123-dev @AshAman999