Last Comment Bug 765069 - Close by swipe velocity checks are wrong
: Close by swipe velocity checks are wrong
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
: Sebastian Kaspari (:sebastian)
: 765165 (view as bug list)
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 User image Wesley Johnston (:wesj) 2012-06-14 15:58:03 PDT
Created attachment 633305 [details] [diff] [review]

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 User image Wesley Johnston (:wesj) 2012-06-14 15:58:54 PDT
Comment on attachment 633305 [details] [diff] [review]

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

>+    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 User image Ed Morley [:emorley] 2012-06-15 05:57:27 PDT
Comment 5 User image Wesley Johnston (:wesj) 2012-06-15 07:33:58 PDT
*** Bug 765165 has been marked as a duplicate of this bug. ***
Comment 6 User image Wesley Johnston (:wesj) 2012-06-19 11:07:26 PDT
Comment on attachment 633305 [details] [diff] [review]

[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 User image 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 User image Wesley Johnston (:wesj) 2012-06-27 16:54:03 PDT

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