[Marble-devel] Review Request: KML ListStyle Tag Handler

[Dennis Nienhüser]

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



src/lib/geodata/data/GeoDataListStyle.h
<http://git.reviewboard.kde.org/r/107934/#comment18376>

    Please move the GeoDataItemIcon class to new files GeoDataItemIcon.h and GeoDataItemIcon.cpp



src/lib/geodata/data/GeoDataListStyle.h
<http://git.reviewboard.kde.org/r/107934/#comment18378>

    The kml reference does not list color style as a parent of list style. Shouldn't it inherit from GeoDataObject instead?
    



src/lib/geodata/data/GeoDataListStyle.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18377>

    Is this a sensible default?



src/lib/geodata/data/GeoDataListStyle.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18379>

    Misses the call to operator= of its parent, sth. like
    GeoDataFeature::operator=( other );



src/lib/geodata/data/GeoDataListStyle.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18380>

    Please add curly brackets



src/lib/geodata/data/GeoDataListStyle.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18381>

    Please add curly brackets, or move out of the else



src/lib/geodata/data/GeoDataListStyle.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18382>

    Simply do a
    return d->m_vector;



src/lib/geodata/data/GeoDataListStyle.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18383>

    Simply do a
    return d->m_vector.indexOf( object );



src/lib/geodata/data/GeoDataListStyle.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18384>

    other->setParent( this );



src/lib/geodata/handlers/kml/KmlListItemTypeTagHandler.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18385>

    Please add an else {} part where you emit a warning (mDebug) that the typeText value found is invalid and a default value of check (?) is used instead.
    



src/lib/geodata/handlers/kml/KmlStateTagHandler.cpp
<http://git.reviewboard.kde.org/r/107934/#comment18386>

    The parsing follows the google earth example from the kml reference quite closely. An issue I see is that a value of "fetching0 open" would not be parsed here although the kml spec does not really disallow it.
    
    I wonder if we shouldn't relax the check here and implement any restrictions which are needed at a later point when using the values. The code would simplify to sth. like this then:
    
    foreach( const QString &value, iconStateTextList ) {
      if ( value == "open" ) {
        itemIconState |= GeoDataItemIcon::Open; 
      } else if ( value == "closed" ) {
         ...
      } else if ( value == "error" ) {
         ...
      } .... fetching0, fetching1, fetching2 {
      else {
        mDebug() << "Cannot parse state value" << value;
      }
      


- Dennis Nienhüser


On Dec. 27, 2012, 8:53 a.m., Mohammed Nafees wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107934/
> -----------------------------------------------------------
> 
> (Updated Dec. 27, 2012, 8:53 a.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Description
> -------
> 
> http://www.google-melange.com/gci/task/view/google/gci2012/8040207
> 
> 
> Diffs
> -----
> 
>   src/lib/geodata/data/GeoDataListStyle.h e69de29 
>   src/lib/geodata/data/GeoDataListStyle.cpp e69de29 
>   src/lib/geodata/data/GeoDataStyle.h e3abbd5 
>   src/lib/geodata/data/GeoDataStyle.cpp 01c4a24 
>   src/lib/geodata/handlers/kml/KmlBgColorTagHandler.cpp 0e0b5f7 
>   src/lib/geodata/handlers/kml/KmlHrefTagHandler.cpp bf8f0a4 
>   src/lib/geodata/handlers/kml/KmlItemIconTagHandler.h e69de29 
>   src/lib/geodata/handlers/kml/KmlItemIconTagHandler.cpp e69de29 
>   src/lib/geodata/handlers/kml/KmlListItemTypeTagHandler.h e69de29 
>   src/lib/geodata/handlers/kml/KmlListItemTypeTagHandler.cpp e69de29 
>   src/lib/geodata/handlers/kml/KmlListStyleTagHandler.h e69de29 
>   src/lib/geodata/handlers/kml/KmlListStyleTagHandler.cpp e69de29 
>   src/lib/geodata/handlers/kml/KmlStateTagHandler.h e69de29 
>   src/lib/geodata/handlers/kml/KmlStateTagHandler.cpp e69de29 
>   src/lib/geodata/parser/GeoDataTypes.h 8ae6a2d 
>   src/lib/geodata/parser/GeoDataTypes.cpp 41aa57b 
>   tests/CMakeLists.txt 0aa5f9e 
>   tests/TestBalloonStyle.cpp dd69f2c 
>   tests/TestListStyle.cpp e69de29 
> 
> Diff: http://git.reviewboard.kde.org/r/107934/diff/
> 
> 
> Testing
> -------
> 
> yes, testing done
> 
> 
> Thanks,
> 
> Mohammed Nafees
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20121228/9451656c/attachment-0001.html>