Fix copy/paste of list properties (#4514) and clean up RecursiveBehavior#4517
Merged
Conversation
The propertiesToJson and propertiesFromJson helpers (used by the
clipboard, project files and world files) did not handle list-typed
properties. With ListsAsExportValues, toExportValue wraps each list
element in an ExportValue. Passing the resulting QVariantList directly
to QJsonValue::fromVariant turned every element into a JSON null, so
pasting a list property produced an empty list whose items would be
saved as type="std::nullptr_t" in TMX.
The same bug affected lists nested inside class properties: a class
member that's a list would also be null-ified by the JSON conversion.
Add a JsonReady RecursiveBehavior in which toExportValue produces a
fully-expanded, JSON-serializable variant tree. Each list element is
written as a {type, propertytype, value} map, and the inverse pre-pass
in toPropertyValue turns those maps back into typed values. This walks
through class maps too, so list members nested inside classes round
trip correctly.
The propertiesToJson, propertiesFromJson, valuesToJson and
valuesFromJson helpers now switch the context into JsonReady mode and
no longer need any special-casing for lists. propertytype is omitted
from the JSON when empty, matching the JSON2 map format.
Closes #4514
MapToVariantConverter::exportValuesToVariantMap and
VariantToMapConverter::convertListValues recursively walked the variant
tree to wrap and unwrap ExportValue items. Now that ExportContext does
the same work in JsonReady mode, switch both converters to JsonReady
and inline the small remaining bit of code (just building the
{name, type, value, propertytype?} map for the top-level property).
The output format is unchanged: propertytype is still omitted from each
property and list-element map when empty.
ListsAsExportValues was never explicitly requested by any caller. It
used to be the default and produced a half-baked
QVariantList<QVariant<ExportValue>> that nothing in the codebase
actually wanted to consume directly. Drop the enumerator and switch
the default to JsonReady, which yields a fully decomposed,
JSON-serializable variant tree.
Make convertListValues defensive while we're at it: it now skips list
items that aren't {type, value, propertytype} wrapper maps, so call
sites that pass already-typed list values through toPropertyValue
(e.g. the XML map reader) keep working under the new default.
Replace the heuristic in convertListValues (skip non-map items, skip maps without a "type" key) with an explicit RecursiveBehavior that declares "values are taken as-is, do not recurse into list elements". RecursiveBehavior is now purely write-side for the other modes, plus this read-side opt-out. The pre-pass in toPropertyValue runs only when the caller has asked for TypedListValues, so it can rely on the input being in wrapper form and no defensive check is needed. NoRecursion is now the default: any caller that has not picked a mode gets a context that does not transform values in either direction, and the JSON helpers and the JSON2 converter remain explicit about asking for TypedListValues. The XML map reader, which builds its own list values from <item> elements, sets NoRecursion explicitly to document intent.
Previously the XML map writer ran the ExportContext in ExportValuesOnly mode, so toExportValue produced a fully wrapped tree (QVariantList<QVariant<ExportValue>> for lists, QVariantMap of QVariant<ExportValue> for class members). writeExportValue then walked that wrapped tree to emit XML. Switch the writer to NoRecursion mode and have writePropertyValue recurse into the original value tree itself, mirroring the way the XML reader recursively decodes <item> and nested <property> elements. The ExportContext is only consulted for per-value type info and scalar conversion (color, file, object reference); compound types are walked by the writer. Add a round-trip test that writes and reads a map containing a list-typed property and a class-typed property whose member is a list, confirming the XML format survives the new writer/reader path.
Previously ClassPropertyType::toExportValue iterated and converted class members regardless of recursive behavior. With NoRecursion, that left a wart: the XML writer (which uses NoRecursion) had to ignore exportValue.value and pull the typed members out of the original PropertyValue itself. Make ClassPropertyType::toExportValue check the mode and skip the loop in NoRecursion, so the returned ExportValue.value is the original map of typed members. The XML writer can then iterate exportValue.value directly. The JSON1 path in MapToVariantConverter::toVariant(const Properties&) relied on the previous NoRecursion behavior to scalar-convert class members, so it now sets ValuesOnly explicitly (which already recurses through classes and converts members) to keep the JSON output valid.
After the XML map writer was switched to NoRecursion, the only remaining caller of ExportValuesOnly was PropertyTypes::toJson, which relied on it so exportValueToJson could walk a tree of QVariant< ExportValue> wrappers. Refactor exportValueToJson to take the original typed value plus the context, and recurse by calling toExportValue at each level for type info. ClassPropertyType::toJson passes the raw member value through. PropertyTypes::toJson now uses the default NoRecursion mode. With no remaining users, drop ExportValuesOnly from the enum and simplify ClassPropertyType::toExportValue's loop.
The two regression tests added earlier set up the same property type, the same Properties value, and compared the same round trip; they only differed in which serialization path was being exercised. Fold the XML round trip from test_MapReader into the existing test_Properties::roundTripListProperty so the shared setup lives once and both paths (the JSON helpers and the TMX MapWriter/MapReader) are verified against the same Properties value.
NoRecursion is now the default RecursiveBehavior, so the explicit setters in MapReaderPrivate::readProperties and MapWriterPrivate::writeProperties were no-ops. Replace them with the brief "// NoRecursion mode" comment used by PropertyTypes::toJson.
The list-element decoding lived in a separate convertListValues helper that toPropertyValue ran as a pre-pass in TypedListValues mode. Move the work into the QVariantList branch of toPropertyValue itself. When a list value reaches that path in TypedListValues mode it now iterates the wrapper maps, reconstructs each ExportValue and recurses, which naturally handles nested lists. ClassPropertyType::toPropertyValue no longer skips list members on the same-type fast path so the inner call still gets a chance to decode wrapper items inside class members.
bjorn
force-pushed
the
fix-list-property-copy-paste-4514
branch
from
9655f22 to
6c80ac3
Compare
propertiesToJson and propertiesFromJson now come in two flavors. The ExportContext-taking overload carries the real implementation, asserts that the context is in TypedListValues mode, and uses the caller's context directly without copying. The QString-taking overload builds an ExportContext, switches it to TypedListValues, and forwards to the ExportContext overload. Project::save and Project::load construct their own ExportContext with the project's PropertyTypes (since they may run while the project is not yet, or is no longer, the current one) and now set TypedListValues on it before calling propertiesToJson / propertiesFromJson. World load uses the path overload. valuesToJson and valuesFromJson stay path-only; the clipboard does not need to thread project-specific types through them. The roundTripListProperty test resets Object::setPropertyTypes at the end of the function rather than inside the XML scope, matching where the setup happens.
bjorn
force-pushed
the
fix-list-property-copy-paste-4514
branch
from
6c80ac3 to
1da39e3
Compare
bjorn
added a commit
that referenced
this pull request
May 22, 2026
…ior (#4517) Fix copy/paste of list properties and related clean-up Copying a list custom property and pasting it onto another object dropped the items, leaving them saved as `type="std::nullptr_t"` in TMX. The same bug applied to lists nested inside class properties and also affected storing list properties on projects or worlds. The clipboard, project files, and world files serialize through `propertiesToJson` / `propertiesFromJson`, which fed `QJsonValue::fromVariant` a tree containing `ExportValue` wrappers it was not programmed to handle. They now use a new `TypedListValues` `RecursiveBehavior` that produces `{type, propertytype, value}` maps for each list element (matching the JSON map format on disk) and decodes the same shape on read. A few cleanups while in the area: - Folded the JSON map converter helpers (`exportValuesToVariantMap`, `convertListValues`) into `ExportContext` so the same recursion logic is shared. - Replaced the half-baked `ListsAsExportValues` default with an explicit `NoRecursion` default. Callers that walk compound values themselves (the XML reader and writer, `PropertyTypes::toJson`) now use the default. The JSON helpers opt in to `TypedListValues`. - Made the XML writer mirror the XML reader, driving recursion through lists and class members itself, so `ExportValuesOnly` was no longer needed and was removed. - Added autotests for round-tripping list values to XML and JSON. Closes #4514 (cherry picked from commit c9ad67b)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4514: copying a list custom property and pasting it onto another object dropped the items, leaving them saved as
type="std::nullptr_t"in TMX. The same bug applied to lists nested inside class properties.The clipboard, project files, and world files serialize through
propertiesToJson/propertiesFromJson, which fedQJsonValue::fromVarianta tree containingExportValuewrappers it could not encode. They now use a newTypedListValuesRecursiveBehaviorthat produces{type, propertytype, value}maps for each list element (matching the existing JSON2 map format on disk) and decodes the same shape on read.A few cleanups while in the area:
exportValuesToVariantMap,convertListValues) intoExportContextso the same recursion logic is shared.ListsAsExportValuesdefault with an explicitNoRecursiondefault. Callers that walk compound values themselves (the XML reader and writer,PropertyTypes::toJson) now use the default; the JSON helpers and the JSON2 converter opt in toTypedListValues.ExportValuesOnlywas no longer needed and was removed.Test plan
test_Properties::roundTripListPropertycovers list, list-of-lists, and class-with-list through both the JSON helpers and the TMX writer/reader.test_properties,test_mapreader,test_automapping, andtest_staggeredrenderershow the same totals as on master (pre-existing failures only).