Add JSON serialization hooks to the lists_getIndex block

[BeksOmega]

Describe the bug

One of the validators on the lists_getIndex block changes whether the block is a stack block (with next and previous connections) or a value block (with an output connection). This information needs to be stored in a mutator so that the block is in the proper state before being connect to its parent block. See the deserialization order documentation for more info.

I didn't realize this originally, so I didn't add JSON hooks when updating the lists_getIndex block.

Expected behavior

The block should have a JSON serialization hook including data about whether it is a stack or value block.

Additional context

Currently the block serializes stringified XML as its extra state. When adding JSON hooks, we need to be careful to make sure it can load this data. I think we can just do an if (typeof value === 'string') check, but there should be a test for it.

If it isn't possible to add JSON hooks, then the comment here should be removed and replaced with a more accurate comment.

This was originally reported by a lovely forum person here.

[gregdyke]

This looks like a good first issue for me, but I need a bit of clarification

• The block currently works, right? The difference will be that after, the serialized version will be a json object (faster to process and empty by default to save space?) rather than "<mutation statement=\"true\" at=\"false\"></mutation>"
• Could you clarify what is meant by "If it isn't possible to add JSON hooks, then the comment here should be removed and replaced with a more accurate comment.". In what case might it not be possible to add JSON hooks?

[gregdyke]

Actually, while I'm asking questions, I realise I don't understand how/why the domToMutation xml is serialized in the JSON? I can't find a default saveExtraState implementation that calls domToMutation. And yet in the forum post, the author reports their json as including the mutation xml in the extra state?

[cmjcharlton]

Looking at the code in /core/serialization/blocks.js the function saveExtraState:

blockly/core/serialization/blocks.js

Line 168 in b92cbd3

const saveExtraState = function(block, state) {

falls back to saving a stringified version of the XML if saveExtraState() isn't defined for a block but mutationToDom() is.

When the block is loaded the function loadExtraState:

blockly/core/serialization/blocks.js

Line 447 in b92cbd3

const loadExtraState = function(block, state) {

loads the XML via domToMutation() if loadExtraState() isn't defined for the block.

[BeksOmega]

This looks like a good first issue for me, but I need a bit of clarification

Awesome! Thank you for your interest :D

The block currently works, right? The difference will be that after, the serialized version will be a json object (faster to process and empty by default to save space?) rather than "<mutation statement="true" at="false">"

Correct!

Could you clarify what is meant by "If it isn't possible to add JSON hooks, then the comment here should be removed and replaced with a more accurate comment.". In what case might it not be possible to add JSON hooks?

So currently saves contain stringified XML. We need to make sure that those saves continue to load. Hence the reference to a backwards-compatible if (typeof value === 'string') check. If for whatever reason we can't get the JSON hooks to load the stringified XML, then we will need to remove the comment (and add a new clarifying comment) as referenced above.

---

I hope that helps! If you have any further questions don't hesitate to ask =)

[gregdyke]

Thanks for the hint @cmjcharlton !

@BeksOmega So all the list blocks (e.g lists_setIndex) that have domToMutation butdon't have explicit save/loadExtraState are actually serializing xml mutations that they probably don't need?

And in general, the following comment is wrong:

  // This block does not need JSO serialization hooks (saveExtraState and
  // loadExtraState) because the state of this object is already encoded in the
  // dropdown values.
  // XML hooks are kept for backwards compatibility.

they do need explicit json serialisation hooks with a null implementation so that unnecessary state is not saved/loaded from xml?

[gregdyke]

(put another way, should I create the - possibly null operation - implementations of JSON serialization hooks for all the lists block as part of fixing this issue? Or should I keep the PRs small with one block at a time?)

[BeksOmega]

(put another way, should I create the - possibly null operation - implementations of JSON serialization hooks for all the lists block as part of fixing this issue? Or should I keep the PRs small with one block at a time?)

Keeping PRs small is always a great way to go because it makes things easier to test and review =)

So all the list blocks (e.g lists_setIndex) that have domToMutation butdon't have explicit save/loadExtraState are actually serializing xml mutations that they probably don't need?

Yes :D It seems my past self didn't realize she needed to update the block definitions again after adding the backwards compatibility hehe.

Also that issue should have a separate tracking number. If you want to file the issue you totally can, or I can get to it later =)

Thank you for your interest in fixing this! We really appreciate it =)

[gregdyke]

I think this can now be closed.

[BeksOmega]

Indeed! Closed by #6170