[OPEN_][Gallery]the crop box is shown dislocation when landscape in edit mode.(617002043345)

VERIFIED FIXED

Status

Firefox OS
Gaia::Gallery
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Firefox_Mozilla, Assigned: jhylands)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Steps to reproduce:
1.Go into Gallery and choose an image to view;
2.tap “edit” button and select crop;
3.landscape
Expected results:
1.the image an the crop box are shown normally.
Actual results:
1.the image is distorted and the crop box is over the image.
(Reporter)

Updated

5 years ago
Blocks: 845223
(Reporter)

Comment 1

5 years ago
Created attachment 722082 [details]
screenshot

Comment 2

5 years ago
Created attachment 722090 [details]
Screenshot of crop error

I can reproduce it with the newest nightly build 20130306070201.

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

5 years ago
blocking-b2g: --- → tef?
(tef+, as this is a pretty bad functional issue with croping)
blocking-b2g: tef? → tef+
David, are you the best owner here?
Assignee: nobody → dflanagan
Reassigning this to Jon, as he's been working on this in bug 842700.

Since this bug is tef+, however, we'll move Jon's work here rather than there.

Note that the editing function has never worked at all in landscape mode. Jon is making it work, but his patch will involve more than cropping.  It doesn't make sense to fix just the cropping because editing would still be unusable in landscape mode in that case.
Assignee: dflanagan → jhylands
(Assignee)

Comment 6

5 years ago
Created attachment 724023 [details]
new pull request
Attachment #724023 - Flags: review?(dflanagan)
(Assignee)

Comment 7

5 years ago
Created attachment 725088 [details]
fixed so crop resize still works after orientation change
Attachment #724023 - Attachment is obsolete: true
Attachment #724023 - Flags: review?(dflanagan)
Attachment #725088 - Flags: review?(dflanagan)
Comment on attachment 725088 [details]
fixed so crop resize still works after orientation change

In testing, this works well.  There are a couple of things that would be nice to fix (but not required)

1) You mentioned you were getting two resize events. When I rotate the phone, I see the image change sizes twice. Can we hide or prevent the first one?

2) If I edit and image by sliding the exposure slider, then the next time I enter edit mode, I see the exposure slider start off at its old location and then move to 0.  Maybe you can reset it to zero when exiting edit mode so it starts off in the right place.

3) Here's a weird corner case. For cropping, the crop handles aren't allowed to get smaller than 100x100 so that the touch regions don't overlap each other. If you start in portrait mode, and do a freeform crop to as small as possible then switch to landscape mode, the crop frame becomes smaller than 100x100 and now it won't let me drag to enlarge it. This isn't something we'd block on because it is easy to work around. But it might be easy to add the necessary logic to prevent making the handles smaller than 100x100 but always allowing them to be enlarged (but not allowing them to be enlarged more than the image size, of course).

There's a meta issue: because of your git troubles, you now have a new commit message that does not include the bug number or describe your fix.  So you need to update the message.  (Do a git rebase -i master and use the 'reword' option to edit it.)

I've commented on the code on github. There are a couple of issues to fix before we can land it.

Overall, though, this is really nice work. Landscape editing works!
Attachment #725088 - Flags: review?(dflanagan) → review-
(Assignee)

Updated

5 years ago
Attachment #725088 - Attachment description: fix dialer history to work with different AppIDs (rebased off master) → cleaned up formatting, fixed 2 new issues in gallery edit
Attachment #725088 - Flags: review- → review?(dflanagan)
Comment on attachment 725088 [details]
fixed so crop resize still works after orientation change

Jon,

Just one more round of fixes. See my comment on github about dealing with the too-small crop regions after rotation.

Thanks for fixing the non-blocking stuff. This is going to be good!  Did getting rid of the extra event handler make the flash on rotation go away?

Finally, I'd still like you to alter your commit message.  Convention seems to be to include the word 'Bug' before the number, and describe the overall effect of the patch, not just the most recent commit.

Something like: 'Bug 848625 - make image editing work in landscape orientation'
Attachment #725088 - Flags: review?(dflanagan) → review-
(Assignee)

Comment 10

5 years ago
Comment on attachment 725088 [details]
fixed so crop resize still works after orientation change

Modified the move() function so that it only allows the user to resize the crop region bigger if it is smaller than the minimum allowed size.
Attachment #725088 - Attachment description: cleaned up formatting, fixed 2 new issues in gallery edit → fixed so crop resize still works after orientation change
Attachment #725088 - Flags: review- → review?(dflanagan)
Comment on attachment 725088 [details]
fixed so crop resize still works after orientation change

r- for lint errors in the travis build:

https://travis-ci.org/mozilla-b2g/gaia/builds/5593977#L63

Looks like just some too-long lines.
Attachment #725088 - Flags: review?(dflanagan) → review-
(Assignee)

Comment 12

5 years ago
Comment on attachment 725088 [details]
fixed so crop resize still works after orientation change

fixed lint issues
Attachment #725088 - Flags: review- → review?(dflanagan)
Landed on gaia master: https://github.com/mozilla-b2g/gaia/commit/3010edd727863a657a3ef784ed7cbce4844e5c4a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 3010edd727863a657a3ef784ed7cbce4844e5c4a
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)
We won't be able to uplift this until bug 826249 is uplifted.

Updated

5 years ago
Duplicate of this bug: 853813
Landed on v1-train: https://github.com/mozilla-b2g/gaia/commit/6dbf0b68622aa80d079fee2af660ddc47f7cd7df
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
Landed on v1.0.1: https://github.com/mozilla-b2g/gaia/commit/d40dcdd112f12e2a5a0d1de46451670918fd4369
status-b2g18-v1.0.1: --- → fixed

Comment 19

5 years ago
This issue no longer reproduces on Unagi device
Build ID: 20130329070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/56c922308fd1
Gaia: 0a9f78bffafda93a159c1f502e8b110c2f49a500

Also, issue appears fixed on
Build ID: 20130329070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/5cc5df16447a
Gaia: 26b463f14caa11e0fc64fda09a17054da4bea68b

The image in the crop box is shown normally, after switching to landscape mode.
Verified Fixed
Status: RESOLVED → VERIFIED
status-b2g18: fixed → verified
status-b2g18-v1.0.1: fixed → verified

Updated

4 years ago
Attachment #725088 - Flags: review?(dflanagan)
You need to log in before you can comment on or make changes to this bug.