-
Notifications
You must be signed in to change notification settings - Fork 125
Auxiliary symbol properties #2136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Auxiliary symbol properties #2136
Conversation
fa730df to
d2790d9
Compare
d2790d9 to
b5ef334
Compare
b5ef334 to
13a86c8
Compare
dg0yt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early notes.
src/core/symbols/symbol.h
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/fileformats/ocd_file_import.cpp
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I put the enums in the source files. I'm not sure whether the anonymous namespaces around are superfluous.
| /** | ||
| * 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/gdal/ogr_file_format.cpp
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
Test file: |
|
Adding an automated test as proposed by @dg0yt shows an interesting behaviour (maybe a bug in my version of OGR):
I assume that this is the root cause (if I understand "%.3g" correctly): |
|
Note: Code 71 due to AutoCAD 2012 DXF Reference Note: Revise |
e89b490 to
c5bdadc
Compare
c5bdadc to
39ebb69
Compare
|
Rebased to resolve merge conflicts. |
39ebb69 to
487d168
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. Extend the Symbol base class for storing auxiliary symbol properties that 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.
0a8367d to
37b09a5
Compare
|
I propose to merge #2421 first. |
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.