Gdal Plugin
Needs ReviewPublic

Authored by chaz6 on Jun 16 2016, 2:43 PM.

Details

Reviewers
None
Group Reviewers
Marble
Summary

Add a new runner plugin called gdal which depends on the GDAL library from www.gdal.org. This allows Marble to read additional vector formats such as OGC GeoPackage. GDAL also supports database connections which I intend to add in a future release of the plugin.

Test Plan

No plans at present but I would like to create some tests when the plugin is ready for final submission.

Diff Detail

Repository
R34 Marble
Lint
Lint Skipped
Unit
Unit Tests Skipped
chaz6 retitled this revision from to Gdal Plugin.Jun 16 2016, 2:43 PM
chaz6 updated this object.
chaz6 edited the test plan for this revision. (Show Details)
chaz6 added a reviewer: Marble.
chaz6 set the repository for this revision to R34 Marble.
chaz6 added a project: Marble.
dfaure added a subscriber: dfaure.Jun 16 2016, 2:54 PM
dfaure added inline comments.
src/plugins/runner/CMakeLists.txt
44

I think you want to do the set_package_properties in all cases, not only if found, so that the report at the end of the cmake run inform people about the missing (optional) dependency.

Looks good. I'll compile and test it in action later as well.

src/plugins/runner/CMakeLists.txt
43

I'd move that three lines below. Otherwise people who have not yet gdal on their system do not get the description in the cmake output.

src/plugins/runner/gdal/CMakeLists.txt
14

What does this do, is it needed? If so it should be done only for GCC since e.g. visual studio would not understand the switch.

src/plugins/runner/gdal/GdalPlugin.cpp
63

Please remove (you can pass --debug-info to bin/marble and bin/marble-qt to activate this when you need it)

src/plugins/runner/gdal/GdalRunner.cpp
51

Can you update error here, too?

69

Should this only be done when the geometry is supported below? Otherwise empty placemarks would be added

chaz6 added inline comments.Jun 16 2016, 3:37 PM
src/plugins/runner/CMakeLists.txt
43

OK!

44

OK!

src/plugins/runner/gdal/CMakeLists.txt
14

This prevents warnings about un-handled enum cases in the switch statement. GDAL has dozens of types so this is for convenience for now; I will remove it before the next code review.

src/plugins/runner/gdal/GdalPlugin.cpp
63

Oops, I will remove it.

src/plugins/runner/gdal/GdalRunner.cpp
51

OK!

69

Understood, I will move it.

chaz6 updated this revision to Diff 4555.Jun 16 2016, 3:50 PM

Fixed previous issues.

chaz6 updated this revision to Diff 4585.Jun 17 2016, 10:18 AM
chaz6 updated this object.

Corrected error handling that I missed out of the last diff.

chaz6 updated this revision to Diff 4609.Jun 18 2016, 12:11 PM

Add support for main remaining geometry types.

chaz6 updated this revision to Diff 4610.Jun 18 2016, 12:35 PM

Missed an error handling.

chaz6 updated this revision to Diff 4612.Jun 18 2016, 1:39 PM

Fixed polygon loops using wrong iterators.

nienhueser added inline comments.Jun 19 2016, 7:40 PM
src/plugins/runner/gdal/GdalRunner.cpp
75

Use p.GetZ() instead of 0 here? Same for the other points below.

I just tested the code, compiles fine here. To see it in action I downloaded airports.gml from http://examples.oreilly.com/9780596008659/ and opened it. The coordinates it extracts seem to be wrong. Looking into the .gml file I wonder whether some additional logic is needed to handle that?

Another thing that would be really nice is to extract additional information present in files. For this one in particular reading the <NAME> and storing it as the placemark's name would be great. Does gdal support that?

chaz6 updated this revision to Diff 5181.Jul 14 2016, 8:20 PM
  • Remove debug info
  • Remove compiler directive
  • Add support for name field
  • Add support for notes field
  • Handle more geometry types
chaz6 marked 8 inline comments as done.Jul 14 2016, 8:23 PM

To keep momentum here, some more comments on code style/API level :) So that your next cozy autumn evening on the couch has another option for entertaining yourself.
Next to the inline comments, please in general also remove spaces between brackets and arguments consistently (yielding if (b) { or for (int i = 0; i < s; ++i) {

src/plugins/runner/gdal/CMakeLists.txt
7

No need for that, Qt include dirs are automatically added by mechanisms elsewhere (if curious how, ask me for details on irc please :) ). Besides, that var is not set in Qt5 worlds anyway.

src/plugins/runner/gdal/GdalPlugin.cpp
32

Please wrap raw strings that will end up as QString object here and below with QStringLiteral() (but not those passed to tr() of course). We might switch to QT_NO_CAST_FROM_ASCII one day, so let's prepare for that with new code.

src/plugins/runner/gdal/GdalPlugin.h
38

Needs to be QVector<PluginAuthor>now.

src/plugins/runner/gdal/GdalRunner.cpp
22

No QtCore module prefix, please.

45

No NULL please., this is C++ :) 0 should work here.

85

To transform the temporary string behind the const char* pointer returned by OGRFeature::GetFieldAsString* into a QString, use QString::fromLatin1(), or, if there is not only 7bit-latin chars in the string, but actually utf-8 encoded ones, QString::fromUtf8().
E.g.

placemark->setName(QString::fromLatin1(gFeature->GetFieldAsString(nameField)));
246

Tabs sneaking in? :) Please use spaces for indentation.

256

No need for else after return