[Marble-devel] Review Request: Bookmark Feature Progress

Wed Jun 9 19:58:35 CEST 2010

> On 2010-06-09 17:26:22, Torsten Rahn wrote:
> > 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?

Information is not getting lost, just run with --debug-info it will print the GeoDataLookAt values

> On 2010-06-09 17:26:22, Torsten Rahn wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp, line 45
> > <http://reviewboard.kde.org/r/4269/diff/1/?file=28268#file28268line45>
> >
> >     This should become:
> >     
> >     bookmark.setLookAt( m_widget->lookAt() );
> >     
> >     in the future.
> >

Will do it Once I add GeoDataLookAt member in GeoDataPlacemark

> On 2010-06-09 17:26:22, Torsten Rahn wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h, line 560
> > <http://reviewboard.kde.org/r/4269/diff/1/?file=28273#file28273line560>
> >
> >     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.

GeoDataDocument is already stored in BookMarkManager as member. It would be better if we return a vector of folders only, folder will automatically contain all bookmaks. 

> On 2010-06-09 17:26:22, Torsten Rahn wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.h, line 93
> > <http://reviewboard.kde.org/r/4269/diff/1/?file=28275#file28275line93>
> >
> >     you forgot a "const":
> >     
> >     void setCoordinates( const GeoDataCoordinates & );
> >     
> >

added

> On 2010-06-09 17:26:22, Torsten Rahn wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.cpp, line 84
> > <http://reviewboard.kde.org/r/4269/diff/1/?file=28276#file28276line84>
> >
> >     const!
> >     
> >     void GeoDataLookAt::setCoordinates( const GeoDataCoordinates& coordinates )

added

> On 2010-06-09 17:26:22, Torsten Rahn wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp, line 272
> > <http://reviewboard.kde.org/r/4269/diff/1/?file=28278#file28278line272>
> >
> >     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!

done

> On 2010-06-09 17:26:22, Torsten Rahn wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp, line 274
> > <http://reviewboard.kde.org/r/4269/diff/1/?file=28278#file28278line274>
> >
> >     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.

if I used coordinate, it will give an error because compiler will treat it as function pointer ;)
Will change this name :)

- 1989gaurav

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

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
> -------
> 
> 
> Screenshots
> -----------
> 
> bookmark feature
>   http://reviewboard.kde.org/r/4269/s/429/
> 
> 
> Thanks,
> 
> 1989gaurav
> 
>