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.
Details
Details
- Reviewers
- None
- Group Reviewers
Marble
No plans at present but I would like to create some tests when the plugin is ready for final submission.
Diff Detail
Diff Detail
- Repository
- R34 Marble
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| 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. | |
Comment Actions
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 | ||
| 15 | 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 | ||
| 64 | 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 | ||
| 52 | Can you update error here, too? | |
| 70 | Should this only be done when the geometry is supported below? Otherwise empty placemarks would be added | |
| src/plugins/runner/CMakeLists.txt | ||
|---|---|---|
| 43 | OK! | |
| 44 | OK! | |
| src/plugins/runner/gdal/CMakeLists.txt | ||
| 15 | 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 | ||
| 64 | Oops, I will remove it. | |
| src/plugins/runner/gdal/GdalRunner.cpp | ||
| 52 | OK! | |
| 70 | Understood, I will move it. | |
| src/plugins/runner/gdal/GdalRunner.cpp | ||
|---|---|---|
| 75 | Use p.GetZ() instead of 0 here? Same for the other points below. | |
Comment Actions
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?
Comment Actions
- Remove debug info
- Remove compiler directive
- Add support for name field
- Add support for notes field
- Handle more geometry types
Comment Actions
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 | ||
|---|---|---|
| 6 | 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 | ||
| 31 | 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 | ||
| 37 | Needs to be QVector<PluginAuthor>now. | |
| src/plugins/runner/gdal/GdalRunner.cpp | ||
| 21 | No QtCore module prefix, please. | |
| 44 | No NULL please., this is C++ :) 0 should work here. | |
| 84 | 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(). placemark->setName(QString::fromLatin1(gFeature->GetFieldAsString(nameField))); | |
| 245 | Tabs sneaking in? :) Please use spaces for indentation. | |
| 255 | No need for else after return | |