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.

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

[GlobalStep] Android - The Zoom indicator while editing an Image is incoherent. #2132

Closed
wptester9845 opened this issue Apr 8, 2020 · 16 comments · Fixed by wordpress-mobile/uCrop#1

Comments

@wptester9845
Copy link

Description

When editing an image using the Block Editor and using the zoom tool, a zoom percentage indicator is displayed. This value reflects the size of the image’s preview on the current screen and does not reflect the final image’s size in proportion to itself. This is immediately evident when changing the device’s orientation which causes a great disparity between zoom values even though the image size when publish is the same.

Reproduction Rate

4/4 100%

Expected behaviour

The displayed zoom value should reflect the image’s final size in proportion to the original size.

Actual behaviour

The displayed zoom value indicates the previewed image’s size while on the editing screen and does not reflect its final size.

Steps to reproduce the behaviour

  1. Install WordPress 14.6
  2. Login to a valid account.
  3. Create a New Post.
  4. Add an Image Block.
  5. Add an Image to the created Block.
  6. Press the button to Edit the Image.
  7. Select the zoom option.
  8. Change device’s orientation and observe the displayed Zoom value.
Tested on the following

Samsung Galaxy S8+ (8.0)

Please see the attached video for more information

AndroidEditZoom.zip

Submitted by:

Luis Pimenta

@designsimply
Copy link
Contributor

@iamthomasbishop can you help out with design direction on this one? Should we make any changes based on potential points of confusion raised in this issue?

@maxme
Copy link
Contributor

maxme commented Apr 9, 2020

@ashiagr and @malinajirka - I'm not 100% sure what the scale number is representing.

Since it's used to reframe/scale the image and not to change the display size, I actually wonder if we should hide the number or reset it to 100% every time the edit screen is opened (if it's possible via the library).

@iamthomasbishop
Copy link
Contributor

Interesting! I haven’t seen this UI before so I also wasn’t sure what was meant by “scale” — it feels more like “zoom” to me.

What I would have expect here is a “zoom” scale that starts at 1 (or “Original”, or similar) and allows you to swipe right-to-left to to “zoom” the image in. As far as I can tell, you can’t scale the image “out” — it simply snaps to its original value when you try this.

@maxme Do we have control over this UI in a way that we can adjust the starting point, direction, and labeling?

@ashiagr
Copy link
Contributor

ashiagr commented Apr 13, 2020

👋 Hi there! Did a preliminary testing and found the issue exists in the uCrop (the external library we use for scaling) sample app as well. Image wasn't scaled yet it displayed 58% as the zoom level:

uCrop_zoom

We'll look into it in about a week.

@ashiagr
Copy link
Contributor

ashiagr commented Apr 24, 2020

I checked scale percent for images of different sizes in the library:

Size Scale Percent
2000x2000 49%
1000x1000 99%
500x500 198%

It seems the library scales down the image if it is bigger in size and vice-versa as the image is fitted into a rectangle. It might look odd at first but it appears to be the right behaviour.

If it is confusing, I can override the text style from the library to hide the text. It will hide the rotate and scale text as both use the same style.

@iamthomasbishop , what do you suggest?

Rotate Scale
device-2020-04-24-144116_hide_rotate device-2020-04-24-144054_hide_scale

@malinajirka
Copy link
Contributor

malinajirka commented Apr 24, 2020

Thanks for looking into this @ashiagr ! ;)

It seems the library scales down the image if it is bigger in size and vice-versa as the image is fitted into a rectangle. It might look odd at first but it appears to be the right behaviour.

When you leave the library, does it save the original size of the image into a file or the scaled-down version?

  • I think the label would ideally display the size in which the image is going to be saved.
  • Moreover, I think it shouldn't scale down the image be default if it does. Imo in most cases the user will want to keep the original size as the post will be published to a web and some users will look at the image on a bigger screen.

I understand that customizability of the library is very limited. I'd just like to understand how it currently behaves and what an optimal solution would be. We can check if the optimal solution is feasible and adjust it accordingly later.

@ashiagr
Copy link
Contributor

ashiagr commented Apr 24, 2020

When you leave the library, does it save the original size of the image into a file or the scaled-down version?

It saves the original size of the image.

I think it shouldn't scale down the image be default if it does.

Library saves the original size.

I think the label would ideally display the size in which the image is going to be saved.

The scale "display" percentage is w.r.t. to device resolution. That's why it appears confusing.

Pixel3a (1080x2220) Pixel 3 XL (1440x2960)
pixel3a pixel3xl

Relevant piece of code from the library with matrix calculations for scaling:
https://git.io/JfLBs
https://git.io/JfLBT

I can spend more time to see if we can make it independent of device resolution, but it might take time, so suggested to hide the label.

@ashiagr
Copy link
Contributor

ashiagr commented May 4, 2020

Slightly modified just the scale "display" text logic in the library's forked repo here.

scalefix

Tested this image with size: 2000x2000 in the library's sample app by replacing the uri here.

Any inputs?

cc @malinajirka

@malinajirka
Copy link
Contributor

Great job @ashiagr ! I like the solution and the code LGTM (I haven't tested the behavior). I'd suggest creating a PR against the original repo. I think the authors might like this approach more than the current behavior. If they don't merge it within a month or so we can consider using the forked repo. Wdyt?

@ashiagr
Copy link
Contributor

ashiagr commented May 4, 2020

I'd suggest creating a PR against the original repo. I think the authors might like this approach more than the current behavior. If they don't merge it within a month or so we can consider using the forked repo. Wdyt?

Agree, will create a PR against the orig repo.
EDIT: PR created here.

@designsimply
Copy link
Contributor

@ashiagr I'd like to close this even though the PR you submitted upstream at Yalantis/uCrop#651 hasn't shipped because the work on our side looks done. What's your feeling about that? Okay to close? Is there anything that can be done to check back in on the open PR in the mean time?

@ashiagr
Copy link
Contributor

ashiagr commented Oct 7, 2020

@designsimply I'd preferably like to keep it open else will lose track of it. I can also target forked branch if everyone agrees.

cc @malinajirka

@malinajirka
Copy link
Contributor

I can also target forked branch if everyone agrees.

I'm not sure what's our stand on targeting forked repos. It feels like we should at least fork it with a WordPress account (fork your repo so you don't lose the contribution). Wdyt @aforcier ?

@aforcier
Copy link

I'm not sure what's our stand on targeting forked repos.

We've had to do this before, one example that comes to mind is Wellsql, used in FluxC: https://github.com/wordpress-mobile/wellsql. It's not ideal but kind of the only option if PRs aren't getting reviewed by the maintainers of the original library.

It feels like we should at least fork it with a WordPress account (fork your repo so you don't lose the contribution).

This sounds like the right move. For each change we needed to make to Wellsql we used to submit a PR to our fork, and also upstream.

So in short, how about if:

  1. We fork uCrop to wordpress-mobile
  2. @ashiagr re-creates her upstream PR on our fork, it's merged in
  3. We point our app to our fork of uCrop (might require setting up Bintray for that repo, like we did for Wellsql)

@ashiagr
Copy link
Contributor

ashiagr commented Nov 2, 2020

Sorry for a late response @aforcier , plan sounds good to me.
How do we fork uCrop to wordpress-mobile? Is it something you can help with?

cc @malinajirka

@malinajirka
Copy link
Contributor

I've forked the repo into wordpress-mobile - https://github.com/wordpress-mobile/uCrop.

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

Successfully merging a pull request may close this issue.

8 participants