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

[Dennis Nienhüser]

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



/trunk/KDE/kdeedu/marble/src/lib/BookmarkInfoDialog.cpp
<http://reviewboard.kde.org/r/5034/#comment7156>

    I think "isBookmark" as the key would be easier to understand than just "bookmark"



/trunk/KDE/kdeedu/marble/src/lib/BookmarkManager.h
<http://reviewboard.kde.org/r/5034/#comment7152>

    QString bookmarkFile() const;
    
    Doxygen comment would be nice.



/trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt
<http://reviewboard.kde.org/r/5034/#comment7153>

    Is that supported cmake syntax, a comment in a multiline string? I'd rather have the line removed.



/trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt
<http://reviewboard.kde.org/r/5034/#comment7154>

    Remove, see above



/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp
<http://reviewboard.kde.org/r/5034/#comment7155>

    This looks awful. There is toBool(), please use it instead of using toString() and relying on it converting true to "true".



/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp
<http://reviewboard.kde.org/r/5034/#comment7157>

    Use curly braces even when the body of a conditional statement contains only one line.
    
    http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Braces
    



/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h
<http://reviewboard.kde.org/r/5034/#comment7158>

    QString bookmarkFile() const;
    
    Doxygen comment, please.



/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.h
<http://reviewboard.kde.org/r/5034/#comment7159>

    int size() const



/trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlDataTagWriter.cpp
<http://reviewboard.kde.org/r/5034/#comment7160>

    Whitespace in inner parenthesis missing. Same for the static_cast below.



/trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlExtendedDataTagWriter.cpp
<http://reviewboard.kde.org/r/5034/#comment7162>

    Whitespace in inner parenthesis missing. Same for the static_cast below.



/trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlPlacemarkTagWriter.cpp
<http://reviewboard.kde.org/r/5034/#comment7161>

    Preferably 
    !placemark.extendedData().isEmpty()
    which is easier to read and faster


- Dennis


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/13411a2e/attachment-0001.htm