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

[Torgny Nyblom]

> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > 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.

Done I think :)


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/headerstyle.cpp, line 623
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10887#file10887line623>
> >
> >     What do the parameters false, true and true mean? Please change those to enums, so it is more readable.

Done


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmreaderwin.h, line 339
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10888#file10888line339>
> >
> >     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.

Done


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmreaderwin.cpp, line 1608
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10889#file10889line1608>
> >
> >     What does this do and why is it needed here?

I just copied the implementation from attachments, but it seems that the DOM is not complete at this stage. Thus if this call is made directly it will fail.


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmreaderwin.cpp, line 2906
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10889#file10889line2906>
> >
> >     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.

Done


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmreaderwin.cpp, line 2909
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10889#file10889line2909>
> >
> >     Q_ASSERT instead of assert please

Done


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmreaderwin.cpp, line 2922
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10889#file10889line2922>
> >
> >     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.

Renamed


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmreaderwin.cpp, line 2936
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10889#file10889line2936>
> >
> >     mini code duplication with the if above: factor out this statement in a bool variable

Done


> On 2009-09-06 15:16:44, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/stringutil.cpp, line 798
> > <http://reviewboard.kde.org/r/1492/diff/2/?file=10891#file10891line798>
> >
> >     I think this should be changed to a Q_ASSERT, as it is the responsibility of the caller to set this properly.

Done


- Torgny


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


On 2009-09-07 16:44:53, Torgny Nyblom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1492/
> -----------------------------------------------------------
> 
> (Updated 2009-09-07 16:44:53)
> 
> 
> 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/pics/attachmentQuicklistClosed.png UNKNOWN 
>   /trunk/KDE/kdepim/kmail/pics/attachmentQuicklistOpened.png UNKNOWN 
>   /trunk/KDE/kdepim/kmail/stringutil.h 1020776 
>   /trunk/KDE/kdepim/kmail/stringutil.cpp 1020776 
>   /trunk/KDE/kdepim/kmail/urlhandlermanager.cpp 1020776 
>   /trunk/KDE/kdepim/kmail/kmreaderwin.h 1020776 
>   /trunk/KDE/kdepim/kmail/kmreaderwin.cpp 1020776 
>   /trunk/KDE/kdepim/kmail/headerstyle.cpp 1020776 
> 
> 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/