Remove serialization from Table<T> and make TaggedValue only store tooling/widget node inputs#4129
Remove serialization from Table<T> and make TaggedValue only store tooling/widget node inputs#4129
Conversation
…peDefault on load Documents written before the TypeDefault mechanism existed have empty Table<Vector>/<Raster>/<Graphic>/<Artboard> values baked into every unwired exposed input. Walk each migrated node's inputs and rewrite any such placeholder NodeInput::Value into the equivalent NodeInput::type_default, so re-saved documents shed the placeholder payloads. Marked with a TODO for eventual removal once enough documents have been re-saved.
…ent(GradientStops)
…hStrokes(Vec<BrushStroke>)
Keavon
requested review from
TrueDoctor and
adamgerhant
and removed request for
adamgerhant
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the TaggedValue enum and its associated serialization/deserialization logic to move away from storing large, type-erased Table<T> structures in favor of more compact, type-specific variants. It introduces TypeDefault to handle default values for types where the specific variant has been removed, and implements custom migration logic for legacy document formats. I have identified a potential infinite recursion issue in the to_dynany and to_any methods regarding TypeDefault handling, which should be refactored into a shared helper function.
| Self::TypeDefault(td) => { | ||
| // Construct the actual default for types without a `TaggedValue` variant directly, instead of going through | ||
| // `from_type_or_none` (which would just return `TypeDefault` again and recurse forever). | ||
| let name = td.name.as_ref(); | ||
| if name == std::any::type_name::<Table<Graphic>>() { return Box::new(Table::<Graphic>::default()); } | ||
| if name == std::any::type_name::<Table<Artboard>>() { return Box::new(Table::<Artboard>::default()); } | ||
| if name == std::any::type_name::<Table<Raster<CPU>>>() { return Box::new(Table::<Raster<CPU>>::default()); } | ||
| if name == std::any::type_name::<Table<Vector>>() { return Box::new(Table::<Vector>::default()); } | ||
| if name == std::any::type_name::<Table<String>>() { return Box::new(Table::<String>::default()); } | ||
| if name == std::any::type_name::<DocumentNode>() { return Box::new(DocumentNode::default()); } | ||
| Self::from_type_or_none(&Type::Concrete(td)).to_dynany() |
There was a problem hiding this comment.
The logic for materializing default values for TypeDefault is duplicated between to_dynany and to_any (lines 168-178). Additionally, the fallback recursion Self::from_type_or_none(&Type::Concrete(td)).to_dynany() is dangerous: if from_type returns TypeDefault for a type not explicitly handled in the if name == ... chain, it will cause an infinite loop.
Consider refactoring this into a helper function that maps a type name to its default value, and ensure the recursion has a base case or check to prevent cycles.
1 participant
Split commit by commit.