Closed Bug 487165 Opened 13 years ago Closed 13 years ago

Make kinetic panning nicer

Categories

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

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pavlov, Unassigned)

References

Details

Attachments

(1 file)

Attached patch fixSplinter Review
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.
Attachment #371378 - Flags: superreview?(gavin.sharp)
Attachment #371378 - Flags: review?(combee)
Attachment #371378 - Flags: review?(combee) → review+
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
Attachment #371378 - Flags: superreview?(gavin.sharp) → review+
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?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.