removing/adding childs in _build(...)
Hi all, For backwards compatibility with old-style guides, we want to convert the old-style guides to new-style in sp_namedview_build. Removing the old-style guides from XML and adding new ones works fine, however, this is not represented at all in the SPObject tree. After removing the old-style guides, the SPObjects are still in memory and hooked up to namedview. Also, adding new guides to XML does not create SPObjects belonging to them. This all works fine outside sp_namedview_build (the mechanism is used in hundreds of places in the code), but for some reason does not work within sp_namedview_build. Is that expected behavior? Where should we move the conversion code to?
Cheers, Johan
2012/10/13 Johan Engelen <jbc.engelen@...2592...>:
Hi all, For backwards compatibility with old-style guides, we want to convert the old-style guides to new-style in sp_namedview_build. Removing the old-style guides from XML and adding new ones works fine, however, this is not represented at all in the SPObject tree. After removing the old-style guides, the SPObjects are still in memory and hooked up to namedview. Also, adding new guides to XML does not create SPObjects belonging to them. This all works fine outside sp_namedview_build (the mechanism is used in hundreds of places in the code), but for some reason does not work within sp_namedview_build. Is that expected behavior? Where should we move the conversion code to?
When the SP tree is initially created, the XML node observers, which are responsible for keeping the SP tree up to date, are not yet active. They are added by SPObject::invoke_build only once the build function for your SPObject finishes executing. The children of each object are created by its virtual build function, such as sp_namedview_build. For this document skeleton:
<svg> <g> <path> <path> </g> </svg>
The order of actions is:
call SPRoot::build call SPItemGroup::build call SPPath::build for the first path add first path observer call SPPath::build for the second path add second path observer add group observer add root observer
The XML observers do not send a notification when an object is destroyed. Deleting and creating SP nodes for children is the responsibility of the parent's listeners. This is the fundamental cause of some still unsolved crashes in the node tool (for example, undoing 2 or more stacked LPE applications through the undo history dialog). In particular, if you first create the child SPObject of the namedview, then remove their XML nodes and add some others, there will be no change in the SP tree because the listener for namedview will only activate once you return from sp_namedview_build. I hope this explains the problem.
A quick hack is to explicitly invoke the necessary listeners (SPObject::sp_object_repr_child_removed, SPObject::sp_object_repr_child_added, etc.) on SPNamedview in the build function, but this is rather ugly. A far better solution is to change the build function for the new SPObject for guides. First, check the XML element name. If you encounter the old guide XML, convert it to the new XML using a series of setAttribute() calls and finally call your XML node's setCodeUnsafe() function to change its element name to the one for new guides. After that, execute normally like for new guide XML. Once this is ready, modify sp-object-repr.cpp so that the new SPObject is created for both old and new guide elements. This way the compatibility code is kept in the element it's related to, rather than in its parent.
Regards, Krzysztof
On 15-10-2012 17:40, Krzysztof Kosiński wrote:
2012/10/13 Johan Engelen <jbc.engelen@...2592...>:
Hi all, For backwards compatibility with old-style guides, we want to convert the old-style guides to new-style in sp_namedview_build. Removing the old-style guides from XML and adding new ones works fine, however, this is not represented at all in the SPObject tree. After removing the old-style guides, the SPObjects are still in memory and hooked up to namedview. Also, adding new guides to XML does not create SPObjects belonging to them. This all works fine outside sp_namedview_build (the mechanism is used in hundreds of places in the code), but for some reason does not work within sp_namedview_build. Is that expected behavior? Where should we move the conversion code to?
When the SP tree is initially created, the XML node observers, which are responsible for keeping the SP tree up to date, are not yet active. They are added by SPObject::invoke_build only once the build function for your SPObject finishes executing. The children of each object are created by its virtual build function, such as sp_namedview_build. For this document skeleton:
<svg> <g> <path> <path> </g> </svg>
The order of actions is:
call SPRoot::build call SPItemGroup::build call SPPath::build for the first path add first path observer call SPPath::build for the second path add second path observer add group observer add root observer
The XML observers do not send a notification when an object is destroyed. Deleting and creating SP nodes for children is the responsibility of the parent's listeners. This is the fundamental cause of some still unsolved crashes in the node tool (for example, undoing 2 or more stacked LPE applications through the undo history dialog). In particular, if you first create the child SPObject of the namedview, then remove their XML nodes and add some others, there will be no change in the SP tree because the listener for namedview will only activate once you return from sp_namedview_build. I hope this explains the problem.
A quick hack is to explicitly invoke the necessary listeners (SPObject::sp_object_repr_child_removed, SPObject::sp_object_repr_child_added, etc.) on SPNamedview in the build function, but this is rather ugly. A far better solution is to change the build function for the new SPObject for guides. First, check the XML element name. If you encounter the old guide XML, convert it to the new XML using a series of setAttribute() calls and finally call your XML node's setCodeUnsafe() function to change its element name to the one for new guides. After that, execute normally like for new guide XML. Once this is ready, modify sp-object-repr.cpp so that the new SPObject is created for both old and new guide elements. This way the compatibility code is kept in the element it's related to, rather than in its parent.
Thanks very much for describing the order of things. What is interesting that it did seem to work once for grids (which may have turned into a bug now), but that might be because grid objects are handled somewhat abnormally in SPNamedview.
I don't like adding the compatibility code to the classes themselves. Also, note that in this case the compatibility-fix is easy: simply change some values. But other compatibility patch-ups may involve more than one new objects or in another place in the SVG. For example, the old-style grid was not an SVG leaf and the patch-up required creating a new leaf.
What about adding a specific compatibility section in our file loading code? So after the XML is loaded, but before all objects are created. So the patch-ups involve modifying the XML. Then when all is well, the object tree is created. This way, all patch-ups are grouped together and don't clutter our codebase in different places.
Cheers, Johan
2012/10/15 Johan Engelen <jbc.engelen@...2592...>:
I don't like adding the compatibility code to the classes themselves. Also, note that in this case the compatibility-fix is easy: simply change some values. But other compatibility patch-ups may involve more than one new objects or in another place in the SVG. For example, the old-style grid was not an SVG leaf and the patch-up required creating a new leaf.
What about adding a specific compatibility section in our file loading code? So after the XML is loaded, but before all objects are created. So the patch-ups involve modifying the XML. Then when all is well, the object tree is created. This way, all patch-ups are grouped together and don't clutter our codebase in different places.
This approach is completely unmaintainable. You end up implementing a large portion of your build and write functions by hand in a place that is very far from the actual definition of those functions. For example, adding flowtext fallbacks would be a nightmare.
There's a slightly different approach than the one I wrote about initially, but I realized it is far better. Here's what the new object's build function should do: 1. Check the XML element name 2. If it's an old guide, read SPObject values from old XML and fix them up after reading 3. Change the XML element's name to the new one and unset all attributes 4. Call the write function (the same one that's used when updating the XML when an undo event is committed)
This way we do not need to write back the old values to XML as text just to read them again, and we avoid duplicating the code that writes XML.
The better pattern for backward compatibility is to put the transformation to the new XML in the SPObject for old guides, but with our current approach to constructing the SP tree, I don't see any obvious way to do this.
Regards, Krzysztof
On 15-10-2012 20:29, Krzysztof Kosiński wrote:
2012/10/15 Johan Engelen <jbc.engelen@...2592...>:
I don't like adding the compatibility code to the classes themselves. Also, note that in this case the compatibility-fix is easy: simply change some values. But other compatibility patch-ups may involve more than one new objects or in another place in the SVG. For example, the old-style grid was not an SVG leaf and the patch-up required creating a new leaf.
What about adding a specific compatibility section in our file loading code? So after the XML is loaded, but before all objects are created. So the patch-ups involve modifying the XML. Then when all is well, the object tree is created. This way, all patch-ups are grouped together and don't clutter our codebase in different places.
This approach is completely unmaintainable. You end up implementing a large portion of your build and write functions by hand in a place that is very far from the actual definition of those functions. For example, adding flowtext fallbacks would be a nightmare.
You could add a static member function to the class if you want to have the code close to that. I don't see the unmaintainable aspect of it. In any solution, you will have to write specialized read code anyway. And if the write code is big you can split it out as a class function and all is well. This is not an inherent problem of my idea of first changing the SVG before you construct all sorts of objects from it.
There's a slightly different approach than the one I wrote about initially, but I realized it is far better. Here's what the new object's build function should do:
- Check the XML element name
- If it's an old guide, read SPObject values from old XML and fix
them up after reading 3. Change the XML element's name to the new one and unset all attributes 4. Call the write function (the same one that's used when updating the XML when an undo event is committed)
Your solution works for guides, but will not work for example for grids (old-style grid is not an XML element, it is part of sp-namedview).
This way we do not need to write back the old values to XML as text just to read them again, and we avoid duplicating the code that writes XML.
Writing to XML and reading back is not a huge problem because it is only once upon load.
The better pattern for backward compatibility is to put the transformation to the new XML in the SPObject for old guides, but with our current approach to constructing the SP tree, I don't see any obvious way to do this.
I don't see why we have to make a whole object of this and do it on the fly while creating the object tree. I think it is much simpler and better if we do all backwards compatibility fix-ups at the start before creating all the objects from it, in a centralized place. And do these things one after another even:
file XML load; // 0.46 grids definition changed: CanvasGrid::upgradeTo046(xml document); // 0.50 guide and grid definition changed: CanvasGrid::upgradeTo050(xml document); SPGuide::upgradeTo050(...); continue with XML parsing to create the SPObject tree
This may break for exotic cases where things are lost in translation, but there is no reason to strictly adhere to this stacking of compatibility functions (first ->0.46 and then ->0.50) if it does not come natural.
My main point: first fix the XML, and then start creating objects from it. Don't create objects and fix while building objects.
Ciao, Johan
2012/10/16 Johan Engelen <jbc.engelen@...2592...>:
Your solution works for guides, but will not work for example for grids (old-style grid is not an XML element, it is part of sp-namedview).
Then this is also a change in the XML mapping of sp-namedview. In this case, you just need to construct the child XML node for the grid and add it to the tree before calling the parent class build function (in this case SPObjectGroup::build, which will create and call invoke_build on its children).
I don't see why we have to make a whole object of this and do it on the fly while creating the object tree. I think it is much simpler and better if we do all backwards compatibility fix-ups at the start before creating all the objects from it, in a centralized place. And do these things one after another even:
file XML load; // 0.46 grids definition changed: CanvasGrid::upgradeTo046(xml document); // 0.50 guide and grid definition changed: CanvasGrid::upgradeTo050(xml document); SPGuide::upgradeTo050(...); continue with XML parsing to create the SPObject tree
This may break for exotic cases where things are lost in translation, but there is no reason to strictly adhere to this stacking of compatibility functions (first ->0.46 and then ->0.50) if it does not come natural.
This would require each of these functions to traverse the entire XML tree, which is rather wasteful and requires repetitive code. If you want to do it this way, the tree should be traversed only once and the required conversion functions called whenever the current XML element has a matching element name or sodipodi:type atribute, preferably using a lookup in std::map or std::tr1::unordered_map.
As long as the object upgrade functions are defined in the files of respective SPObjects and are only called by the file upgrade function on relevant XML nodes (e.g. they don't need to reimplement tree traversal), this may be a workable solution.
Regards, Krzysztof
participants (2)
-
Johan Engelen -
Krzysztof Kosiński