487165 - Make kinetic panning nicer

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Stuart Parmenter]

Attachment: fix

OK, Kinetic panning was driving me nuts so I fixed it.

Basically now we take in to account all the movement and for each step of the movement we calculate the speed in pixels/ms.  We then add those up and average them together to figure out our average speed.  When moving, we simply repeat:
 move speed*intervaltime pixels, decelerate, check if speeds are 0 and if so stop

The only real problem with this patch is that we don't fully take locking in to account when doing kinetic stuff, so sometimes you can get it to behave a little weird.

Comment 1

[Ben Combee (:bcombee)]

Comment on attachment 371378 [details] [diff] [review]
fix

Code looks good.  The comment in _dragBy about setDragPositon can be removed.  I like the change to allow momemtunBuffer to grow as much as needed.  average func could just use "for (i in buf)" instead of counting.  I like the new _startKineticTimer code a lot more than before

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 371378 [details] [diff] [review]
fix

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

>+  _startKineticTimer: function _startKineticTimer() {

Some tabs in here screwing up alignment.

>+        if (self._speedX == 0 && self._speedY == 0)
>+          self.endKinetic();

>+      if (!panned)
>+        self.endKinetic();

Should return after each of these, right? Otherwise we'll end up calling endKinetic multiple times unnecessarily, AFAICT.

>   startKinetic: function startKinetic(sX, sY) {

>+    for (let i = 1; i < mblen; i++) {
>+      let me = mb[i];

Could use for each (let me in this.momentumBuffer)

>+    // average the speeds out (This could probably be a bit smarter)
>+    function average(buf) {

could use buf.reduce(function (a,b) a+b)/buf.length, if you want to be fancy.

>+  addData: function addData(sX, sY) {

>+    this.momentumBuffer.push({'t': Date.now(), 'sx' : sX, 'sy' : sY});

Probably wouldn't hurt to use one of sx/sX consistently.

>diff --git a/chrome/content/WidgetStack.js b/chrome/content/WidgetStack.js

>+  // dragBy: process a mouse move by dx,dy for an ongoing drag
>+  dragBy: function dragMove(dx, dy) {

Fix the function name?