[Marble-devel] Re: Review Request: Configuration plugin for the coordinate lines.

[Torsten Rahn]

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


Looks good. You still need to fix the issues mentioned below :-)


/trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.h
<http://svn.reviewboard.kde.org/r/6281/#comment10540>

    No need to have the color dialog as a member.



/trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp
<http://svn.reviewboard.kde.org/r/6281/#comment10541>

    no need to have the color dialogs created as members.



/trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp
<http://svn.reviewboard.kde.org/r/6281/#comment10535>

    No :-)
    
    You don't need the QColorDialog as a member. This doesn't make sense. Also don't assign the color that gets returned as a member before you know it's valid. 
    
    Just use:
    
    QColor c;
    c = QColorDialog::getColor( m_gridColor, 0, "Please choose the color for the coordinate grid" );
    
    if ( c.isValid() ) {
      [...]
    }



/trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp
<http://svn.reviewboard.kde.org/r/6281/#comment10536>

    Like this you create an "empty" default palette and change a single color of that one. 
    
    But that's not what you want.
    You want to pick the palette that is already used by the button and change a single color:
    
    QPalette gridPalette = ui_configWidget->gridPushButton->palette();



/trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp
<http://svn.reviewboard.kde.org/r/6281/#comment10537>

    Please adjust the same way as for the gridGetColor method!



/trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp
<http://svn.reviewboard.kde.org/r/6281/#comment10538>

    Please adjust the same way as for the gridGetColor method!



/trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp
<http://svn.reviewboard.kde.org/r/6281/#comment10539>

    Since the changes of the about dialog are already in trunk it doesn't make sense to commit them again :-)


- Torsten


On 2011-01-05 17:13:59, Cezar Mocan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6281/
> -----------------------------------------------------------
> 
> (Updated 2011-01-05 17:13:59)
> 
> 
> Review request for KDE-Edu, marble and Torsten Rahn.
> 
> 
> Summary
> -------
> 
> Configuration plugin for the coordinate lines. 
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/graticule/CMakeLists.txt 1201473 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticuleConfigWidget.ui PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.h 1201473 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp 1201473 
> 
> Diff: http://svn.reviewboard.kde.org/r/6281/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cezar
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20110105/1ac7b004/attachment.htm