Review Request 124670: Scrolling fixes for Dolphin

[Maxim Mikityanskiy]

> On Авг. 9, 2015, 11:13 д.п., Mark Gaiser wrote:
> > src/kitemviews/private/kitemlistsmoothscroller.cpp, line 175
> > <https://git.reviewboard.kde.org/r/124670/diff/1/?file=391356#file391356line175>
> >
> >     The event is passed on to handleWheelEvent which in turn calls event->accept(). The Qt documentation for that states:
> >     "Setting the accept parameter indicates that the event receiver wants the event. Unwanted events might be propagated to the parent widget."
> >     
> >     The way i read it is that you can either accept it or return true (also accepting the event). You do both now. While that is fine, i wonder if it changes anything.

You are talking about different thing. event->accept() and event->ignore() are to be used in QWidget event handlers e.g. inside QWidget::wheelEvent() or QWidget::mousePressEvent(). QWidgets are nested, so at given point on the screen there may be multiple widgets. For example, if you have two QPushButtons with common parent QWidget that contains them, and you press one of the buttons, QPushButton's mousePressEvent is called, and if this handler does not accept the event (although real QPushButton::mousePressEvent will accept it), parent QWidget's mousePressEvent will be also called. So event->accept()'s purpose is to prevent event from propagating to parent widgets if our widget handled it.

We have a different case there. We have event filter, not the event handler. Please look here: http://doc.qt.io/qt-5/eventsandfilters.html#event-filters

>When the filter object's eventFilter() implementation is called, it can accept or reject the event, and allow or deny further processing of the event. If all the event filters allow further processing of an event (by each returning false), the event is sent to the target object itself. If one of them stops processing (by returning true), the target and any later event filters do not get to see the event at all.

It means that if I return true from event filter, QWidget's event handler will not be called at all. And if I return false from filter, QWidget's event handler will be called as usual. event->accept() and event->ignore() are only relevant when called from event handler, so accepting event from filter has no effect, and it still arrives to QScrollBar and causes extra scroll, because we don't return true from filter.


- Maxim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124670/#review83598
-----------------------------------------------------------


On Авг. 9, 2015, 9:52 д.п., Maxim Mikityanskiy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124670/
> -----------------------------------------------------------
> 
> (Updated Авг. 9, 2015, 9:52 д.п.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> KItemListSmoothScroller::handleWheelEvent has some issues.
> 
> At first, when I scroll file list holding mouse over the list, one mouse wheel tick corresponds to 1/4 page interval, but when I hover on QScrollBar, one wheel tick corresponds to 1 page interval.
> 
> At second, in KItemListSmoothScroller::eventFilter we don't return true, so that QScrollBar also handles this event, and total scroll interval is m_scrollBar->pageStep() + m_scrollBar->singleStep().
> 
> At third, when I use touchpad that supports smooth scrolling via XInput2, and I hover it over QScrollBar, I can only scroll content if I move my fingers very fast, because numSteps = event->delta() / 8 / 15 is just zero unless I move very fast (event->delta() in this case is less than 120).
> 
> At fourth, holding Shift while scrolling has no effect when holding mouse over QScrollBar in contrast to scrolling faster when holding mouse over file list.
> 
> The patch eliminates all these issues making the behavior of KItemListSmoothScroller the same as in KItemListContainer::wheelEvent, adding support for QWheelEvent::pixelDelta() and removing usage of deprecated QWheelEvent::delta().
> 
> 
> Diffs
> -----
> 
>   src/kitemviews/private/kitemlistsmoothscroller.cpp e70f478 
> 
> Diff: https://git.reviewboard.kde.org/r/124670/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Maxim Mikityanskiy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20150809/8fbbb9ab/attachment.htm>