[Marble-devel] Re: Review Request: Maemo: Tracking dialog with option to show/hide/clear/save the track record

[Dennis Nienhüser]

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

Ship it!


Works fine (tested directly on the N900), just some nitpicking.



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

    Preferably also initializing m_showTrackingDialogAction( 0 )



/trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.cpp
<http://svn.reviewboard.kde.org/r/6079/#comment10074>

    Please include with qt module names, i.e
    #include <QtCore/QDateTime>
    #include <QtGui/QFileDialog>



/trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.cpp
<http://svn.reviewboard.kde.org/r/6079/#comment10075>

    Nice catch :)



/trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.cpp
<http://svn.reviewboard.kde.org/r/6079/#comment10077>

    A couple of additional whitespaces would be nice.



/trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.ui
<http://svn.reviewboard.kde.org/r/6079/#comment10073>

    Seems to clash with another spacer, please rename



/trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.ui
<http://svn.reviewboard.kde.org/r/6079/#comment10076>

    Given that we introduce string changes anyway I think it's a good chance to change this label's title: "Re-center" is not very descriptive and does not cover auto zooming. What about "Map Adjustment" or "Auto View"?



/trunk/KDE/kdeedu/marble/src/lib/PositionTracking.h
<http://svn.reviewboard.kde.org/r/6079/#comment10078>

    The doxygen comment should mention that when saving succeeds, the fileName parameter contains the final name of the file (i.e. original fileName or fileName appended by .kml)
    



/trunk/KDE/kdeedu/marble/src/lib/PositionTracking.cpp
<http://svn.reviewboard.kde.org/r/6079/#comment10079>

    #include <QtCore/QFile>



/trunk/KDE/kdeedu/marble/src/lib/PositionTracking.cpp
<http://svn.reviewboard.kde.org/r/6079/#comment10080>

    ".kml"?


- Dennis


On 2010-12-11 17:49:18, Thibaut Gridel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6079/
> -----------------------------------------------------------
> 
> (Updated 2010-12-11 17:49:18)
> 
> 
> Review request for marble and Dennis Nienhüser.
> 
> 
> Summary
> -------
> 
> A small dialog around the Current Position widget, for Maemo.
> UI to show/hide/clear/save the track record.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.ui 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PositionTracking.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PositionTracking.cpp 1202435 
> 
> Diff: http://svn.reviewboard.kde.org/r/6079/diff
> 
> 
> Testing
> -------
> 
> works on desktop widget, and also on Maemo.
> 
> 
> Thanks,
> 
> Thibaut
> 
>

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