701586 - Axis lock during panning

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Patrick Walton (:pcwalton)]

birch-pan-zoom needs an axis lock during panning.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add axis lock (WIP)

I think this needs more work, but I need feedback from people who try it out first.

Comment 2

[Patrick Walton (:pcwalton)]

Comment on attachment 575503 [details] [diff] [review]
Add axis lock (WIP)

Review of attachment 575503 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Though I'm just going to trust the trigonometry is correct.

::: embedding/android/ui/PanZoomController.java
@@ +177,2 @@
>          case PANNING_HOLD:
> +            // fall through

nit: I've seen it written as "// FALL THROUGH" right before the next "case" in Mozilla code.

@@ +255,5 @@
> +            } else if (Math.abs(angle - (Math.PI / 2)) < AXIS_LOCK_ANGLE) {
> +                // lock to y-axis
> +                x = mX.firstTouchPos;
> +            } else {
> +                // break axis lock but log the angle so we can fine-tune this when people complain

:)

@@ +274,5 @@
> +                mState = PanZoomState.PANNING_HOLD;
> +            } else if (mState == PanZoomState.PANNING_LOCKED) {
> +                mState = PanZoomState.PANNING_HOLD_LOCKED;
> +            } else {
> +                // should never happen, but handle anyway for robustness

Not relevant to this patch, but we really should have some kind of non-fatal-in-production, fatal-in-debug assertion setup...

Comment 3

[Kartikaya Gupta (email:[e-mail])]

> nit: I've seen it written as "// FALL THROUGH" right before the next "case"
> in Mozilla code.

Fixed; I was juggling some lines around and missed that.

> 
> Not relevant to this patch, but we really should have some kind of
> non-fatal-in-production, fatal-in-debug assertion setup...

Yeah, agreed. Usually the java assert statement satisfies this criteria, and it seems like we can do this on android - assertions can be enabled by "adb shell setprop dalvik.vm.enableassertions all". I'll test it out and add a note on the java idioms page.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

http://hg.mozilla.org/projects/birch/rev/ec82d9d943f9

I'll leave the bug open for a bit to see if there's any complaints on the behaviour.

Comment 5

[Catalin Suciu [:csuciu]]

Verified fixed on:
Nightly 13.0a1 (2012-02-21)
Beta 11.0 (20120221151724)
Device: Samsung Galaxy SII (Android 2.3.4)