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

Status: RESOLVED FIXED

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

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

From : https://bugzilla.mozilla.org/show_bug.cgi?id=951812#c14

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
Gecko     http://hg.mozilla.org/mozilla-central/rev/599100c4ebfe
BuildID   20131220040201
Version   29.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013
Buri

Note:
Video : http://www.youtube.com/watch?v=uk6__z-6_8k&feature=youtube_gdata_player
(I added some extra tapping .. didn't feel like retaking the video; those tappings are unnecessary)

Comment 1

[David Flanagan [:djf]]

That slider has always been a pain in the butt.  This may be a recent regression

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

It seems we forget to reset sliderStartExposure to zero. And it seems we have this issue in 1.2.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

I had tested with v1.1 and v1.2. It is reproducible on v1.2

Comment 4

[ranjith253]

Attachment: Pointer to Pull Request.html https://github.com/mozilla-b2g/gaia/pull/14934

Please review the patch & let me know the review comments

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8351174 [details]
Pointer to Pull Request.html  https://github.com/mozilla-b2g/gaia/pull/14934

Ranjith,

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.

Comment 6

[ranjith253]

Attachment: Proposed patch for this bug

Hi Djf,

I have made required changes as suggested by you &  waiting for your review comments.

Comment 7

[David Flanagan [:djf]]

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.

Comment 8

[ranjith253]

Attachment: Pointer to Pull Request.html

Please review & update the new pull request.
This new pull request has review changes.

Comment 9

[ranjith253]

Attachment: PR

Adding updated PR for review.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8472239 [details]
PR

Ranjith,

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.

Comment 11

[Punam Dahiya [:pdahiya]]

Comment on attachment 8472239 [details]
PR

Hi Ranjith
Sorry for delay in review, patch looks good and has my r+. Thanks!

Comment 12

[Punam Dahiya [:pdahiya]]

Patch landed on master
https://github.com/mozilla-b2g/gaia/commit/42914c0e8dc0d228e4f3e009f1f99eb30bd4c988

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8359085 [details]
Pointer to Pull Request.html

clearing an obsolete review request