[Marble-devel] Review Request: Bookmark Feature Progress

[Torsten Rahn]

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


Looks great to me :-)
I don't see exactly why this shouldn't work. Does the information about the coordinates get lost before invoking flyTo? If it gets lost then at which point does it get lost?


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

    This should become:
    
    bookmark.setLookAt( m_widget->lookAt() );
    
    in the future.
    



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h
<http://reviewboard.kde.org/r/4269/#comment5642>

    This is ok for now. But in the future this should return a GeoDataDocument containing the folders and the bookmarks stored inside the bookmark structure. Doesn't matter much right now but we should keep this in mind. 



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.h
<http://reviewboard.kde.org/r/4269/#comment5643>

    you forgot a "const":
    
    void setCoordinates( const GeoDataCoordinates & );
    
    



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.cpp
<http://reviewboard.kde.org/r/4269/#comment5646>

    const!
    
    void GeoDataLookAt::setCoordinates( const GeoDataCoordinates& coordinates )



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp
<http://reviewboard.kde.org/r/4269/#comment5645>

    This should return a GeoDataLookAt object by value. 
    
    So please make the API like this:
    
    GeoDataLookAt GeoDataPlacemark::lookAt() const
    
    All this inspite of the idea to internally reference this stuff as a pointer!



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp
<http://reviewboard.kde.org/r/4269/#comment5644>

    Ouch. You are creating a _local_ variable here that is named like a member. Only members should be prefixed "m_". Additionally I think that you don't need to create the local here. So this line can be removed.


- Torsten


On 2010-06-09 16:30:13, 1989gaurav wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4269/
> -----------------------------------------------------------
> 
> (Updated 2010-06-09 16:30:13)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> I have implemented most of the bookmarking and viewing. But It isn't working as expected. On startup it loads bookmark file MARBLE_DATA_PATH/bookmarks/bookmarks.kml and store it in a GeoDataDocument. On adding new bookmark, new node is appended in GeoDataDocument and bookmarks file is updated. On view GeoDataLookAt object is extracted and bookmark is shown using flyTo(GeoDataLookAt& ) method. 
> 
> File is getting modified properly, but flyTo is not working. run marble with --debug-info to print bookmark values added or read. Suggest me what could be the possible modifications.
> 
> 
> Diffs
> -----
> 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/data/CMakeLists.txt 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/data/bookmarks/bookmarks.kml PRE-CREATION 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/QtMainWindow.h 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1136009 
>   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/BookmarkInfoDialog.ui PRE-CREATION 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.h 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.cpp 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h 1136009 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp 1136009 
> 
> Diff: http://reviewboard.kde.org/r/4269/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> 1989gaurav
> 
>