KMB import fix #6006

strump · 2023-09-12T08:28:13Z

strump commented

2023-09-12 08:28:13 +00:00

The main problem with updated KMB format that version ID is not changed. KMB file is generated with version V8 while internal changes are incompatible with V8. Introduced new version V8MM for new KMB files format.

KBM v8MM files don't have compilations. I removed compilations from the code. Tracks data has extra 3 bytes with unknown purpose. Also timestamp is now stored as milliseconds instead of seconds.

Updated unit tests.

Changes in `kml_to_kmb` tool

kml_to_kmb now accepts optional version parameter. It can generate not only files in V9 (latest) format but with V8.
This was needed to generate test data in V8 format.

Fixes #5718

The main problem with updated KMB format that version ID is not changed. KMB file is generated with version `V8` while internal changes are incompatible with `V8`. Introduced new version `V8MM` for new KMB files format. KBM v8MM files don't have compilations. I removed compilations from the code. Tracks data has extra 3 bytes with unknown purpose. Also timestamp is now stored as milliseconds instead of seconds. Updated unit tests. ### Changes in `kml_to_kmb` tool `kml_to_kmb` now accepts optional version parameter. It can generate not only files in V9 (latest) format but with V8. This was needed to generate test data in V8 format. Fixes #5718

biodranik (Migrated from github.com) reviewed 2023-09-12 11:59:13 +00:00

biodranik (Migrated from github.com) left a comment

Is it possible to create a subtype for version81 and incorporate all fixes there?

vng (Migrated from github.com) reviewed 2023-09-13 11:03:51 +00:00

kml/types.hpp

vng (Migrated from github.com) commented

2023-09-13 11:03:36 +00:00

So the HasCompilationsSection() should be enough, no?
Why comment here? Let's keep as-is.

So the HasCompilationsSection() should be enough, no? Why comment here? Let's keep as-is.

kml/types_v8.hpp

vng (Migrated from github.com) commented

2023-09-13 11:03:30 +00:00

So the HasCompilationsSection() should be enough, no?
Why comment here? Let's keep as-is.

So the HasCompilationsSection() should be enough, no? Why comment here? Let's keep as-is.

strump reviewed 2023-09-13 11:45:20 +00:00

kml/types_v8.hpp

strump commented

2023-09-13 11:45:20 +00:00

Not only compilations section is disabled but also Bookmark and Category fields list should be updated. If I enable back "compilations" field parsing would fail.

biodranik (Migrated from github.com) reviewed 2023-09-16 11:59:46 +00:00

biodranik (Migrated from github.com) left a comment

LGTM with minor comments.

kml/kml_to_kmb/kml_to_kmb.cpp

biodranik (Migrated from github.com) commented

2023-09-16 11:55:17 +00:00

    std::string const versionStr = argv[2];

```suggestion std::string const versionStr = argv[2]; ```

kml/types_v8.hpp

biodranik (Migrated from github.com) commented

2023-09-16 11:57:01 +00:00

Is it really needed?

  BookmarkDataV8() = default;

Is it really needed? ```suggestion BookmarkDataV8() = default; ```

biodranik (Migrated from github.com) commented

2023-09-16 11:57:16 +00:00

  BookmarkDataV8(BookmarkData const & src)

```suggestion BookmarkDataV8(BookmarkData const & src) ```

strump reviewed 2023-09-20 08:33:00 +00:00

kml/serdes_binary_v8.hpp

					
				@ -0,0 +18,4 @@

				  explicit SerializerKmlV8(FileData & data) : SerializerKml(data) {};

				  template <typename Sink>

				  void Serialize(Sink & sink)

strump commented

2023-09-20 08:32:13 +00:00

This method is cop-n-paste of SerializerKml::Serialize(sink).
The only differences are that it writes version 0x08 as first byte and it calls SerializerKmlV8::SerializeBookmarks() instead of SerializerKml::SerializeBookmarks().
Is there a better way? Should I introduce virtual methods SerializerKml::SerializeVersion() and SerializerKml::SerializeBookmarks()?

This method is cop-n-paste of `SerializerKml::Serialize(sink)`. The only differences are that it writes version 0x08 as first byte and it calls `SerializerKmlV8::SerializeBookmarks()` instead of `SerializerKml::SerializeBookmarks()`. Is there a better way? Should I introduce virtual methods `SerializerKml::SerializeVersion()` and `SerializerKml::SerializeBookmarks()`?

strump reviewed 2023-09-21 07:55:41 +00:00

kml/kml_to_kmb/kml_to_kmb.cpp

strump commented

2023-09-21 07:54:34 +00:00

Fixed

kml/types.hpp

strump commented

2023-09-21 07:50:47 +00:00

Compiler shows warning: all paths through this function will call itself because types Timestamp and TimestampMillis could be converted to one another. Is there any way to force use DebugPrint(Timestamp) function from base/internal/message.hpp ?

Compiler shows warning: `all paths through this function will call itself` because types `Timestamp` and `TimestampMillis` could be converted to one another. Is there any way to force use `DebugPrint(Timestamp)` function from `base/internal/message.hpp` ?

kml/types_v8.hpp

strump commented

2023-09-21 07:54:23 +00:00

Fixed

strump commented

2023-09-21 07:54:28 +00:00

Fixed.
We need default constructor to initialize std::vector<BookmarkDataV8> m_bookmarksData; field in FileDataV8

Fixed. We need default constructor to initialize `std::vector<BookmarkDataV8> m_bookmarksData;` field in FileDataV8

biodranik (Migrated from github.com) reviewed 2023-09-22 13:11:57 +00:00

kml/serdes_binary_v8.hpp

					
				@ -0,0 +18,4 @@

				  explicit SerializerKmlV8(FileData & data) : SerializerKml(data) {};

				  template <typename Sink>

				  void Serialize(Sink & sink)

biodranik (Migrated from github.com) commented

2023-09-22 13:11:57 +00:00

That's ok.

strump reviewed 2023-09-26 08:59:19 +00:00

kml/types_v8mm.hpp

strump commented

2023-09-26 08:59:13 +00:00

Appeared that V8MM format doesn't have m_type field. Because of that we had m_visibility = false. Using default m_type for all V8MM.

Appeared that V8MM format doesn't have `m_type` field. Because of that we had `m_visibility = false`. Using default `m_type` for all V8MM.

map/bookmark_manager.cpp

					
				@ -1873,3 +1874,3 @@

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Text);

				      else if (ext == ".gpx")

				      else if (ext == kGpxExtension)

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Gpx);

strump commented

2023-09-26 08:57:02 +00:00

When you import KMB file it is converted to KML first in GetKMLPath(...) function and then KML is imported.
In such case filePath = "some_file.kmb and fileSavePath = "some_file.kmb2.kml". Because we parse fileSavePath we should get extension correctly here.

When you import KMB file it is converted to KML first in `GetKMLPath(...)` function and then KML is imported. In such case `filePath = "some_file.kmb` and `fileSavePath = "some_file.kmb2.kml"`. Because we parse `fileSavePath` we should get extension correctly here.

map/bookmark_manager.cpp

strump commented

2023-09-26 08:57:46 +00:00

In my cases there was a problem with fileSavePath parsing. And temp files were not cleaned up.

In my cases there was a problem with `fileSavePath` parsing. And temp files were not cleaned up.

biodranik (Migrated from github.com) reviewed 2023-09-28 23:15:13 +00:00

map/bookmark_manager.cpp

					
				@ -1873,3 +1874,3 @@

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Text);

				      else if (ext == ".gpx")

				      else if (ext == kGpxExtension)

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Gpx);

biodranik (Migrated from github.com) commented

2023-09-28 23:15:13 +00:00

It would be even better to see these comments in the code instead of the PR ;-)

biodranik (Migrated from github.com) reviewed 2023-09-28 23:16:08 +00:00

kml/types_v8mm.hpp

biodranik (Migrated from github.com) commented

2023-09-28 23:16:08 +00:00

Let's mention it in the code.

biodranik (Migrated from github.com) reviewed 2023-09-28 23:19:36 +00:00

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2023-09-28 23:19:35 +00:00

Both if and else branches have this line. Put it before the if.

biodranik (Migrated from github.com) reviewed 2023-09-28 23:20:11 +00:00

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2023-09-28 23:20:11 +00:00

What kind of problem did you have with parsing? Should it be fixed/handled properly?

biodranik (Migrated from github.com) reviewed 2023-09-29 09:28:51 +00:00

kml/serdes_binary_v8.hpp

biodranik (Migrated from github.com) commented

2023-09-29 09:27:20 +00:00

nit: remove unnecessary newline

biodranik (Migrated from github.com) commented

2023-09-29 09:27:24 +00:00

nit: remove unnecessary newline

biodranik (Migrated from github.com) commented

2023-09-29 09:27:48 +00:00

namespace kml::binary

kml/types_v8mm.hpp

biodranik (Migrated from github.com) commented

2023-09-29 09:28:47 +00:00

// BookmarkDataV8MM contains the same fields as BookmarkDataV8 but without compilations.

nit: Have dots at the end of sentences.

```suggestion // BookmarkDataV8MM contains the same fields as BookmarkDataV8 but without compilations. ``` nit: Have dots at the end of sentences.

strump reviewed 2023-09-30 14:47:24 +00:00

map/bookmark_manager.cpp

					
				@ -1873,3 +1874,3 @@

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Text);

				      else if (ext == ".gpx")

				      else if (ext == kGpxExtension)

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Gpx);

strump commented

2023-09-30 14:47:24 +00:00

Reviewed this part of code because ext is always "kml".

Reviewed this part of code because `ext` is always `"kml"`.

strump reviewed 2023-09-30 14:49:11 +00:00

map/bookmark_manager.cpp

strump commented

2023-09-30 14:49:11 +00:00

If input KML/KMZ file has invalid format we get kmlData == nullptr.

If input KML/KMZ file has invalid format we get `kmlData == nullptr`.

strump reviewed 2023-09-30 14:49:52 +00:00

kml/types_v8mm.hpp

strump commented

2023-09-30 14:49:52 +00:00

Removed this field completly. Added comment.

strump reviewed 2023-09-30 14:50:03 +00:00

kml/serdes_binary_v8.hpp

strump commented

2023-09-30 14:50:03 +00:00

Fixed.

strump reviewed 2023-09-30 14:50:10 +00:00

kml/serdes_binary_v8.hpp

strump commented

2023-09-30 14:50:09 +00:00

Fixed.

strump reviewed 2023-09-30 14:50:15 +00:00

kml/serdes_binary_v8.hpp

strump commented

2023-09-30 14:50:15 +00:00

Fixed

strump reviewed 2023-09-30 14:50:34 +00:00

kml/types_v8mm.hpp

strump commented

2023-09-30 14:50:34 +00:00

Fixed.

biodranik (Migrated from github.com) reviewed 2023-09-30 18:39:36 +00:00

map/bookmark_manager.cpp

					
				@ -1873,3 +1874,3 @@

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Text);

				      else if (ext == ".gpx")

				      else if (ext == kGpxExtension)

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Gpx);

biodranik (Migrated from github.com) commented

2023-09-30 18:39:36 +00:00

Did you add a comment in the code? Btw, why is it always kml there?

biodranik (Migrated from github.com) reviewed 2023-09-30 20:33:45 +00:00

map/bookmark_manager.cpp

					
				@ -1873,2 +1873,3 @@

				      if (ext == kKmlExtension)

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Text);

				      else if (ext == ".gpx")

				      else if (ext == kGpxExtension)

biodranik (Migrated from github.com) commented

2023-09-30 20:33:45 +00:00

Are you 100% sure that this code should be removed? Did you test it with other file types? Is it guaranteed that this code is called only for kml/kmz files?

strump reviewed 2023-10-01 11:22:16 +00:00

map/bookmark_manager.cpp

					
				@ -1873,2 +1873,3 @@

				      if (ext == kKmlExtension)

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Text);

				      else if (ext == ".gpx")

				      else if (ext == kGpxExtension)

strump commented

2023-10-01 11:22:16 +00:00

I tested with KML, KMZ, KMB, and GPX files.
Found an issue with GPX and fixed. Current implementation imports all formats successfully.

One questionable part: KMB is first converted to temp KML and then KML is converted to final KML. Maybe we can skip temporary step.

Nice to have: unit tests 🙂

I tested with KML, KMZ, KMB, and GPX files. Found an issue with GPX and fixed. Current implementation imports all formats successfully. One questionable part: KMB is first converted to temp KML and then KML is converted to final KML. Maybe we can skip temporary step. Nice to have: unit tests 🙂

biodranik (Migrated from github.com) reviewed 2023-10-01 20:52:32 +00:00

map/bookmark_manager.cpp

					
				@ -1873,2 +1873,3 @@

				      if (ext == kKmlExtension)

				        kmlData = LoadKmlFile(fileSavePath, KmlFileType::Text);

				      else if (ext == ".gpx")

				      else if (ext == kGpxExtension)

biodranik (Migrated from github.com) commented

2023-10-01 20:52:32 +00:00

Converting kml to kml doesn't look necessary ) Is it easy to fix?

biodranik (Migrated from github.com) reviewed 2023-10-01 20:56:50 +00:00

map/bookmark_helpers.cpp

					
				@ -358,6 +376,7 @@ std::string GetKMLPath(std::string const & filePath)

				  }

				  else if (fileExt == kKmzExtension)

				  {

				    // Extract KML file from KMZ archive and save to temp KML with unique name.

biodranik (Migrated from github.com) commented

2023-10-01 20:56:50 +00:00

Why is this copying necessary?

strump reviewed 2023-10-02 09:00:46 +00:00

map/bookmark_manager.cpp

strump commented

2023-10-02 09:00:14 +00:00

collection has type std::vector<std::pair<std::string, std::unique_ptr<kml::FileData>>>
I converted std::optional<kml::FileData> to std::unique_ptr<kml::FileData> here.
Should we change collection type?

`collection` has type `std::vector<std::pair<std::string, std::unique_ptr<kml::FileData>>>` I converted `std::optional<kml::FileData>` to `std::unique_ptr<kml::FileData>` here. Should we change `collection` type?

biodranik (Migrated from github.com) reviewed 2023-10-02 09:13:22 +00:00

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2023-10-02 09:13:22 +00:00

Good question, likely yes, if the collection is not moved often. Moving unique_ptr is cheaper than moving FileData.

Otherwise:

    collection->emplace_back(filePath, std::make_unique<kml::FileData>(std::move(kmlData)));

Good question, likely yes, if the collection is not moved often. Moving unique_ptr is cheaper than moving FileData. Otherwise: ```suggestion collection->emplace_back(filePath, std::make_unique<kml::FileData>(std::move(kmlData))); ```

strump reviewed 2023-10-02 09:52:06 +00:00

map/bookmark_manager.cpp

strump commented

2023-10-02 09:52:06 +00:00

Your suggested code doesn't work. Compilation error:
error: no matching constructor for initialization of 'kml::FileData'

Replaced with:
std::make_unique<kml::FileData>(std::move(kmlData.value()))

Your suggested code doesn't work. Compilation error: `error: no matching constructor for initialization of 'kml::FileData'` Replaced with: `std::make_unique<kml::FileData>(std::move(kmlData.value()))`

vng (Migrated from github.com) reviewed 2023-10-02 12:25:22 +00:00

vng (Migrated from github.com) left a comment

So you what was the reason to modify existing code with optional?
And then writing "nice":

std::make_unique<kml::FileData>(std::move(kmlData.value())))

So you what was the reason to modify existing code with optional? And then writing "nice": ``` std::make_unique<kml::FileData>(std::move(kmlData.value()))) ```

kml/type_utils.hpp

					
				@ -19,0 +20,4 @@

				public:

				  TimestampMillis() = default;

				  explicit TimestampMillis(Timestamp const & ts) : Timestamp{ts} {}

				  TimestampMillis & operator=(Timestamp const & ts)

vng (Migrated from github.com) commented

2023-10-02 12:19:42 +00:00

I suspect it is not needed.

rtsisyk reviewed 2023-10-02 12:46:18 +00:00

kml/kml_tests/tests_data.hpp

					
				@ -1525,1 +1525,4 @@

				};

				// kBinKmlV8 contains the same bookmarks and tracks as kBinKmlV7

				std::vector<uint8_t> const kBinKmlV8 = {