[Marble-devel] Review Request 114819: Open Geo URIs with Marble

[Dennis Nienhüser]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114819/#review46676
-----------------------------------------------------------



src/apps/marble-qt/qtmain.cpp
<https://git.reviewboard.kde.org/r/114819/#comment33324>

    the error check should be easy to do - just append
    && i+1 < args.size()
    to the if clause. For error reporting I'd go for a simple qWarning(). Bonus points for doing the same for marbledatapath :-)
    
    



src/lib/marble/GeoUriParser.h
<https://git.reviewboard.kde.org/r/114819/#comment33318>

    - Since we also have a setter, there should be a default value.
    - For performance reasons pass a reference.
    
    I.e. const QString &geoUri = QString()
    



src/lib/marble/GeoUriParser.h
<https://git.reviewboard.kde.org/r/114819/#comment33319>

    QString geoUri() const



src/lib/marble/GeoUriParser.cpp
<https://git.reviewboard.kde.org/r/114819/#comment33321>

    This should invalidate any previous parse result as well, i.e. call
    m_coordinates = GeoDataCoordinates()
    
    Ideally also add a test case to assure that behavior
    



src/lib/marble/GeoUriParser.cpp
<https://git.reviewboard.kde.org/r/114819/#comment33320>

    Please remove / change to mDebug
    



tests/GeoUriParserTest.cpp
<https://git.reviewboard.kde.org/r/114819/#comment33322>

    Please also check the coordinate values parsed:
    QTest::addColumn<double>( "lat" );
    QTest::addColumn<double>( "lon" );
    QTest::addColumn<double>( "alt" );
    and provide data that is verified in testGeoUri
    



tests/GeoUriParserTest.cpp
<https://git.reviewboard.kde.org/r/114819/#comment33323>

    Please test with more variance:
    - Coordinates from all four hemispheres
    - with and without the geo: prefix
    - with and without altitude values
    - with and without uncertainty parameter, e.g. geo:37.786971,-122.399677;u=35
    - with and without CRS, e.g. geo:37.786971,-122.399677;crs=Moon-2011;u=35
    
    Even though we don't extract the additional parameters parsing should still succeed for the coordinates.
    


- Dennis Nienhüser


On Jan. 3, 2014, 10:09 a.m., Levente Kurusa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114819/
> -----------------------------------------------------------
> 
> (Updated Jan. 3, 2014, 10:09 a.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This is a GCI task: http://www.google-melange.com/gci/task/view/google/gci2013/5847453085466624
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-kde/CMakeLists.txt 4817a15 
>   src/apps/marble-kde/kdemain.cpp 1b4733d 
>   src/apps/marble-kde/marble.protocol PRE-CREATION 
>   src/apps/marble-qt/qtmain.cpp 2f593bb 
>   src/lib/marble/CMakeLists.txt 15a02b6 
>   src/lib/marble/GeoUriParser.h PRE-CREATION 
>   src/lib/marble/GeoUriParser.cpp PRE-CREATION 
>   tests/CMakeLists.txt 6531fd4 
>   tests/GeoUriParserTest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/114819/diff/
> 
> 
> Testing
> -------
> 
> Created four unit tests, they are mostly testing the regexp, they all pass.
> 
> 
> Thanks,
> 
> Levente Kurusa
> 
>

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