[Kde-pim] Review Request: Expandable/Collapsible "To" and "Cc" fields in KMail

[Thomas McGuire]

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


Nice, I like this very much.

The patch needs a few improvements though:

- When collapsed, the list ends with a comma, which looks a bit strange, IMHO it should end with ", ...".

- In the UrlHandler, you should also set the status bar text, else hovering over the expand button shows "kmail:showFullToAdressList".

- Adress should be "Address", it is misspelled in some places.

- Coding style: Some places where there are not enough spaces

- Coding style: New methods in .h files should have a short API documentation

As always, I have made some remarks inline as well, see below.


/trunk/KDE/kdepim/kmail/headerstyle.cpp
<http://reviewboard.kde.org/r/1492/#comment1538>

    What do the parameters false, true and true mean? Please change those to enums, so it is more readable.



/trunk/KDE/kdepim/kmail/kmreaderwin.h
<http://reviewboard.kde.org/r/1492/#comment1545>

    I know the other methods above don't do it either, but I think the implementation should be in the .cpp file, which is cleaner.



/trunk/KDE/kdepim/kmail/kmreaderwin.cpp
<http://reviewboard.kde.org/r/1492/#comment1540>

    What does this do and why is it needed here?



/trunk/KDE/kdepim/kmail/kmreaderwin.cpp
<http://reviewboard.kde.org/r/1492/#comment1539>

    I always get two warnings here, for To and CC, when selecting a message with few addresses.
    
    So I think there shouldn't be a warning here.



/trunk/KDE/kdepim/kmail/kmreaderwin.cpp
<http://reviewboard.kde.org/r/1492/#comment1541>

    Q_ASSERT instead of assert please



/trunk/KDE/kdepim/kmail/kmreaderwin.cpp
<http://reviewboard.kde.org/r/1492/#comment1542>

    Looks like the icon name doesn't fit anymore.
    Either rename the icon to something more generic or make a copy of the icon with a name specific to this usecase.



/trunk/KDE/kdepim/kmail/kmreaderwin.cpp
<http://reviewboard.kde.org/r/1492/#comment1543>

    mini code duplication with the if above: factor out this statement in a bool variable



/trunk/KDE/kdepim/kmail/stringutil.cpp
<http://reviewboard.kde.org/r/1492/#comment1544>

    I think this should be changed to a Q_ASSERT, as it is the responsibility of the caller to set this properly.


- Thomas


On 2009-08-31 11:19:09, Torgny Nyblom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1492/
> -----------------------------------------------------------
> 
> (Updated 2009-08-31 11:19:09)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Makes the "To" and "Cc" fields collapsible if more then three addresses are present in the respective field.
> Image is borrowed from the same function for attachments, but code is slightly different.
> 
> As of now this is just enabled for "Fancy headers" but the concept is easily expanded into all kinds of header styles.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/headerstyle.cpp 1017748 
>   /trunk/KDE/kdepim/kmail/kmreaderwin.h 1017748 
>   /trunk/KDE/kdepim/kmail/kmreaderwin.cpp 1017748 
>   /trunk/KDE/kdepim/kmail/stringutil.h 1017748 
>   /trunk/KDE/kdepim/kmail/stringutil.cpp 1017748 
>   /trunk/KDE/kdepim/kmail/urlhandlermanager.cpp 1017748 
> 
> Diff: http://reviewboard.kde.org/r/1492/diff
> 
> 
> Testing
> -------
> 
> Built, opened test email in diffent header styles and expanded/collapsed the headers.
> 
> 
> Screenshots
> -----------
> 
> Collapsed
>   http://reviewboard.kde.org/r/1492/s/195/
> One expanded, one collapsed
>   http://reviewboard.kde.org/r/1492/s/196/
> Expanded
>   http://reviewboard.kde.org/r/1492/s/197/
> 
> 
> Thanks,
> 
> Torgny
> 
>

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