[kde-edu]: Re: Review Request: Marble: config dialog for map scale plugin + scale ratio implemented

[Torsten Rahn]

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


Hi Nhan,

Almost correct - but you misunderstood the aim of the task a bit: We don't need another means of showing and hiding the whole scalebar. What this task is about is just showing and hiding the scale ratio below the scale ratio. However this should be quite easy to fix for you.
Your patch adds the functionality of displaying the scale ratio nicely! However there are some things that you need to adjust in order to mark the task as complete:

- There needs to be the margin honored below the scale ratio. Please ensure that the distance is kept between the scale ratio and the border of the floatitem.
- The scale ratio should be displayed centered (and not left aligned). 
- The scale ration should be displayed in user readable form (so no "1:1.233456e6", but instead something like "1:1230000")

- You need to remove the whole "visible" stuff that you introduced. It's not needed for the plugin since this stuff already gets handled elsewhere.

- Instead you should adjust your patch so that the user can toggle the visibility of the scale ratio text (the text string e.g. "1:123000") that you introduced. When toggling the visibility of the scale ratio the visibility of the rest of the scale bar should stay unaffected.

Maybe instead of labeling the configuration item "Show scale" it should get labeled "Show scale ratio". This might make things more clear ;-)

Could you make those improvements and resubmit the patch?
Also make sure you don't actually commit the patch to svn trunk since we are on feature freeze :-) We just want to have the patch on review board so that it doesn't get lost :-) 

 


/trunk/KDE/kdeedu/marble/src/lib/global.h
<http://svn.reviewboard.kde.org/r/6153/#comment10184>

    The comment is not quite correct



/trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.h
<http://svn.reviewboard.kde.org/r/6153/#comment10185>

    This is not required. It's being handled elsewhere already. Please remove it.



/trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.cpp
<http://svn.reviewboard.kde.org/r/6153/#comment10186>

    This is not required. It's being handled elsewhere already. Please remove it.



/trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.cpp
<http://svn.reviewboard.kde.org/r/6153/#comment10187>

    This is not required. It's being handled elsewhere already. Please remove it.


- Torsten


On 2010-12-18 13:14:47, Khanh-Nhan Nguyen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6153/
> -----------------------------------------------------------
> 
> (Updated 2010-12-18 13:14:47)
> 
> 
> Review request for KDE-Edu.
> 
> 
> Summary
> -------
> 
> - Config dialog for map scale plugin (with a single option visible/invisible).
> - Scale ratio implemented and displayed below the scale bar (with an acceptable error).
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/global.h 1207419 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/CMakeLists.txt 1207419 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleConfigWidget.ui PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.h 1207419 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.cpp 1207419 
> 
> Diff: http://svn.reviewboard.kde.org/r/6153/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Config dialog
>   http://svn.reviewboard.kde.org/r/6153/s/584/
> After uncheck the box in the config dialog - scale bar becomes invisible
>   http://svn.reviewboard.kde.org/r/6153/s/585/
> 
> 
> Thanks,
> 
> Khanh-Nhan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-edu/attachments/20101218/e4c8c9d1/attachment-0001.htm