Closed Bug 765069 Opened 9 years ago Closed 9 years ago

Close by swipe velocity checks are wrong

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox15 fixed)

RESOLVED FIXED
Firefox 16
Tracking Status
firefox15 --- fixed

People

(Reporter: wesj, Unassigned)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter 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 on attachment 633305 [details] [diff] [review]
Patch

I haven't had a good mbrubeck review in awhile. You want to look at this?
Attachment #633305 - Attachment is patch: true
Attachment #633305 - Flags: review?(mbrubeck)
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?
Attachment #633305 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/2448a35540d2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Duplicate of this bug: 765165
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.
Attachment #633305 - Flags: approval-mozilla-aurora?
Approved for Aurora 15. We'll back the new tabs out if they pose significant issues before Beta.
Attachment #633305 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.