Opened 6 years ago

Closed 21 months ago

#9057 closed defect (fixed)

Fix for using-mouse-wheel-makes-window-scroll-forever

Reported by: linmain Owned by:
Priority: normal Milestone:
Component: wxStyledText Version:
Keywords: Cc: linmain, robind
Blocked By: Blocking:
Patch: yes

Description

This fix/optimization attacks one common annoying problem among software: When windows take some time to render themself, and the user scrolls the window up or down, the user will think "hey, scroll faster" and scrolls even heavier. Doing that, the user pushes many wheel events onto the event queue, so that after the user found the right position in the document, the window still processes wheel events and scrolls too far, often over 5 seconds longer than wanted (depending on the rendering complexity).

This patch tries to fix it by measuring the time taken to evaluate and process the current mouse wheel event. When the next event arrives before the current event is processed completely (this is seen by their relative timestamps and the time taken to process the current event), then it is ignored.

This patch has considerably improved the responsibility of the mouse wheel for me, especially with styled text. The patch is attached. It is taken against wx version 2.8.7.

Attachments (3)

better_wheel_responding.patch download (1.8 KB) - added by linmain 6 years ago.
patch in unified patch format
speed_up_scrolling_in_stc.patch download (600 bytes) - added by jens 21 months ago.
speed up scrolling in stc a lot
remove_9057_related_workaround.patch download (2.3 KB) - added by jens 21 months ago.
Remove the workaround introduced with this ticket, becaause it's no longer needed with the speed-up patch

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by linmain

patch in unified patch format

comment:1 Changed 6 years ago by linmain

i'm sorry the first file seems not to be the right .patch format. (i'm still a bit n00bish with diff and patch :))
File Added: better_wheel_responding.patch

comment:2 Changed 6 years ago by wxsite

  • Status changed from assigned to confirmed

transitioning old 'assigned' status to new 'confirmed' status

comment:3 Changed 6 years ago by wojdyr

  • Type set to defect

comment:4 follow-up: Changed 6 years ago by vadz

  • Resolution set to fixed
  • Status changed from confirmed to closed

Thanks, applied to trunk as r54229.

I wonder if this shouldn't be done at wxWindow/mouse wheel event generation level though, it doesn't seem right to do this for wxSTC only as the problem doesn't seem to be STC-specific at all.

I also couldn't really test whether this works as I don't manage to make scrolling in the stc sample to lag (even in a VM) but this seems to do the right thing.

Finally, this can't be applied as is to the 2.8 branch as a new member variable changes the ABI but the patch could probably be modified to use a static variable for it inside wxStyledTextCtrl::OnMouseWheel() itself if you're motivated to do it.

Thanks again!

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by DrewBoo

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to vadz:

Thanks, applied to trunk as r54229. I also couldn't really test whether this works as I don't manage to make scrolling in the stc sample to lag (even in a VM) but this seems to do the right thing.

This bugfix introduces two (arguably) more serious bugs.

The core problem that this patch intends to fix is a computer can be too slow to respond to user input fluidly.

This patch solves that problem by selectively ignoring user input. This is a rather harsh solution -- if I slide my finger quickly along the mouse wheel, wxScintilla now behaves as if I clicked the wheel once. All other motion is ignored. This is not in line with the user's intentions and nothing like the mouse wheel behavior in any other application.

A far more serious bug is that wxMouseEvent::GetTimeStamp() can (and does) return a negative number in some environments. In that scenario, all mouse wheel notifications are ignored, always.

A better solution to the original problem would be to accumulate rapidly occurring scroll requests and execute the requests as a single large scroll. Then, a computer that is struggling to keep up with user input still does what the user wants, sacrificing the fluidity of the scrolling instead of sacrificing the entire request.

I could probably propose a patch, if there is interest.

DrewBoo

comment:6 in reply to: ↑ 5 Changed 5 years ago by vadz

  • Status changed from reopened to infoneeded_new

Replying to DrewBoo:

This patch solves that problem by selectively ignoring user input. This is a rather harsh solution

Yes, absolutely. It's better than nothing but clearly not ideal.

A far more serious bug is that wxMouseEvent::GetTimeStamp() can (and does) return a negative number in some environments.

This is a separate (and serious) bug. What are these environments? This bug should be fixed anyhow.

A better solution to the original problem would be to accumulate rapidly occurring scroll requests and execute the requests as a single large scroll. Then, a computer that is struggling to keep up with user input still does what the user wants, sacrificing the fluidity of the scrolling instead of sacrificing the entire request.

I could probably propose a patch, if there is interest.

We'd be definitely glad to have such patch, TIA!

comment:7 Changed 3 years ago by robind

  • Owner robind deleted
  • Status changed from infoneeded_new to new

I can confirm that the GetTimeStamp() value can be negative, a wxPython user has seen it while trying to figure out the scrolling problem in his application using STC. He is using Windows 7. See https://groups.google.com/forum/#!topic/wxpython-dev/zQ7qmrQ0Jss for the message thread.

comment:8 Changed 3 years ago by vadz

  • Patch unset

Not sure what to do about this... Should we change the type of wxEvent::m_timeStamp to unsigned? Will this even fix a problem? Clearly we'll still have problems at least when the time wraps around but this, of course, should only happen once every 42 days or something like this so it's not too bad.

comment:9 Changed 2 years ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [70500]) Fix bug with mouse wheel scrolling wxStyledTextCtrl in long running programs.

In long running programs the wxEvent time stamp could wrap around resulting in
all mouse wheel events being ignored in wxStyledTextCtrl as the comparison of
the (positive) time until which all the subsequent events were supposed to be
blocked and the (now negative) current event time stamp would be always false.

Fix this by using wxStopWatch::TimeInMicro() to avoid wraparound instead of
wxEvent::GetTimestamp().

Also rename the variable to have a more clear name as the original code wasn't
easy to understand.

Closes #9057.

comment:10 Changed 21 months ago by jens

  • Patch set
  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm one of the Code::Blocks core developers.
Code::Blocks uses a slightly modified (wx)scintilla as editor component.

We had several reports about slow scrolling on some wxGTK systems, mostly ubuntu.

After many tests and discussion I found a possible reason.

I decided to reopen this ticket, because it is directly related to the issue, or better it's a workaround for it.

In r11533, exactly in ScintillaWX.cpp:173 a call to Update() in every call of ScrollText() was introduced.

This forces a repaint for every scroll.

I did a simple mesuring with a stopwatch to get the time needed for the call of ScrollTo() in ScintillaWX::DoMouseWheel().
It takes 40 ~ 100 ms with the call to Update and about 500 µs without the call.
I did not see any painting issues on linux (wx 2.8.12 and 2.9.5 trunk) and windows (2.8.12).

Removing all code related to the workaround introduced here also did not lead to any problems.

So if the call to Update() can safely be removed, the whole workaround might also be obsolete.

I add two patches for each of the changes.

Changed 21 months ago by jens

speed up scrolling in stc a lot

Changed 21 months ago by jens

Remove the workaround introduced with this ticket, becaause it's no longer needed with the speed-up patch

comment:11 Changed 21 months ago by vadz

I agree that this Update() should be removed, scrolling should work without it. And it does seem to work for me too in wxGTK, so if there are no objections I'm going to commit this.

Thanks Jens!

comment:12 Changed 21 months ago by VZ

(In [72254]) Remove unnecessary Update() in wxStyledTextCtrl scrolling code.

Calling Update() every time ScrollText() dramatically slowed down scrolling
and doesn't seem to be necessary, so remove it.

See #9057.

comment:13 Changed 21 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [72255]) Remove workaround for slow scrolling in wxStyledTextCtrl.

We don't need to drop mouse wheel events coming too fast after the previous
commit as now scrolling in wxStyledTextCtrl works quickly enough.

Closes #9057.

Note: See TracTickets for help on using tickets.