Closed Bug 952651 Opened 8 years ago Closed 7 years ago

[Gallery][Photo Editor]Slider for brightness can slide from the last position the slider was in than the middle position.


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

Gonk (Firefox OS)
Not set


(b2g-v1.4 affected)

Tracking Status
b2g-v1.4 --- affected


(Reporter: nhirata, Assigned: ranjith253)


(Whiteboard: [g+],[LibGLA,TD86974,WW, B] )


(2 files, 2 obsolete files)

From :

1. launch gallery
2. select a photo
3. Go into edit mode,
4. drag the slider way to the left. 
5. Click X to exit edit mode and come right back to it. 
6. Drag the slider 

Actual : it will jump way back to the left where you left it the last time and then drag from there.
Expected: the slider will drag from the middle to where you placed it currently

Gaia      574f79512a7b8a9ab99211b16a857ab812d7994e
BuildID   20131220040201
Version   29.0a1 Nov 14 10:58:33 CST 2013

Video :
(I added some extra tapping .. didn't feel like retaking the video; those tappings are unnecessary)
Summary: [Gallery][Photo Editor]Slider for brightness will jump to the last placed position rather than the current position when reopening the panel. → [Gallery][Photo Editor]Slider for brightness can slide from the last position the slider was in than the middle position.
That slider has always been a pain in the butt.  This may be a recent regression
It seems we forget to reset sliderStartExposure to zero. And it seems we have this issue in 1.2.
I had tested with v1.1 and v1.2. It is reproducible on v1.2
Please review the patch & let me know the review comments
Attachment #8351174 - Flags: review?(dflanagan)
Comment on attachment 8351174 [details]
Pointer to Pull Request.html


Welcome! And thank you for taking this bug.

I've given r- here because your patch inadvertently changed the file permissions on the file.

I also think that there is a better way to do this.  Here's how I'd try fixing this bug.  For context, the way GestureDetector works is that it sends a series of pan events followed by a single swipe event when the user's finger goes up.

First, I'd rename sliderStartExposure to something simpler like dragStart.

I'd initialize it to null rather than to 0.  Null indicates that it is not set at all.  Then, in sliderDrag, I'd check to see if it was null. If so, I'd set it to the current exposure value.  And in the 'swipe' event handler, I'd remove the line that sets it to current exposure and instead set it back to null.

We only ever need this value in the sliderDrag function. So set it on the first pan event when the gesture begins and clear it on the swipe event when the gesture ends.

I'd also move the variable declaration up to the top of the containing function, and give the swipe event handler a function of its own named sliderDragEnd().

Set the needinfo flag for me (:djf) or email me directly if you have questions about this.
Attachment #8351174 - Flags: review?(dflanagan) → review-
Attached patch Proposed patch for this bug (obsolete) — Splinter Review
Hi Djf,

I have made required changes as suggested by you &  waiting for your review comments.
Attachment #8357657 - Flags: review?(dflanagan)
Assignee: nobody → ranjith253
Comment on attachment 8357657 [details] [diff] [review]
Proposed patch for this bug

Review of attachment 8357657 [details] [diff] [review]:

r- but just because of a couple of nits. It looks like the code would run fine (I have not actually tested it) but would like you to fix these nits before landing it.

::: apps/gallery/js/ImageEditor.js
@@ +130,5 @@
>    // prepare gesture detector for slider
>    var gestureDetector = new GestureDetector(thumb);
>    gestureDetector.startDetecting();
>    thumb.addEventListener('pan', sliderDrag);

Its weird that this event listener refers to a function defined below while the other event listeners have their functions inline.

While you're fixing this bug, please also cleanup the code and move the sliderDrag function so it is defined here where it is used.

@@ +150,5 @@
>      var exposureDelta = delta / parseInt(bar.clientWidth, 10) * 6;
> +    // For the firt time of pan event triggered
> +    // set start value to current value.
> +    if (!dragStart)
> +    dragStart = currentExposure;

the indentation of this line is wrong.
Attachment #8357657 - Flags: review?(dflanagan) → review-
Please review & update the new pull request.
This new pull request has review changes.
Attachment #8351174 - Attachment is obsolete: true
Attachment #8357657 - Attachment is obsolete: true
Attachment #8359085 - Flags: review?(dflanagan)
Whiteboard: [g+]
Attached file PR
Adding updated PR for review.
Attachment #8472239 - Flags: review?(pdahiya)
Attachment #8472239 - Flags: review?(dflanagan)
Whiteboard: [g+] → [g+],[LibGLA,TD86974,WW, B]
Comment on attachment 8472239 [details]


It looks like I ignored your patch for more than 6 months. I'm not sure how that happened, and I apologize. Based on my previous review, this is probably fine, but I'm going to leave final review to Punam who is currently working on ImageEditor changes.

Punam: this bug still happens with your patch applied so I think we should land this unless you find any problems with it.
Attachment #8472239 - Flags: review?(dflanagan)
Comment on attachment 8472239 [details]

Hi Ranjith
Sorry for delay in review, patch looks good and has my r+. Thanks!
Attachment #8472239 - Flags: review?(pdahiya) → review+
Patch landed on master
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8359085 [details]
Pointer to Pull Request.html

clearing an obsolete review request
Attachment #8359085 - Flags: review?(dflanagan)
You need to log in before you can comment on or make changes to this bug.