[Marble-devel] Review Request 114854: Support worldwind URIs in Marble

[Dennis Nienhüser]

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



src/apps/marble-kde/kdemain.cpp
<https://git.reviewboard.kde.org/r/114854/#comment33399>

    this will always be false. Please compare like
    uriParser.planet() != window->marbleControl()->marbleModel()->planet()



src/apps/marble-kde/kdemain.cpp
<https://git.reviewboard.kde.org/r/114854/#comment33396>

    no abbreviations please, so better manager than mtm.



src/apps/marble-kde/kdemain.cpp
<https://git.reviewboard.kde.org/r/114854/#comment33397>

    QString => const QString &



src/apps/marble-kde/kdemain.cpp
<https://git.reviewboard.kde.org/r/114854/#comment33398>

    I'd use Qt::CaseInsensitive as second parameter for startsWith for a more relaxed check



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

    Same comments as above. Why not move most of the block to ControlView and just call
    window->marbleControl()->openGeoUri( geoUriString ); here?



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

    shouldn't be possible to set that from the outside, so I'd remove this method.



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

    - Please make it a private method of the class, not a c-function. I.e. add GeoUriParser:: prefix.
    - url is not changed, so const QUrl &
    - both QString should be passed as const references
    - I'd rename them key and alternativeKey
    



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

    QString str => const QString &planet



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

    Is i+1 safe here?



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

    We're not using these values yet, so I'd comment these lines. That's better than removing since we'll have a use for them as soon as we report not only GeoDataCoordinates, but a GeoDataCamera / GeoDataLookat



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

    QString str => const QString &planet


- Dennis Nienhüser


On Jan. 4, 2014, 2:57 p.m., Levente Kurusa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114854/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2014, 2:57 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This is a GCI task: https://www.google-melange.com/gci/task/view/google/gci2013/5784750857912320
> 
> It introduces a few new warnings, I didn't know what to do with them. :-)
> 
> /usr/src/marble/marble-3/src/lib/marble/GeoUriParser.cpp: In member function ‘bool Marble::GeoUriParser::parse()’:
> /usr/src/marble/marble-3/src/lib/marble/GeoUriParser.cpp:110:16: warning: unused variable ‘bank’ [-Wunused-variable]
>          double bank = getDoubleFromParameter(worldwindUrl, "bank", "");
>                 ^
> /usr/src/marble/marble-3/src/lib/marble/GeoUriParser.cpp:111:16: warning: unused variable ‘dir’ [-Wunused-variable]
>          double dir = getDoubleFromParameter(worldwindUrl, "dir", "direction");
>                 ^
> /usr/src/marble/src/lib/marble/GeoUriParser.cpp:112:16: warning: unused variable ‘tilt’ [-Wunused-variable]
>          double tilt = getDoubleFromParameter(worldwindUrl, "tilt", "");
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-kde/CMakeLists.txt a39bf51 
>   src/apps/marble-kde/kdemain.cpp 84c43d3 
>   src/apps/marble-kde/marble_worldwind.desktop PRE-CREATION 
>   src/apps/marble-qt/qtmain.cpp 26e1a03 
>   src/lib/marble/GeoUriParser.h fc0a6ba 
>   src/lib/marble/GeoUriParser.cpp de56e90 
>   tests/GeoUriParserTest.cpp 44bd7de 
> 
> Diff: https://git.reviewboard.kde.org/r/114854/diff/
> 
> 
> Testing
> -------
> 
> Attached unit tests pass.
> 
> 
> File Attachments
> ----------------
> 
> Konq opening worldwind:// uri
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/01/04/16b93c0e-ca78-4ec1-84e1-cdcb06af7394__geouriworking2.png
> 
> 
> Thanks,
> 
> Levente Kurusa
> 
>

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