Closed Bug 321447 Opened 15 years ago Closed 14 years ago

Allow slower minimum speed for autoscroll

Categories

(Toolkit :: XUL Widgets, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: svl-bmo, Assigned: csthomas)

Details

(Keywords: fixed-seamonkey1.1a)

Attachments

(2 files, 2 obsolete files)

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
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.
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).
Attached patch how about something like this? (obsolete) — Splinter Review
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.
Attachment #207003 - Flags: superreview?(neil.parkwaycc.co.uk)
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]
Attached patch patch (obsolete) — Splinter Review
Attachment #207003 - Attachment is obsolete: true
Attachment #210672 - Flags: superreview?(neil)
Attachment #210672 - Flags: review?(jag)
Attachment #207003 - Flags: superreview?(neil)
Attachment #207003 - Flags: review?(jag)
Comment on attachment 210672 [details] [diff] [review]
patch

[slightly off topic] line 609 is unused, isn't it?
Attachment #210672 - Flags: superreview?(neil) → superreview+
Attachment #210672 - Flags: review?(jag) → review?(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.
Attached patch patchSplinter Review
Move the duplicated code into a helper function.  Carrying forward sr=neil
Attachment #210672 - Attachment is obsolete: true
Attachment #214274 - Flags: superreview+
Attachment #214274 - Flags: review?(db48x)
Attachment #210672 - Flags: review?(db48x)
Comment on attachment 214274 [details] [diff] [review]
patch

r=db48x
Attachment #214274 - Flags: review?(db48x) → review+
Attachment #214274 - Flags: approval-seamonkey1.1a?
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 on attachment 214274 [details] [diff] [review]
patch

a=me for SeaMonkey 1.1, given you address Neil's last nit
Attachment #214274 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
Checked in after addressing comment #10
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
works beautifully - thanks. :)
Status: RESOLVED → VERIFIED
We regressed this with the switch to toolkit (I didn't implement it in bug 242621).  I think this should be fixed in toolkit.
Status: VERIFIED → REOPENED
Component: XP Apps: GUI Features → XUL Widgets
Flags: superreview+
Flags: approval-seamonkey1.1a+
Product: Mozilla Application Suite → Toolkit
Resolution: FIXED → ---
Comment on attachment 269009 [details] [diff] [review]
patch for toolkit

with comment 10 fixed again ;)
Attachment #269009 - Flags: review?(mano) → review+
<Mano> get post-facto ui-r from beltzner or mconnor 

Checked in.
Status: REOPENED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.