[Kde-pim] Review Request: Migrate color settings in KMail to KConfigXT

[Thomas McGuire]

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


I don't like the current way in configuredialog.cpp, it is now more code than before...
Maybe create a list of KConfigSkeletonItem's once, then you can just iterate over the list and need to fill it only once at the beginning.
This would remove some of the tedious code here.

I'm sorry for this late review request. I'd prefer if you make this change in the akonadi-ports branch instead (you have it compiled now anyway).


/trunk/KDE/kdepim/kmail/configuredialog.cpp
<http://reviewboard.kde.org/r/2271/#comment2792>

    I think you can get a translated label right out of the kcfg, with KConfigSkeletonItem::label().
    
    So the i18n description can move to the kcfg as well.



/trunk/KDE/kdepim/kmail/configuredialog.cpp
<http://reviewboard.kde.org/r/2271/#comment2794>

    I think this can all be removed, if you use KConfigSkeletonItem::default() when reading in the color.



/trunk/KDE/kdepim/kmail/csshelper.cpp
<http://reviewboard.kde.org/r/2271/#comment2796>

    Same problem here as somewhere else here: if useDefaultColors() is true here, the colors are not initalized.



/trunk/KDE/kdepim/kmail/kmail.kcfg.cmake
<http://reviewboard.kde.org/r/2271/#comment2795>

    I don't think that would work.
    How about just
    <default code>KColorScheme( QPalette::Active, KColorScheme::View ).foreground( KColorScheme::LinkText ).color()</default>



/trunk/KDE/kdepim/kmail/messagelistview/storagemodel.cpp
<http://reviewboard.kde.org/r/2271/#comment2793>

    When using default colors, mColorNewMessage and friends are undefined now, aren't they?
    
    You probably should use KConfigSkeletonItem::default() in this case. However, that function does not yet exist.


- Thomas


On 2009-11-23 18:07:17, Torgny Nyblom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2271/
> -----------------------------------------------------------
> 
> (Updated 2009-11-23 18:07:17)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Migrate the color settings in KMail to using KConfigXT. Patch should probably be ported to the messageviewer as well.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/configuredialog.cpp 1053083 
>   /trunk/KDE/kdepim/kmail/csshelper.cpp 1053083 
>   /trunk/KDE/kdepim/kmail/folderview.cpp 1053083 
>   /trunk/KDE/kdepim/kmail/kmail.kcfg.cmake 1053083 
>   /trunk/KDE/kdepim/kmail/messagelistview/storagemodel.cpp 1053083 
> 
> Diff: http://reviewboard.kde.org/r/2271/diff
> 
> 
> Testing
> -------
> 
> Changed all colors using a pre patch version of KMail. Using this kmailrc file with a patched version of KMail shows all color changed in the settings dialog.
> 
> 
> 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/