Fixed Serialization issues #9
Conversation
| } | ||
|
|
||
| /*@Override | ||
| public ChangeDialogAction deserialize(PersistedData data) { |
There was a problem hiding this comment.
I think you could clean it up. :-)
| public CloseDialogAction deserialize(PersistedData data, DeserializationContext context) { | ||
| return new CloseDialogAction(); | ||
| public Optional<CloseDialogAction> deserialize(PersistedData data) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Optional.empty() is returned only when there was no value to deserialize/there was an error deserializing the value. In this case, you should return Optional.of(new CloseDialogAction()).
| } | ||
|
|
||
| @Override | ||
| public PersistedData serialize(ChangeDialogAction action, PersistedDataSerializer context) { |
There was a problem hiding this comment.
Please delete this and override serializeNonNull instead.
|
|
||
| @Override | ||
| public PersistedData serialize(ChangeDialogAction action, SerializationContext context) { | ||
| protected PersistedData serializeNonNull(ChangeDialogAction value, PersistedDataSerializer serializer) { | ||
| return null; |
There was a problem hiding this comment.
Please change this definition to your serialize definition. Also, it is not advised to return null PersisitedData instances -- use serializer.serializeNull() if you want to return null serialized values.
There was a problem hiding this comment.
@eviltak I am just a bit confused, what exactly do you mean by change to my serialize definition
There was a problem hiding this comment.
Sorry for not being clear enough. By that I meant move the code in serialize to serializeNonNull, and delete the serialize method.
|
|
||
| @Override | ||
| public PersistedData serialize(CloseDialogAction action, SerializationContext context) { | ||
| public PersistedData serialize(CloseDialogAction action, PersistedDataSerializer context) { |
There was a problem hiding this comment.
Same as above -- override serializeNonNull instead.
| } | ||
|
|
||
| @Override | ||
| protected PersistedData serializeNonNull(CloseDialogAction value, PersistedDataSerializer serializer) { |
There was a problem hiding this comment.
Same as above -- move serialize's definition here.
| Map<String, PersistedData> data = ImmutableMap.of( | ||
| "type", context.create(action.getClass().getSimpleName()), | ||
| "prefab", context.create(action.getPrefab())); | ||
| "type", context.serialize(action.getClass().getSimpleName()), |
There was a problem hiding this comment.
We don't need this "type" mapping anymore, it can be removed. You could also choose to serialize this as a single string representing the prefab, like return context.serialize(action.getPrefab())
| Map<String, PersistedData> data = ImmutableMap.of( | ||
| "type", context.create(action.getClass().getSimpleName())); | ||
| "type", context.serialize(action.getClass().getSimpleName())); |
There was a problem hiding this comment.
Same as above -- don't need "type" anymore, this can be serialized as a return context.serializeNull() (since there is nothing to serialize)
|
|
||
| @Override | ||
| public PersistedData serialize(NewDialogAction action, SerializationContext context) { | ||
| public PersistedData serialize(NewDialogAction action, PersistedDataSerializer context) { |
There was a problem hiding this comment.
Same as above -- move to serializeNonNull.
| } | ||
|
|
||
| @Override | ||
| protected PersistedData serializeNonNull(NewDialogAction value, PersistedDataSerializer serializer) { | ||
| return null; |
There was a problem hiding this comment.
Same as above -- move serialize's definition here
| "type", context.create(action.getClass().getSimpleName()), | ||
| "target", context.create(action.getTarget())); | ||
| "type", context.serialize(action.getClass().getSimpleName()), | ||
| "target", context.serialize(action.getTarget())); |
There was a problem hiding this comment.
Same as above -- "type" mapping is unnecessary, you could simply return context.serialize(action.getTarget())
| } | ||
|
|
||
| @Override | ||
| public ChangeDialogAction deserialize(PersistedData data, DeserializationContext context) { | ||
| public Optional<ChangeDialogAction> deserialize(PersistedData data) { | ||
| PersistedDataMap root = data.getAsValueMap(); | ||
| String prefab = root.get("prefab").getAsString(); |
There was a problem hiding this comment.
Since you are only serializing the prefab string, data will not be a value map any more -- it will simply be a string. String prefab = data.getAsString() should work. Also, make sure to add a failing condition if the PersistedData is not as expected. For this case, you might want to do something like
if (!data.isString()) {
// Optional.empty() signifies that the data could not be deserialized
return Optional.empty()
}Without this condition, getAsString will throw an exception and the program may crash.
| import org.terasology.persistence.typeHandling.RegisterTypeHandler; | ||
| import org.terasology.persistence.typeHandling.SerializationContext; | ||
| import org.terasology.persistence.typeHandling.SimpleTypeHandler; | ||
| import org.terasology.persistence.typeHandling.*; |
There was a problem hiding this comment.
Please remove this star import. You can also change your IntelliJ glob import setting to a very high number to prevent IntelliJ from doing this automatically :)
| } | ||
|
|
||
| @Override | ||
| public NewDialogAction deserialize(PersistedData data, DeserializationContext context) { | ||
| public Optional<NewDialogAction> deserialize(PersistedData data) { | ||
| PersistedDataMap root = data.getAsValueMap(); | ||
| String target = root.get("target").getAsString(); |
There was a problem hiding this comment.
Same as above -- change deserialization code to use the format in which this type of value is serialized, and use failing conditions to prevent exceptions.
|
Also @MKDEVELOPEMENT in the future please do not merge |
|
Noted, @eviltak |
|
Closing in favor of #11 - thanks again for the work and inspiration during code review :-) |
module now runs without errors