Auxiliary symbol properties #2136
Conversation
dl3sdo
force-pushed
the
auxiliary-symbol-properties
branch
from
fa730df to
d2790d9
Compare
dl3sdo
force-pushed
the
auxiliary-symbol-properties
branch
from
d2790d9 to
b5ef334
Compare
Mapper sometimes needs to handle symbol properties that are not part of Mapper's own set of symbol properties and that need to be applied when importing objects. Example: horizontal and vertical text alignment is a symbol property in OCAD but an object property in Mapper. Mapper uses auxiliary data structures to temporarily store those symbol properties for applying them later on object import. This change adds a QVariantHash element to the Symbol base class that allows a flexible and type safe storage of auxiliary symbol properties. These auxiliary symbol properties are neither saved to a map file nor loaded from it.
Text object properties like the text itself, alignment and rotation were imported by putting them into a string and transporting the string via the description property of the related text symbol. This approach suffers from multiple deficiencies: the auxiliary string was not removed from the description field, the rotation angle was rounded (e.g., -17.2 became 20) and the way of constructing and parsing the string was unnecessary complex. Use the auxiliary symbol properties instead to transport the object properties.
Auxiliary symbol properties are replacing the text_halign_map and text_valign_map hashes that were used to store OCAD's symbol properties for horizontal and vertical text alignment for being applied later to text object import.
dl3sdo
force-pushed
the
auxiliary-symbol-properties
branch
from
b5ef334 to
13a86c8
Compare
src/core/symbols/symbol.h
Outdated
| @@ -650,6 +676,7 @@ class Symbol | |||
| bool is_hidden; /// \see isHidden() | |||
| bool is_protected; /// \see isProtected() | |||
| bool is_rotatable = false; | |||
| QVariantHash auxiliary_properties; /// Auxiliary properties for import of objects (e.g., .ocd, .dxf files). Auxiliary properties are not saved in a map file. | |||
There was a problem hiding this comment.
QVariantHash means QHash<QString,QVariant>. Given that the use of this member is tightly scoped to the import of objects, i.e. to particular importers, I would prefer an int key type with named constants (from the actual importer) instant of arbitrary strings. This helps to detect typos at compile time and is cheaper than QString. And with QHash instead of any array-like structure, it should still be robust and convenient enough.
| QVariantHash auxiliary_properties; /// Auxiliary properties for import of objects (e.g., .ocd, .dxf files). Auxiliary properties are not saved in a map file. | |
| QHash<int,QVariant> auxiliary_properties; /// For temporary use during import. Not to be saved on export. |
src/fileformats/ocd_file_import.cpp
Outdated
| @@ -1922,7 +1922,7 @@ Object* OcdFileImport::importObject(const O& ocd_object, MapPart* part) | |||
| auto t = new TextObject(symbol); | |||
| t->setText(getObjectText(ocd_object)); | |||
| t->setRotation(convertAngle(ocd_object.angle)); | |||
| t->setHorizontalAlignment(text_halign_map.value(symbol)); | |||
| t->setHorizontalAlignment((symbol->getAuxiliaryProperty(QLatin1String("HAlign")).value<TextObject::HorizontalAlignment>())); | |||
There was a problem hiding this comment.
With the suggested int keys, this becomes something like
| t->setHorizontalAlignment((symbol->getAuxiliaryProperty(QLatin1String("HAlign")).value<TextObject::HorizontalAlignment>())); | |
| t->setHorizontalAlignment(symbol->getAuxiliaryProperty(OcdFileImportProperties::HAlign).value<TextObject::HorizontalAlignment>()); |
Still longer than the original code... the generalization isn't beneficial here.
There was a problem hiding this comment.
Done, I put the enums in the source files. I'm not sure whether the anonymous namespaces around are superfluous.
src/core/symbols/symbol.h
Outdated
| /** | ||
| * Returns and then removes an auxiliary property of this symbol. | ||
| */ | ||
| QVariant consumeAuxiliaryProperty(QString key); | ||
|
|
||
| /** | ||
| * Returns and then removes an auxiliary property of this symbol or returns a default value. | ||
| */ | ||
| QVariant consumeAuxiliaryProperty(QString key, QVariant default_value); | ||
|
|
There was a problem hiding this comment.
I tend to suggest a single clearAuxiliaryProperties() instead of the consume... pattern. This function could be called for all symbols at the end of an import if necessary and appropriate.
src/gdal/ogr_file_format.cpp
Outdated
| const auto& description = symbol->getDescription(); | ||
| auto length = description.length(); | ||
| auto split = description.indexOf(QLatin1Char(' ')); | ||
| FILEFORMAT_ASSERT(split > 0); | ||
| FILEFORMAT_ASSERT(split < length); | ||
|
|
||
| auto label = description.right(length - split - 1); | ||
| auto label = symbol->consumeAuxiliaryProperty(QLatin1String("label")).toString(); |
There was a problem hiding this comment.
I didn't test now, but I guess that this shows the potential and the missing pieces:
Putting the label into the description was a hack, but it helped to assign objects with the same label to a a single symbol. Probably also on a second and third import into the same map.
This could be covered with an automated test: a test vector file with to identical objects, imported twice. At the end there must be a map with four objects with one symbol.
There was a problem hiding this comment.
Due to my understanding of OgrFileImport::getSymbolForLabel() objects with the same color and font size are assigned to a single symbol, the label as well as anchor and angle are always overwritten.
My proposal would not change the current behaviour, IMO.
|
Test file: |
Mapper sometimes needs to handle symbol properties that are not part of Mapper's own set of symbol properties and that need to be applied when importing objects.
Example: horizontal and vertical text alignment is a symbol property in OCAD but an object property in Mapper.
Mapper uses auxiliary data structures to temporarily store those symbol properties for later applying them on object import.
This change adds a QVariantHash element to the Symbol base class that allows a flexible and type safe storage of auxiliary symbol properties.
These auxiliary symbol properties are neither saved to a map file nor loaded from it.
The second commit applies the auxiliary symbol properties to the DXF import where text object properties like the text itself, alignment and rotation were previously imported by putting them into a string and transporting the string via the description property of the related text symbol.
Besides being an awkward design the approach suffers from multiple deficiencies: the auxiliary string was not removed from the description field, the rotation angle was rounded (e.g., -17.2 became 20) and the way of constructing and parsing the string was unnecessary complex.
Note that this commit replaces #2135.
The third commit applies the auxiliary symbol properties to the import of OCAD maps.
Auxiliary symbol properties are replacing the text_halign_map and text_valign_map hashes that were used to store OCAD's symbol properties for horizontal and vertical text alignment for being applied later to text object import.
This PR is a POC to demonstrate a possible way of preserving symbol/object related properties until object creation.
Auxiliary symbol properties could be considered with respect to #2082 and #1639.