Copying attribute_entries when creating a new block

[kduske-n4]

I'm trying to create a new block in a tree processor. This block should retain the attribute_entries of another block, but when I pass a copy of the original block's attributes to Processor#createBlock, they are not stored. This leads to document variables being replaced with incorrect values for the newly created block.

The first problem appears to be in RubyAttributesMapDecorator#createJavaMap which doesn't handle keys that are Ruby symbols. Adding

 } else if (rubyKey instanceof RubySymbol) {
      key = ((RubySymbol) rubyKey).asJavaString();

puts the attribute_entries into the Java map. But then the second problem kicks in, which is that this key must be converted back to a Ruby symbol when the attributes are stored in the options. In method Processor#createBlock(StructuralNode, String, List<String>, Map<String, Object>, Map<Object, Object>), the attributes get put into the options:

options.put(Options.SOURCE, content);
options.put(Options.ATTRIBUTES, new HashMap<String, Object>(attributes));

They are then passed on to Processor#createBlock(StructuralNode, String, Map<Object, Object>), where the options are converted to a Ruby hash:

RubyHash convertMapToRubyHashWithSymbols = RubyHashUtil.convertMapToRubyHashWithSymbolsIfNecessary(rubyRuntime, options);

But the key attribute_entries in the nested attributes is not treated specially, so it remains a string rather than a symbol. If I convert the attributes map to a Ruby hash myself (converting the keys to symbols) before they are put into the options, it works, but then other keys get lost (e.g. role) since those are converted to symbols when they aren't expected to be.

So, long story short, how to I store things which are expected to be symbols in the attributes map and have them show up correctly in the Ruby AST?

[robertpanzer]

@kduske-n4 Thank you for such a detailed issue report, this is really awesome.
I tried to reproduce the error you mentioned but I couldn't find it.
I added a test case in #514, maybe you can take a look at that and see if I missed sth in in your description.

What I realized though when creating the test was, that there is no way yet to set the style of a node (Or I forgot it due to vacation mode in my brain). Therefore I added that setter.

[kduske-n4]

Hi @robertpanzer, thank you for your reply. I will create a pull request with a test that demonstrates the issue tomorrow.

[mojavelinux]

I want to add a word of warning here. If you are relying on attribute_entries, your code is likely going to break in the future. That's an internal API that's slated to be removed once we fix how we manage attributes internally. I don't yet have a timetable for when we are going to remove it, but I don't recommend interacting with it directly.

What we could do is introduce a method in the AsciidoctorJ API that allows you to clone a block. That would abstract all the internals away so that when core changes, AsciidoctorJ can make the necessary adjustments. The client code would then be safe from breakage.

[mojavelinux]

The way you know that an attribute is internal is that it's a symbol. The public attributes are the ones that have string keys (we're still discussing the integer-based, positional attributes and how to handle those).

[mojavelinux]

In addition / alternative to cloning a block, we can introduce a method to clone a block's attributes. Either way, this internal state needs to be abstracted away from the client.

[kduske-n4]

Hi @mojavelinux, thanks for your input. Cloning a block would be a great addition indeed. I would suggest adding cloning support for all AST nodes, as blocks are not the only types of nodes that may need to get cloned. However, there must still be the possibility to change the clones, for example, I may need to clone a block, but change its content while keeping everything else the same. Since I cannot change the content after the fact, the clone method(s) must take the content as a parameter. This might also apply to other properties of the cloned nodes that cannot be changed otherwise, such as the text of table cells and list items.

@robertpanzer In light of @mojavelinux's feedback, I suppose that a test case for the behavior I described initially is not necessary. I'll just wait for the cloning support and until then, we can make do without document variables.

[kduske-n4]

Thinking about it, I would not need to clone anything at all if it was possible to change all properties of AST nodes. I am only cloning the blocks because I need to change their contents. If I could do that, I could do away with the cloning altogether.

[mojavelinux]

I would suggest adding cloning support for all AST nodes, as blocks are not the only types of nodes that may need to get cloned.

You read my mind.

I would not need to clone anything at all if it was possible to change all properties of AST nodes.

That's what we should continue pushing for in the 1.6.0 branch. Perhaps after a few more fixes @robertpanzer would be willing to do a alpha.4 release (on his return, of course).

[robertpanzer]

Perhaps after a few more fixes @robertpanzer would be willing to do a alpha.4 release (on his return, of course).

Of course. I just need to know what's missing.
Setting the style of a node seemed to be one thing.
Will go through the properties again.

[mojavelinux]

Keep in mind that this is the first I've really thought about which properties are public and which are internal, so it might help if you list the properties you want to set and we can go through them.

[kduske-n4]

@robertpanzer Currently, I have to work around the following problems with hacks / node cloning:

• Cannot query the text of a list item or a table cell without having all substitutions applied (I need the raw text).
• Cannot change the text of a list item or a table cell.
• Cannot change the contents of a block.
• Cannot copy the document variables attached to a structured node (see above).
• Looking at the interface DescriptionListEntry, it appears that it might require a setter for its description, too, but I haven't had the need for this (yet).
• Cannot simply copy the substitutions gotten from a block via getSubstitutions to another block, since I have to prepend them all with ':' (this is really minor).
• Cannot simply copy the content model gotten from a block via getContentModel since I have to prepend the value with ':' (this is really minor).

This is what I can think of right now.

[mojavelinux]

Where are we with this issue? It might be best to create separate issues so we can track each improvement independently, though it's okay to use a comment to summarize / brainstorm.