Opened 9 months ago

Closed 7 months ago

Last modified 7 months ago

#15357 closed defect (fixed)

wxHVScrolledWindow does not trigger SCROLLWIN event handler during vertical scrolling

Reported by: eco Owned by:
Priority: normal Milestone: 3.0.0
Component: GUI-generic Version: 2.9.5
Keywords: Cc:
Blocked By: Blocking:
Patch: yes

Description

I'm upgrading our product from wx 2.8 to wx 2.9.5 and one snag I hit was wxHVScrolledWindow is not calling our EVT_SCROLLWIN event handler when the user scrolls vertically (but does when the user scrolls horizontally).

The wxHVScrolledWindow we were using with 2.8 was just a patch I made for 2.8 from when I originally wrote wxHVScrolledWindow (and Bryan gussied up for inclusion in wx) but looking at a diff it appears wxHVScrolledWindow is largely the same with the exception of:

"event.Skip(wasSkipped);" was removed from an "if ( processed )" inside the scroll event helper but restoring that if block didn't help.

The bug is easy to reproduce with the vscroll sample. Just stick a breakpoint in the HVScrollWindow::OnScroll event handler and scroll vertically (after switching the Mode to Horizontal/Vertical in the GUI). Interestingly VScrollWindow::OnScroll works fine so I suspect some sort of multiple inheritance or multiple pushed event handler problem (why it would manifest in 2.9.5 but not 2.8.x I don't know).

I tried adding an override to ProcessEvent to grab it before wxHVScrolledWindow's scroll event helper could get its hands on the event but the event never shows up there (horizontal SCROLLWIN events show up though).

I'm going to keep investigating but any insight would be great as my overall understanding of event processing is somewhat lacking.

Attachments (1)

fix-vscroll-scrollwin.patch download (9.3 KB) - added by eco 9 months ago.
Fix wxHVScrolledWindow SCROLLWIN event propagation by porting recent wxScrollHelperEvtHandler::ProcessEvent changes

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 months ago by eco

Another clue. Changing the order of the base classes in the inheritance list on wxVarHVScrollHelper flips the behavior so that SCROLLWIN events are received when scrolling vertically but events while scrolling horizontally are not received.

comment:2 Changed 9 months ago by eco

So returning false when SCROLLWIN events come into wxVarScrollHelperEvtHandler::ProcessEvent causes the events to show up in my class's SCROLLWIN handler.

I noticed that wxVarScrollHelperEvtHandler::ProcessEvent is based on wxScrollHelperEvtHandler::ProcessEvent but that function has been updated since. I'm wondering if there were some changes to event handling that haven't been taken into account in wxVarScrollHelperEvtHandler::ProcessEvent and that's why this all worked in 2.8.x but not in 2.9.5. I'll experiment with porting the changes made to wxScrollHelperEvtHandler::ProcessEvent to wxVarScrollHelperEvtHandler.

Changed 9 months ago by eco

Fix wxHVScrolledWindow SCROLLWIN event propagation by porting recent wxScrollHelperEvtHandler::ProcessEvent changes

comment:3 Changed 9 months ago by eco

Still testing to see if this patch fixes everything.

comment:4 Changed 9 months ago by eco

The patch appears to be working perfectly (and as a bonus keyboard scrolling now works).

I didn't port two of the sections:

  1. Child focus handling as porting that would have been much more involved and not really related to this issue.
  2. Mouse enter/leave used for auto-scrolling because wx[H/V/HV]ScrolledWindow doesn't support auto-scrolling (were Bryan's efforts there never merged?).

comment:5 Changed 9 months ago by eco

  • Patch set

comment:6 follow-up: Changed 9 months ago by vadz

  • Milestone set to 3.0

Thanks for looking at this!

I'm probably going to end up by committing this even though I don't like to increase the amount of the duplicated code even more -- but I just don't believe I'm going to have time to do anything about it before 3.0.

One thing I'd really like to do would be to break this patch in separate changes instead of mixing several apparently unrelated things such as keyboard navigation and scroll event handling. If you already have the intermediate version of the patch, it would be great if you could please submit it, otherwise I'll try to do it myself.

Thanks again!

comment:7 in reply to: ↑ 6 Changed 9 months ago by eco

Replying to vadz:

Thanks for looking at this!

I'm probably going to end up by committing this even though I don't like to increase the amount of the duplicated code even more -- but I just don't believe I'm going to have time to do anything about it before 3.0.

One thing I'd really like to do would be to break this patch in separate changes instead of mixing several apparently unrelated things such as keyboard navigation and scroll event handling. If you already have the intermediate version of the patch, it would be great if you could please submit it, otherwise I'll try to do it myself.

Thanks again!

I don't have just the necessary changes broken out, unfortunately. I did it all in one pass.

comment:8 Changed 7 months ago by vadz

I'm going to commit my somewhat (but still not sufficiently) refactored version of this patch, please retest and let me know if I broke anything. TIA!

comment:9 Changed 7 months ago by VZ

(In [74813]) Add wxAnyScrollHelperBase to reduce code duplication in wxVarScrollHelperBase.

This is just a small refactoring to move some trivially common parts of
wxScrollHelperBase and wxVarScrollHelperBase in a new common base class.
This will make it possible to apply other corrections to wxVarScrollHelperBase
without having to physically duplicate the code from wxScrollHelperBase in it.

See #15357.

comment:10 Changed 7 months ago by VZ

(In [74814]) Propagate the event handling fixes to wxVarScrollHelperBase.

Merge the fixes to wxScrollHelperBase::ProcessEvent() of r64358, r64370,
r64464, r72939 and possibly a few more in wxVarScrollHelperBase to fix its
behaviour too, as it wasn't generating the correct events any longer.

Unfortunately the fix right now is to physically copy the code from one class
to the other. This should be avoided, of course, and a more in depth
refactoring should be done to move the code common to both classes into
wxAnyScrollHelperBase after 3.0 release. But for now continuing to duplicate
code is better than not having a working wxVarScrollHelperBase.

See #15357.

comment:11 Changed 7 months ago by VZ

(In [74815]) Make default keyboard handling available in wxVarScrollHelperEvtHandler too.

Factor out the keyboard handling code in wxAnyScrollHelperBase allowing its
reuse in wxVarScrollHelperEvtHandler.

Now wxVarScrollHelperBase handles cursor keys in a sane way by default
too and also allows disabling their handling, just as wxScrolledWindow.

See #15357.

comment:12 Changed 7 months ago by VZ

(In [74816]) Disable handling of wxEVT_MOUSEWHEEL in wxVarScrollHelperEvtHandler in wxGTK.

Just for consistency with wxScrollHelperBase, not really sure what problem
exactly does this solve.

See #15357.

comment:13 Changed 7 months ago by vadz

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

comment:14 Changed 7 months ago by eco

Everything seems to be working with your changes. Thanks.

Note: See TracTickets for help on using tickets.