321447 - Allow slower minimum speed for autoscroll

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, enhancement)

Description

[Sander]

Depending on line-width / technical density of text, I find the minimum speed for autoscroll slightly too fast to be able to keep it running and read a large piece of text that way - and I am not a slow reader. A slowdown of the minimum speed by 50% (ideally controlled by a preference) would be greatly appreciated.

Snippets from discussion in IRC:
<NeilAway> CTho|away: you can't scroll less than 1 pixel
<CTho|away> NeilAway: we could accumulate rounding error and handle it manually ;)
<NeilAway> CTho|away: yeah, that would work
<NeilAway> I suppose we could have a general.autoscroll.interval pref
<CTho|away> i unddersatnd that we can't easily scroll by subpixel amounts
<CTho|away> comment (cc me?) with info that'll be useful, and whether you want to pref the timer, or do our own subpixel tracking
<NeilAway> well, subpixel tracking sounds nicer right now

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Ok, this doesn't seem too hard.  One question: do we keep the "dead" zone at the center at ~4px?  Or do we make any non-zero movement scroll (though very slowly)?  

Either way is not difficult.   It was ~14 lines of code (~7 lines for x, ~7 for y) so far, with no "dead" zone.

Comment 2

[[e-mail]]

If you want to keep the dead zone then the only way to reduce the minimum speed is to pref the timer interval. Side note: for even more accurate calculation you could give the autoScrollLoop a dummy "lateness" argument; its value is in milliseconds (but has been observed to be negative which is the OS's fault).

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: how about something like this?

This allows for a slow minimum speed, keeps a dead zone at the center, and makes speed increase more than linearly so you can still get to high scroll speeds.  Ignore the cursor setting lines.

Comment 4

[[e-mail]]

Comment on attachment 207003 [details] [diff] [review]
how about something like this?

>+  if (x > 0)
>+    x = Math.pow(x, accel);
>+  else
>+    x = -1*Math.pow(-x, accel);
...
>+  if (x > 1)
>+    x--;
>+  else if (x < -1)
>+    x++;
>+  else
>+    x = 0
If we do end up with a nonlinear scheme, combine the two expressions:
if (x > 1)
  x = x * Math.sqrt(x) - 1;
else if (x < -1)
  x = x * Math.sqrt(-x) + 1;
else
  x = 0;
[I don't know if sqrt is faster than pow]

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 6

[[e-mail]]

Comment on attachment 210672 [details] [diff] [review]
patch

[slightly off topic] line 609 is unused, isn't it?

Comment 7

[Daniel Brooks [:db48x]]

Comment on attachment 210672 [details] [diff] [review]
patch

> function autoScrollLoop()
> {
>-  var x = gCurrX - gStartX;
>-  var y = gCurrY - gStartY;
>-  const speed = 4;
>-  if (Math.abs(x) >= speed || Math.abs(y) >= speed)
>+  var speed = 12;
>+  var x = (gCurrX - gStartX) / speed;
>+  var y = (gCurrY - gStartY) / speed;
>+  if (x > 1)
>+    x = x * Math.sqrt(x) - 1;
>+  else if (x < -1)
>+    x = x * Math.sqrt(-x) + 1;
>+  else
>+    x = 0;
>+  if (y > 1)
>+    y = y * Math.sqrt(y) - 1;
>+  else if (y < -1)
>+    y = y * Math.sqrt(-y) + 1;
>+  else
>+    y = 0;
>+
>+  if (x || y)
>     gIgnoreMouseEvents = false;
>-  gScrollingView.scrollBy(x / speed, y / speed);
>+
>+  var desiredScrollX = gAccumulatedScrollErrorX + x;
>+  var actualScrollX = roundToZero(desiredScrollX);
>+  gAccumulatedScrollErrorX = (desiredScrollX - actualScrollX);
>+  var desiredScrollY = gAccumulatedScrollErrorY + y;
>+  var actualScrollY = roundToZero(desiredScrollY);
>+  gAccumulatedScrollErrorY = (desiredScrollY - actualScrollY);
>+  gScrollingView.scrollBy(actualScrollX, actualScrollY);
> }
> 

You know, instead of repeating all of the code for x and y, it'd be nice if you had a function you could call once for x and once for y.

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Move the duplicated code into a helper function.  Carrying forward sr=neil

Comment 9

[Daniel Brooks [:db48x]]

Comment on attachment 214274 [details] [diff] [review]
patch

r=db48x

Comment 10

[[e-mail]]

Comment on attachment 214274 [details] [diff] [review]
patch

>+    if (val > 1)
>+        return val * Math.sqrt(val) - 1;
>+    else if (val < -1)
>+        return val * Math.sqrt(-val) + 1;
>+    return 0;
50% correct: you remembered not to put the else before the return 0; but you don't need the other else either.

Comment 11

[Robert Kaiser]

Comment on attachment 214274 [details] [diff] [review]
patch

a=me for SeaMonkey 1.1, given you address Neil's last nit

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Checked in after addressing comment #10

Comment 13

[Sander]

works beautifully - thanks. :)

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

We regressed this with the switch to toolkit (I didn't implement it in bug 242621).  I think this should be fixed in toolkit.

Comment 15

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch for toolkit

Comment 16

[Asaf Romano (gone)]

Comment on attachment 269009 [details] [diff] [review]
patch for toolkit

with comment 10 fixed again ;)

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

<Mano> get post-facto ui-r from beltzner or mconnor 

Checked in.