GNOME Bugzilla – Bug 600571
Connections spanning layers can not be serialized
Last modified: 2014-01-19 22:08:50 UTC
Created attachment 146852 [details] Small diagram deomonstrating connection point layer error The following steps result in the above error message when re-opening the saved diagram: * Add a shape * Add a new layer * Ensure that connections are turned ON for both layers * Switch the the new layer * Add a connecting line from any connection point on the shape At this point everything is fine, move the shape and the connected line adjusts accordingly. * Save the diagram * Close the diagram * Open the diagram You now get the "Connection point ..." error and the connection is broken. I spotted this on a much more complex diagram involving UML objects and connectors but it seems to be a general issue with retaining connections between layers on re-opening a diagram. The attached simple example should give the error.
When layer spanning connections were implemented the iterative nature of serialization was not considered. Thus the current code can only work if the connections are going backwards in the layer stack, i.e. from an object in an upper layer to some object in a layer below. The given example does not fall in that category. So not only the loading is broken (because the object to connect to is not known when the connection is tried to restore) but also the saving (the connection target is not numbered when the connection is written: <dia:connections> <dia:connection handle="0" to="O0" connection="6"/> </dia:connections> So the 'to' attribute is pointing to the wrong object, here this would mean a connection to itself. Maybe the broken feature of connections spanning layers should be removed again. I don't have a good idea how to make it work without bending the serialization code quite a lot.
Could you not recurse
I had a look to see if this could be done quickly, but there's a slight problem with responsibility for writing out objects: - the logic to ensure objects are written only once, via a hash, is at the highest driving level - in app/load_save - the per-object saves have no visibility of the dedup hash. If each object save was responsible itself for adding itself to the dedup hash then you could recursively write out objects, I think.
IIRC the hash's task is not to dedup, but to give ids to objects. I dont understand your idea with recursion, maybe you should write a patch? It is certainly possible to fix this bug, but I still wonder if the feature is worth the extra complexity.
The hash is also used to track which objects have been saved, it seems. Look at app/load_save.c::write_objects() where does a hash lookup. I was thinking of recursively writing out objects in connections (i.e. calling obj->save in write_connections on the to object - presuming object-saving was aware of the dedup hash in some way, which I'd started to implement). This would work if connections were simplex, e.g. if lines referred to connected_to objects, but the objects referrred to did not refer to the lines. Writing out the lines last then solves the problem (presuming libxml writes out nodes in the same order they were created). If connections are not simplex, then ordering won't cut it. Instead we'd need a 2-pass loading process. Looking at things, this seems even easier than the above and, by avoiding being dependent on libxml preserving ordering, more robust. I've got a first cut at a 2-pass patch, which seems to improve things somewhat (it preserves my lines locations at least), but for some reason its still not setting up connections on load.
Created attachment 148939 [details] [review] First draft patch to 2-pass load diagrammes - not functional though load diagrammes in 2-passes of the XML tree: pass 1: load all objects in all layers, so that all objects are known before: pass 2: load all connections This seems to do better than stock dia - it now at least preserves locations of links that had connections, where it didn't before (or tried to re-arrange them), but it's still not setting back the connections. Anyone know why?
Created attachment 148991 [details] [review] Another attempt at a 2-pass load patch So, previous version was basically not processing the connections at all, cause read_connections wants the objects and layers to match. So I thought I'd hacked that together, but still the xmlNodePtr that read_connections is getting lacks connection data. E.g. this: (gdb) print *obj_node->next->children->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->properties->children $105 = {_private = 0x0, type = XML_TEXT_NODE, name = 0x710a890 "text", children = 0x0, last = 0x0, parent = 0x8669a90, next = 0x0, prev = 0x0, doc = 0x86218b0, ns = 0x0, content = 0x85ff790 "start_arrow_width", properties = 0x0, nsDef = 0x0, psvi = 0x0, line = 89, extra = 0 In the XML file, dia:connections follows <dia:attribute name="start_arrow_width>, but it's not there. As if the XML node has been removed.. Odd...
First I'd suggest to correctly save the connections. I think creating the hash table mapping object pointers to object ids _before_ starting to save anything could do the trick here. The loading could be implemented by temporay storing connection requests between objects which are not constructed yet. But still I wonder if the feature is worth the complexity ...
*** Bug 348871 has been marked as a duplicate of this bug. ***
Fixed on master: https://git.gnome.org/browse/dia/commit/?id=0d83d34579414084155ab9f62f401606c6695e5d
I havn't tested it, but... awesome, thanks!