Last Comment Bug 765069 - Close by swipe velocity checks are wrong
: Close by swipe velocity checks are wrong
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 765165 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-14 15:58 PDT by Wesley Johnston (:wesj)
Modified: 2012-07-01 02:13 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (2.54 KB, patch)
2012-06-14 15:58 PDT, Wesley Johnston (:wesj)
mbrubeck: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Wesley Johnston (:wesj) 2012-06-14 15:58:03 PDT
Created attachment 633305 [details] [diff] [review]
Patch

So when I first wrote close by swipe I threw in a min velocity of 1000px/sec based on some testing on my end. Then, I changed that to be dpi independent, but forgot to update the value. So right now, you have to fling at > 1000 in/sec for the fling to trigger slide closed (releasing your finger on the opposite side you started also triggers it, so you don't notice this is broken.

This fixes that use a min velocity of 5 in/sec, which feels good to me (its hard to go much slower without feeling like an idiot). It also fixes some problems with the speed calculation I didn't notice because this wasn't working.
Comment 1 Wesley Johnston (:wesj) 2012-06-14 15:58:54 PDT
Comment on attachment 633305 [details] [diff] [review]
Patch

I haven't had a good mbrubeck review in awhile. You want to look at this?
Comment 2 Matt Brubeck (:mbrubeck) 2012-06-14 16:16:57 PDT
Comment on attachment 633305 [details] [diff] [review]
Patch

>+    private static final int SWIPE_CLOSE_VELOCITY = 5;
>     private static final int MAX_ANIMATION_TIME = 250;

Please add units for both of these, either in the identifiers themselves or in comments right here.

>             if (Math.abs(velocityX)/GeckoAppShell.getDpi() > SWIPE_CLOSE_VELOCITY) {
>+                // is this is a swipe, we want to continue the row moving at the swipe velocity
>                 float d = (velocityX > 0 ? 1 : -1) * mView.getWidth();
>+                animateTo(mView, (int)d, (int)((d + mView.getScrollX())*1000/velocityX));

So velocityX is in px/s, and the 1000 is because animateTo takes an a duration in ms, right?  Could you add comments to onFling and animateTo noting the units of those arguments?
Comment 4 Ed Morley [:emorley] 2012-06-15 05:57:27 PDT
https://hg.mozilla.org/mozilla-central/rev/2448a35540d2
Comment 5 Wesley Johnston (:wesj) 2012-06-15 07:33:58 PDT
*** Bug 765165 has been marked as a duplicate of this bug. ***
Comment 6 Wesley Johnston (:wesj) 2012-06-19 11:07:26 PDT
Comment on attachment 633305 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 713450
User impact if declined: Close animation ain't right. Polish
Testing completed (on m-c, etc.): Landed on central last week
Risk to taking this patch (and alternatives if risky): Low risk fix if we take bug 13450.
String or UUID changes made by this patch: None.
Comment 7 Alex Keybl [:akeybl] 2012-06-19 19:28:05 PDT
Approved for Aurora 15. We'll back the new tabs out if they pose significant issues before Beta.
Comment 8 Wesley Johnston (:wesj) 2012-06-27 16:54:03 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bb8c28d8879

Note You need to log in before you can comment on or make changes to this bug.