[Kde-pim] Review Request: Sort template Insert Command menu list

[Thomas McGuire]

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


Mostly ok, except for minor coding style things and the translation issue.
I suggest reading http://techbase.kde.org/Development/Tutorials/Localization/i18n (although sadly that doesn't seem to cover I18N_NOOP)

Thanks for reducing the code duplication here!


/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment808>

    the actual structure definition is defined four times, that is code duplication.
    Please only use one definition but 4 instances (OriginalCommands, CurrentCommands etc



/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment806>

    cdng stl: dnt abbrvt



/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment807>

    coding style: start variable names with lower case



/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment813>

    This should be only "Subject", which you dropped.
    The rest is context for the translators, because "Subject" can be ambiguous.
    
    Use I18N_NOOP2 here (see a comment below for the reason)
    
    If there are other dropped contexts in the list, they should be re-added.



/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment812>

    make this method static.
    Also, give it another name, this sounds like it inserts an action map into something else.



/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment811>

    coding style: indent this so that the K of KActionMenu is right under the c of const QMap



/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment810>

    use constBegin() and constEnd()



/trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp
<http://reviewboard.kde.org/r/809/#comment809>

    This i18n will not work, as the translation scripts can't extract the strings.
    The translation scripts usually extract the literal string right after "i18n(", they have no way to know what OriginalCommands[i].name would return.
    
    Grep for I18N_NOOP in KMail to see how that can be solved


- Thomas


On 2009-06-10 02:12:38, Jonathan Armond wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/809/
> -----------------------------------------------------------
> 
> (Updated 2009-06-10 02:12:38)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Template Insert Command menu is currently loosely sorted in English. This patch sorts it according to translated string. Also reduces code duplication.
> 
> 
> This addresses bug 188394.
>     https://bugs.kde.org/show_bug.cgi?id=188394
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/templatesinsertcommand.cpp 979267 
> 
> Diff: http://reviewboard.kde.org/r/809/diff
> 
> 
> Testing
> -------
> 
> Insert Command menu items are sorted and still work.
> 
> 
> Thanks,
> 
> Jonathan
> 
>

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