Allow slower minimum speed for autoscroll

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: svl-bmo, Assigned: csthomas)

Tracking

({fixed-seamonkey1.1a})

Trunk
fixed-seamonkey1.1a
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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.

Comment 2

13 years ago
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).
Created attachment 207003 [details] [diff] [review]
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.
Attachment #207003 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 4

13 years ago
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]
Created attachment 210672 [details] [diff] [review]
patch
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 6

13 years ago
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.
Created attachment 214274 [details] [diff] [review]
patch

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 10

13 years ago
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

13 years ago
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
Last Resolved: 13 years ago
Keywords: fixed-seamonkey1.1a
Resolution: --- → FIXED
(Reporter)

Comment 13

13 years ago
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 → ---
Attachment #269009 - Flags: review?(beltzner)
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
Last Resolved: 13 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.