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