[Kde-pim] Re: Review Request: option to close kmail tabs with middle click

[Thomas McGuire]

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


Thanks for the patch. Marius.
I added some inline comments to this patch. All of them are nitpicks, i.e. minor comments that are not that important, mostly about coding style. Still it would be nice to have them fixed.
The other one is about not calling eventFilter() of the superclass, which is the only real improvement the patch needs.
Please update your patch with the proposed changed and upload it again.


messagelist/pane.cpp
<http://git.reviewboard.kde.org/r/101632/#comment3247>

    For consistency in this file, it should probably be a QtGui/ include, like the others. There is no real reason for that though.



messagelist/pane.cpp
<http://git.reviewboard.kde.org/r/101632/#comment3249>

    "this->" should be unnecessary here, please remove.
    Also, may I introduce you to the special KDEPIM coding style: We have spaces at the inside of the parenthesis, so it is "( this )" instead of "(this)" in the current line. Applies to other lines as well.



messagelist/pane.cpp
<http://git.reviewboard.kde.org/r/101632/#comment3250>

    can be QMouseEvent * const mouseEvent, for better const-correctness.



messagelist/pane.cpp
<http://git.reviewboard.kde.org/r/101632/#comment3251>

    Indentation: This line doesn't seem to have the same amount of spaces in front of it than the line before.



messagelist/pane.cpp
<http://git.reviewboard.kde.org/r/101632/#comment3252>

    This should be:
    return KTabWidget::eventFilter( object, event)
    
    Otherwise, in case if KTabWidget or QTabWidget reimplement that function, it would never get called.


- Thomas


On June 15, 2011, 6:08 p.m., Marius Knaust wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101632/
> -----------------------------------------------------------
> 
> (Updated June 15, 2011, 6:08 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Summary
> -------
> 
> Possible fix using KTabWidget instead of QTabWidget and a eventFilter to catch
> the MidButton MouseButtonPress event to avoid the tab selection.
> 
> 
> This addresses bug 207270.
>     http://bugs.kde.org/show_bug.cgi?id=207270
> 
> 
> Diffs
> -----
> 
>   messagelist/pane.h d019b78 
>   messagelist/pane.cpp ab1473d 
> 
> Diff: http://git.reviewboard.kde.org/r/101632/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marius
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/