Custom TypedShape to reference dyn Shape instead #216
Conversation
| /// A custom user-defined shape with a type identified by a number. | ||
| Custom(u32), | ||
| /// A custom user-defined shape. | ||
| Custom(&'a dyn Shape), |
There was a problem hiding this comment.
Can a dyn Shape serialize/deserialize ?
There was a problem hiding this comment.
@Vrixyz This would be difficult due to the Deserialize trait bound having a lifetime. This is generally worked around using erased_serde but some additional steps are needed to make deserialization actually work. This sounds out of the scope of this PR.
| DeserializableWorkspaceData::Custom(_) => None, | ||
| DeserializableWorkspaceData::Custom => None, |
There was a problem hiding this comment.
Do I understand that custom shapes are (currently) not supported by serialization/deserialization ? (at least here specifically)
There was a problem hiding this comment.
That is correct. And this is the same behavior as before.
There was a problem hiding this comment.
LGTM, thank you! The u32 was supposed to be a unique id identifying the custom shape type so that some custom serialization/deserialization code could handle it. Though that custom ser/deser isn’t implemented yet and will probably not be implemented for some time.
So I think it’s is fine to remove the u32 for now. Eventually, it will probably be re-added but as a Uuid instead.
The existing
TypedShape::Customcontains a user-defined shape with a type identified by a number.This number has questionable utility. Let's make it so that
TypedShapereferences the custom shape with a&dyn Shapeinstead. The user may check for the actual custom shape type using downcast.Please let me know if there are any reasons to keep this as a
u32I might not be aware of.