[Marble-devel] Re: Review Request: Map Creation Wizard

Tue Dec 21 18:19:28 CET 2010

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

Looks good in general, but the creation and deletion of the archive is more suited for the command line. When called from a program, more care needs to be taken to assure that no wrong data is included, no wrong data is deleted and no external data gets overwritten.

trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10353>

    Preferably before the QTimer call above.

trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10354>

    "this wizard" sounds a bit strange here since no wizard is visible at the time the user will read the message. What about 

    "A wizard guides you through the creation of your own map theme."

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10357>

    Missing tr() calls

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10358>

    Missing tr() calls

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10359>

    Please use curly brackets even for one liners. 
    if ( condition ) {
        ..
    } else {
        ...
    }

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10364>

    This is dangerous, better use a temporary file/dir with random characters. Qt has a QTemporaryFile (or similar) for this.

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10360>

    That looks wrong. Are you relying on QDir::tempPath() returning "/tmp"?

    Furthermore it should be enough to switch the current working directory for the started process, not the Marble process itself. That also prevents you from resetting it later (where you shouldn't hardcode it to the home dir as well!)

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10363>

    I'd rather avoid the whole copy operation. It should be doable with tar in one go like this:

    tar --create --gzip --file /tmp/archive.tar.gz --directory ~/marble/export/share/marble/data maps/earth/openstreetmap/openstreetmap.dgml maps/earth/openstreetmap/openstreetmap-preview.png maps/earth/openstreetmap/legend.html maps/earth/openstreetmap/legend maps/earth/openstreetmap/0/0/0.png

    with appropriate values. That'd also avoid the inclusion of the downloaded tiles.

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10361>

    Please add tr() calls to GUI elements

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10362>

    Please add tr() calls to GUI elements

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10365>

    Never call "rm -r" from any program or script.

    Shouldn't be needed when just using tar, see above.

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10366>

    tr() calls for all gui elements, also in the message boxes below.

trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10367>

    Is this a critical error? I don't think so, it's more an information. Preferably rephrase it as a request, e.g. "Sorry, some required information is missing. Please fill in all fields.".

trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10355>

    See above

trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://svn.reviewboard.kde.org/r/6070/#comment10356>

    Is it possible to open two upload dialogs at once? Then better save the archive file to a local variable above and only delete that one when the dialog is closed.

- Dennis

On 2010-12-21 16:13:25, Utku Aydin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6070/
> -----------------------------------------------------------
> 
> (Updated 2010-12-21 16:13:25)
> 
> 
> Review request for marble, Torsten Rahn and Dennis Nienhüser.
> 
> 
> Summary
> -------
> 
> Map wizard helps users create a new map easily.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdeedu/marble/src/ControlView.h 1207748 
>   trunk/KDE/kdeedu/marble/src/ControlView.cpp 1207748 
>   trunk/KDE/kdeedu/marble/src/QtMainWindow.h 1207748 
>   trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/MapViewWidget.h 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/MapViewWidget.cpp 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/MapViewWidget.ui 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/MapWizard.h PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/lib/MapWizard.ui PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.h 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.cpp 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/MarbleThemeSelectView.h 1207748 
>   trunk/KDE/kdeedu/marble/src/lib/MarbleThemeSelectView.cpp 1207748 
>   trunk/KDE/kdeedu/marble/src/marble.kcfg 1207748 
>   trunk/KDE/kdeedu/marble/src/marble_part.h 1207748 
>   trunk/KDE/kdeedu/marble/src/marble_part.cpp 1207748 
> 
> Diff: http://svn.reviewboard.kde.org/r/6070/diff
> 
> 
> Testing
> -------
> 
> Tested WMS and online map cases with different servers and source bitmap case with different source images.
> 
> * Validates the data properly.
> * Creates the legend.
> * Downloads and shows tiles from both WMS and a static HTTP URL properly.
> 
> Tested servers:
> * http://onearth.jpl.nasa.gov/wms.cgi (WMS)
> * http://tile.openstreetmap.org (online map)
> * http://andy.sandbox.cloudmade.com/tiles/cycle/ (online map)
> * http://mt.google.com/vt/lrys=m@130&hl=tr&x={x}&y={y}&z={zoomLevel}&s=Ga
> 
> Note:
> I didn't test upload feature because the server is not available at the moment.
> 
> 
> Screenshots
> -----------
> 
> Step One
>   http://svn.reviewboard.kde.org/r/6070/s/577/
> Step Two
>   http://svn.reviewboard.kde.org/r/6070/s/578/
> 
> 
> Thanks,
> 
> Utku
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20101221/40603ad9/attachment-0001.htm 