[Marble-devel] Review Request: Bookmark feature

[Torsten Rahn]

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

Ship it!


Looks very good! Please fix the results of the review and then continue with your implementation of the other tasks :-)


svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp
<http://reviewboard.kde.org/r/4225/#comment5609>

    There is a bug in the API of GeoDataPlacemark. Could you change the API so that it's using
    
        void setCoordinate( const GeoDataCoordinate &coordinate );
    
    instead of the current GeoDataPoint parameter?
    
    then we should use:
    
    bookmark.setCoordinate( m_widget->lookAt()->coordinates() );
    
    Argh. And I just realize that we still have some API inconsistency in Marble:
    
    We seem to use setCoordinate as well as setCoordinates. I guess it should always be setCoordinates. Could you make a fix for the API for GeoDataPlacemark? Maybe leave the current API there as deprecated but internally calling the new API and printing a warning on the command line :-)
    
    
    
    
    
    
    



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp
<http://reviewboard.kde.org/r/4225/#comment5610>

    spelling error: updat E BookmarkFile



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp
<http://reviewboard.kde.org/r/4225/#comment5612>

    typo: updateBookmarkFile()


- Torsten


On 2010-06-04 16:40:25, 1989gaurav wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4225/
> -----------------------------------------------------------
> 
> (Updated 2010-06-04 16:40:25)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> A Step towards Bookmark feature implementation.
> Added addBookmark button which shows longitude, latitude and zoom level on console.
> 
> 
> Diffs
> -----
> 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/QtMainWindow.h 1133781 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1133781 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.h PRE-CREATION 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp PRE-CREATION 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1133781 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/PlacemarkInfoDialog.h 1133781 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/plugins/render/CMakeLists.txt 1133781 
> 
> Diff: http://reviewboard.kde.org/r/4225/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> 1989gaurav
> 
>