[Marble-devel] Review Request: Bookmark Feature : Icons, Coordinate in bookmarks.kml and Edit Bookmark

[1989.gaurav at gmail.com]

> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp, line 159
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33978#file33978line159>
> >
> >     I think "isBookmark" as the key would be easier to understand than just "bookmark"

Changed


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt, line 198
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33981#file33981line198>
> >
> >     Is that supported cmake syntax, a comment in a multiline string? I'd rather have the line removed.

removed


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt, line 225
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33981#file33981line225>
> >
> >     Remove, see above

removed


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp, line 177
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33982#file33982line177>
> >
> >     Use curly braces even when the body of a conditional statement contains only one line.
> >     
> >     http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Braces
> >

done


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h, line 580
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33983#file33983line580>
> >
> >     QString bookmarkFile() const;
> >     
> >     Doxygen comment, please.

done


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.h, line 61
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33985#file33985line61>
> >
> >     int size() const

done


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlDataTagWriter.cpp, line 22
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33988#file33988line22>
> >
> >     Whitespace in inner parenthesis missing. Same for the static_cast below.

done


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlExtendedDataTagWriter.cpp, line 24
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33990#file33990line24>
> >
> >     Whitespace in inner parenthesis missing. Same for the static_cast below.

done


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlPlacemarkTagWriter.cpp, line 53
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33991#file33991line53>
> >
> >     Preferably 
> >     !placemark.extendedData().isEmpty()
> >     which is easier to read and faster

done


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp, line 176
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33982#file33982line176>
> >
> >     This looks awful. There is toBool(), please use it instead of using toString() and relying on it converting true to "true".

done, but idis don't want to push it untill its required.
So just commenting for now and will push it in my next patch which is Edit Bookmark feature


> On 2010-08-15 14:54:50, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/BookmarkManager.h, line 39
> > <http://reviewboard.kde.org/r/5034/diff/3/?file=33979#file33979line39>
> >
> >     QString bookmarkFile() const;
> >     
> >     Doxygen comment would be nice.

done


- 1989gaurav


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


On 2010-08-15 13:59:02, 1989gaurav wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/5034/
> -----------------------------------------------------------
> 
> (Updated 2010-08-15 13:59:02)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> Added Icons in Bookmark menu.
> Added GeoDataCoordinate in bookmark.kml file along with lookAt.
> Added ExtendedData Tag writer to write in bookmark.kml file.
> Adding Edit Bookmark Feature along with some modification in GeoDataTreeModel ( In Process )
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.h 1163977 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/BookmarkManager.h 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/BookmarkManager.cpp 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.h 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.cpp 1163977 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlDataTagWriter.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlDataTagWriter.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlExtendedDataTagWriter.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlExtendedDataTagWriter.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlPlacemarkTagWriter.cpp 1163977 
>   /trunk/KDE/kdeedu/marble/src/marble.qrc 1163977 
> 
> Diff: http://reviewboard.kde.org/r/5034/diff
> 
> 
> Testing
> -------
> 
> Running
> 
> 
> Thanks,
> 
> 1989gaurav
> 
>

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