Review Request: Fix for CTRL+Tab not switcing tabs in Konqueror when the active tab is a Dolphin filemanagement part

[Dawit Alemayehu]

> On Sept. 25, 2012, 9:03 p.m., Frank Reininghaus wrote:
> > Thanks for analysing this issue and for the patch!
> > 
> > We should of course let those events propagate. However, I'm a bit confused. The docs of QObject::eventFilter() don't mention anything about setting the 'accepted' state of the event. If I understand it correctly, the event is filtered (i.e., not handled further) only if eventFilter() returns true.
> > 
> > Even if your patch fixes the issue, I'm therefore wondering if it just hides the real bug.
> > 
> > I think that what is actually supposed to happen is that DolphinView does not filter the event and that the KItemListView does not handle the event either, such that the unhandled event is passed through to the parent widgets until it ends up in the KonqCombo, such that KonqMainWindow::eventFilter() gets it, right? If that is true, I think the real bug might be that some class pretends to handle the event, such that its parents don't see it, although it does nothing with the event.
> > 
> > I had a quick look at the code and found that KItemListView::event() accepts the event and returns true if m_controller->processEvent() returns true, but does not ignore() the event if that is not the case. I haven't tested it yet, but maybe that is the real problem?
> 
> Thomas Lübking wrote:
>     Accepting the event means that it's not further propagated (to the parenting widget)
>     returning "true" in the event filter means to "eat" the event, ie. prevent regular handling in following eventfilters or even the widget event handling itself.
>     
>     The patch will ensure that the (input, i assume) event goes up to the parent widget and is handled there if nothing else accepts it.
>     If you returned true, no further processing would happen at all (ie. even DolphinView::event() would process it)
>     
>     You should btw. ensure that the parenting QScrollArea accepts it, or oxygen will get bugs because selecting items in dolphin will start to move the window ;-)
>     
>     Disclaimer: i've not even seen the bug, just generally commented on question and the one-liner diff
> 
> Thomas Lübking wrote:
>     PS: the actually more correct fix was to align to the Qt::WA_NoMousePropagation attribute and set it accordingly (allowing the usercode to manage the behavior this way)
> 
> Frank Reininghaus wrote:
>     Thanks Thomas for your message.
>     
>     > Accepting the event means that it's not further propagated (to the parenting widget)
>     > returning "true" in the event filter means to "eat" the event, ie. prevent regular handling in following eventfilters or even the widget event handling itself.
>     
>     Yes, that's just what I thought.
>     
>     > The patch will ensure that the (input, i assume) event goes up to the parent widget and is handled there if nothing else accepts it.
>     
>     Yes, but what I meant is that the patch ensures that by "ignore()'ing" the event in the wrong place. AFAIU, the eventFilter() should not mess around with the 'accepted' state of the event at all. I think the actual problem is that some event() method does not handle the event, but forgets to ignore() it.
>     
>     > You should btw. ensure that the parenting QScrollArea accepts it, or oxygen will get bugs because selecting items in dolphin will start to move the window ;-)
>     
>     Hm, but then I don't see how the key press should actually reach one of the parents inside Konqueror, but maybe I've misunderstood how Konqueror's Control+Tab feature actually works. Maybe I have to debug the path that the event takes step by step when I find some time.

No need because I can answer that question for you. The events reach inside Konqueror because Konqueror's view class (KonqView) installs an event filter on the widget of the part it embeds just like DolphinView does here. It does it so that it can intercept certain events just like DolphinView. 

Anyhow, none of that really matters because you are indeed correct. The culprit in this case is the KItemListView::event function. In order to allow the event propagation up the chain, it should call event->ignore() when it does not consume events just like it calls event->accept() for the events it does. Simply returning true or false is not enough for the events to be sent up the chain for the reasons already discussed above. I will update the patch accordingly. 


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106569/#review19423
-----------------------------------------------------------


On Sept. 25, 2012, 3:38 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106569/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2012, 3:38 p.m.)
> 
> 
> Review request for Dolphin and KDE Base Apps.
> 
> 
> Description
> -------
> 
> This patch fixes a bug where pressing CTRL+Tab does not switch tabs when active tab in Konqueror is a Dolphin's filemanagement part. It happens because DolphinView installs an event filter and does not call event->ignore() or event->setAccepted(false) to allow those events to be propagated up the event chain.
> 
> 
> This addresses bug 302329.
>     http://bugs.kde.org/show_bug.cgi?id=302329
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/dolphinview.cpp 0584972 
> 
> Diff: http://git.reviewboard.kde.org/r/106569/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120926/a1bc6635/attachment.htm>