[Kde-pim] Review Request: Junior Job complete: "Re-add ability to hide quick search"

[Thomas McGuire]

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


Sorry for the late answer, I didn't have much time for KDE stuff in the last weeks, should get better now.

Thank you very much for your patch. It is fine and works. I just noticed have a bunch of little things that can be improved, I added those as inline comments below. Please look at them and improve the patch. Looking forward to the new version of the patch.


/trunk/KDE/kdepim/messagelist/core/widgetbase.cpp
<http://reviewboard.kde.org/r/4382/#comment6119>

    mToolButton is a generic name, hard to tell what it actually is. A name like mOpenFullSearchButton would be much more descriptive.



/trunk/KDE/kdepim/messagelist/core/widgetbase.cpp
<http://reviewboard.kde.org/r/4382/#comment6114>

    In this function, you hide all widgets explicitly.
    
    It is better to put the search bar, the toolbutton and the combobox into the same parent widget, and just hide that parent widget.
    The parent widget would simply be an empty QWidget that has 0 margin.



/trunk/KDE/kdepim/messagelist/core/widgetbase.cpp
<http://reviewboard.kde.org/r/4382/#comment6120>

    An idea might be to set the focus to the quick search line after showing it again, and setting the focus to something else (like the message list) when it is hidden.



/trunk/KDE/kdepim/messagelist/core/widgetbase.cpp
<http://reviewboard.kde.org/r/4382/#comment6121>

    Yep, good idea!



/trunk/KDE/kdepim/messagelist/core/widgetbase.cpp
<http://reviewboard.kde.org/r/4382/#comment6115>

    Reviewboard shows that you have trailing spaces here, it marks this line in red. Please remove the trailing spaces your patch adds here and in other places.



/trunk/KDE/kdepim/messagelist/widget.cpp
<http://reviewboard.kde.org/r/4382/#comment6116>

    Please put the new includes in alphabetical order, that helps with preventing duplicate #include lines.
    
    Yes, in this case KComboBox doesn't adhere to that rule either...



/trunk/KDE/kdepim/messagelist/widget.cpp
<http://reviewboard.kde.org/r/4382/#comment6112>

    coding style: In KDEPIM, we use spaces inside of parenthesis, so the better way would be:
      if ( d->XmlGuiClient )
    
    Same for many other places in the patch, I would appreciate if you changed those to use the correct coding style.



/trunk/KDE/kdepim/messagelist/widget.cpp
<http://reviewboard.kde.org/r/4382/#comment6118>

    You can set the text directly in the constructor of KAction, instead of using a seperate setText() call, that is a bit more compact (KAction has multiple constructors)
    
    Another small thing: We don't use underscores ('_') in our variable names, we use camelCase naming scheme instead.
    
    



/trunk/KDE/kdepim/messagelist/widget.cpp
<http://reviewboard.kde.org/r/4382/#comment6113>

    The text in setText() will be user-visible, and therefore has to be marked as translatable, which can be done with the i18n() call, like this:
      quicksearch_vis->setText( i18n( "Show/Hide Quick Search Bar" );
    
    Note that "quicksearchbar" is probably not something that is a valid, "Quick Search Bar" is more correct.



/trunk/KDE/kdepim/messagelist/widget.cpp
<http://reviewboard.kde.org/r/4382/#comment6117>

    setting the object name is not strictly necessary, it is only used for debugging, for example in QObject::dumpObjectTree().
    It doesn't need to be the same as the action identifier in the addAction() call.
    Not important at all, just FYI.


- Thomas


On 2010-06-18 16:02:30, Richard Plangger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4382/
> -----------------------------------------------------------
> 
> (Updated 2010-06-18 16:02:30)
> 
> 
> Review request for KDE PIM and Thomas McGuire.
> 
> 
> Summary
> -------
> 
> Junior Job "Re-add ability to hide quick search" (http://techbase.kde.org/Projects/PIM/KMail_Junior_Jobs#KMail_Junior_Jobs)
> 
> CTRL + H shows hides the searchbar, combobox and toolbutton!
> 
> This is my fist KDE patch, advice & criticism appreciated!
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/messagelist/core/widgetbase.h 1139529 
>   /trunk/KDE/kdepim/messagelist/core/widgetbase.cpp 1139529 
>   /trunk/KDE/kdepim/messagelist/widget.cpp 1139529 
> 
> Diff: http://reviewboard.kde.org/r/4382/diff
> 
> 
> Testing
> -------
> 
> Tested on local machine
> 
> 
> Screenshots
> -----------
> 
> before hitting ctrl + h
>   http://reviewboard.kde.org/r/4382/s/436/
> after
>   http://reviewboard.kde.org/r/4382/s/437/
> 
> 
> Thanks,
> 
> Richard
> 
>

_______________________________________________
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/