[Kde-pim] Review Request: adds kmail support for RFC2369, List-* headers to display in preview pane with links to URLs

Thu Oct 8 16:30:02 BST 2009

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

I agree that this needs a configure option before this patch can be applied.
Fortunately, there is enough space in the message window configure tab for an other checkbox :)
In addition, it might be an idea to have these list links at the bottom of a mail, not at the top. Not sure if that is better, though, just an opinion.

As usual, I have made some inline comments below, please have a look at them.

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

    Please remove the trailing whitespace here

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

    it would be nice to rename style to cssStyle or something like that, since otherwise a variable named style is confusing in a file named headerstyle.cpp :)

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

    Add missing spaces, which are needed because of the coding styles rules, this should be:
    if ( list.size() == 0 ) {
    Or even better, use list.isEmpty()

    Please add those spaces inside of parenthesis also in other places

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

    Should be a kWarning, instead of "warn" in the debug statement

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

    QString() instead of QString::null

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

    This is only used inside the for loop, right? Then it should be declared there, not here.
    In general, variables should be declared as late as possible.

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

    Using foreach here would be significantly less code

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

    This an "web" below need to be i18n-ized

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

    "or" is not i18n-ized

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

    That will not work if " or " is i18n-ized

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

    this "by" needs i18n context, otherwise it can't properly be translated.

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

    This variable shadows another one with the same name, that is a bit confusing

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

    Please move the { to a new line for function definitions.

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

    Please use more descriptive variable names, not slist, flist and mlist.

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

    There is no i18n context here, although it is there for other uses of "List: "

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

    Where is this used?

/trunk/KDE/kdepim/kmail/mailinglist-magic.h
<http://reviewboard.kde.org/r/1768/#comment1915>

    Please use the standard comment style for all comments in this file, which is:

    /**
     * Bla Bla Bla
     */

- Thomas

On 2009-10-05 14:29:35, Daniel Black wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1768/
> -----------------------------------------------------------
> 
> (Updated 2009-10-05 14:29:35)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> This patch displays the List{-ID,post,subscriber,unsubscribe,archive,help,owner} (<a href="http://tools.ietf.org/html/rfc2369">RFC2369</a> List-ID is RFC2919) URLs of email header fields on the view plane in kmail. The display occurs on styles: Enterprise, Fancy, Standard, and Long. The format is:
> List: Linux Australia  Post Unsubscribe(http mailto) Subscribe(http mailto) Archive Help 
> The Post, Archive and Help are links to the single URL present. The Subscribe/Unsubcribe have two links so the http and mailto, extracted from the URL protocol, are links to those two options. Only list tags that exist are show.
> Visual separation from the title and Post.... is a bit weak. Is "List: List Title <Post....>" better?
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/headerstrategy.cpp 1031501 
>   /trunk/KDE/kdepim/kmail/headerstyle.cpp 1031501 
>   /trunk/KDE/kdepim/kmail/mailinglist-magic.h 1031501 
>   /trunk/KDE/kdepim/kmail/mailinglist-magic.cpp 1031501 
> 
> Diff: http://reviewboard.kde.org/r/1768/diff
> 
> 
> Testing
> -------
> 
> tested on kmail-1.12.1 (kde-4.3.1)
> 
> 
> Screenshots
> -----------
> 
> Enterprise Style
>   http://reviewboard.kde.org/r/1768/s/224/
> Fancy Style
>   http://reviewboard.kde.org/r/1768/s/225/
> Long Style
>   http://reviewboard.kde.org/r/1768/s/226/
> 
> 
> Thanks,
> 
> Daniel
> 
>

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