833738 - [Gallery] Displaying zoom image while setting the wallpaper from gallery

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Leo]

1. Title : Displaying the zoom image while setting the wallpaper from gallery 
2. Precondition : at least a image is needed in gallery 
3. Tester's Action : Open gallery -> open image -> Full view  -> share -> wallpaper -> open
4. Detailed Symptom (ENG.) : It is showing zoom image when selecting the wallpaper
5. Gaia Revision #:  "7b8dea7e7ec377a2143151bbe5e9998d87f7b36d"
6. Expected : It should show original image size without any zoom or cut.
7.Reproducibility: Y
1)Frequency Rate : 100%
8.Comparison Results : 
1)Model Comparing : 
9. Attached files: 
1)Log : 
2)Test Contents : 
3)Video file :

Comment 1

[Dietrich Ayala (:dietrich)]

This sounds like a minor issue. Please provide a screenshot to better illustrate the problem.

Comment 2

[Leo]

Attachment: Screenshot with original image and the image while opening take ~8sec

Screenshot with original image and the image while opening

Comment 3

[Leo]

For a smaller resolution image, this bug was not clearly seen.We tried with a higher resolution image, We found loading a image itself takes pretty high time(almost ~ 8sec).

Comment 4

[Leo]

Attachment: High Resolution Image as a Wallpaer

I have attached a RAR file. It has 3 images. 
1. original file of resolution: 2560x1600
2. Same file when displaying as FULL SCREEN VIEW
3. same image, when I tried to set as a wallpaper.
When try to set such a high resolution image from GALLERY -> Shared Button -> It not allowing to scroll and crop also.

Comment 5

[Dietrich Ayala (:dietrich)]

Please renominate for leo? if you think the bug should block the release of the product, and explain why.

Comment 6

[Leo]

Attachment: Original full view mode

Original Full view mode

Comment 7

[Leo]

Attachment: Zoomed image view , while setting as wallpaper

1. Every listed images in Gallery should be able to display, fully on screen before gets cropped by user
2. Setting image as wallpaper from gallery has been used widely.
3. Showing only a part of the image is very confusing user experience.

I have attached the screen shots one more time.

Comment 8

[David Flanagan [:djf]]

Michal's work on bug 856543 will probably fix this one as well. That one has been marked tef+ (because of performance)

Comment 9

[David Flanagan [:djf]]

Comment on attachment 732229 [details]
Original full view mode

Clearing the review flags. Thanks for the screen shots. We generally use r? for patches. If you're just attaching a screenshot, I'd suggest just using needinfo to get our attention.

Comment 10

[Tony Chung [:tchung]]

not blocking leo, but request tracking-b2g18.

Comment 11

[Anthony Ricaud (:rik)]

To be clear, the fix in bug 856543 will not fix this issue. It will just crop the image from the center instead of from a side. A better fix for this would be to add a UI to let the user crop the image.

Comment 12

[David Flanagan [:djf]]

(In reply to Anthony Ricaud (:rik) from comment #11)
> To be clear, the fix in bug 856543 will not fix this issue. It will just
> crop the image from the center instead of from a side. A better fix for this
> would be to add a UI to let the user crop the image.

I disagree strongly. The work required to add cropping to the wallpaper app would be substantial. A crop UI is a lot of work, even when copied from the Gallery app. It just doesn't seem worth undertaking a LOE:M bug for what is a very minor UX affordance. Especially when we don't have any input (do we?) from the UX team saying that the current behavior is incorrect.

If the user shares an image from the gallery app to the wallpaper app, it seems to me that they've told the wallpaper app that that is the image they'd like to use as wallpaper, and the wallpaper app should display as much of the image as possible.

If the user wants to use only a portion of an image as wallpaper, they can use the Gallery's editing features to crop the image, and then share the edited image as wallpaper.

I can see the argument that maybe if the user has zoomed in on an image and then does a share what they want is to share the current-visible portion of the image, not the entire image.  I don't think I agree with that, but I can at least understand the argument.  If we want to enable the wallpaper app to use only the currently-visible portion of an image, let's modify Gallery to pass the entire image blob (as it does currently) along with the coordinates of the currently visible region.  Then the wallpaper app (or any other app that receives the share) can use those coordinates to crop the received image, if that is the behavior they want to implement.  

To summarize, here are the solutions I see, in my order of preference:

1) Only do cropping in the gallery app. If the user wants to use just a portion of an image as wallpaper, they edit it in the gallery and share the edited version

2) Modify the Gallery Share activity to pass the currently-visible region of the image, and modify the Wallpaper app to crop the image to that region.

Adding a cropping UI to the wallpaper app shouldn't even be an option that we consider.

Comment 13

[Anthony Ricaud (:rik)]

I don't have a strong opinion on how we should fix this issue. My comment was just here to respond to comment 8 where you stated that the work in bug 856543 would probably fix this issue.

Feel free to fix this with the solution you think is the best.

Comment 14

[Josh Carpenter [:jcarpenter]]

Just getting up to speed on this bug. I respectfully disagree with the Expected behavior:

> Expected : It should show original image size without any zoom or cut.

I think showing the zoomed in photo is correct. But I agree that we then need to enable the user to frame the shot. Ideally they should able to drag the image from side to side (or up and down, depending on aspect ration of original image). In the attachment from comment #7, for example, I could drag the photo to the right to show the male character's face. Selecting "Set as Wallpaper" would then crop and save the new version of the photo as my wallpaper.

David, this is probably more involved than your two options above, but hopefully is a few notches easier than replicating the more complex cropping UI from our Gallery app?

Comment 15

[Michał Budzyński (:michalbe)]

Attachment: patch

Comment 16

[Michał Budzyński (:michalbe)]

(In reply to Josh Carpenter [:jcarpenter] from comment #14)
> Ideally they should able to drag
> the image from side to side (or up and down, depending on aspect ration of
> original image). In the attachment from comment #7, for example, I could
> drag the photo to the right to show the male character's face. Selecting
> "Set as Wallpaper" would then crop and save the new version of the photo as
> my wallpaper.
> 

I prepared a patch based on your request, users are now able to drag the photo and choose where to crop it.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 750863 [details]
patch

This patch works well enough to convince me that Josh's compromise is a good one. I like the interaction. For v1.2 we should consider adding some kind of translucent arrow overlays to indicate to the user that they can pan the image.  And we should make the two buttons translucent too, because they currently hide the image and look strange in white.  But those things are out of scope here.

I'm giving r- on this because there are some errors in the code to be corrected, and also because the wallpaper image only pans at 20fps.  In order to add the dragging feature, I think we can't use CSS backgroundImage.

See detailed comments on github.

Comment 18

[Michał Budzyński (:michalbe)]

fixed, thanks.

Comment 19

[David Flanagan [:djf]]

Comment on attachment 750863 [details]
patch

The framerate is great now! But there is a bug where it doesn't always respond to quick swipes and feels sluggish.  Fix that, and a few stylistic points in my github comments, and you can land it.

Don't forget to squash your commits and fix lint before landing.

If you'd like another review before landing, just ping me.

Comment 20

[Michał Budzyński (:michalbe)]

Merged. Thanks!

https://github.com/mozilla-b2g/gaia/commit/320f68ae3facdf60eafd812f6692264b5bafb058