OK, I think I understand now why removing "count++" was wrong in this commit:

http://cvs.sourceforge.net/viewcvs.py/inkscape/inkscape/src/sp-object.cpp?r1...

and why I had to restore it, having run into a bug. I think it might be interesting to others too, so I'm copying to the list. (Mental: one question/request for you is at the bottom.)

Since count is a global static variable, before that change it was incremented on every object added to any document, no matter was there a conflict or not, including adding object on document loading. As a result, at each point of time, count was no less than the total number of element nodes in all loaded documents.

After pjrm's change, count was incremented only for conflicting ids, i.e. very unfrequently. This produced nice small ids and worked fine, but only so long as you are within a single document. The badness of this approach was only evident when you copied things across documents. For example, imagine that you copy a group which has three objects, two of which use the same gradient and the third uses some other gradient. Then the defs_clipboard will contain three objects with ids, for example.

gradient100 gradient100 gradient56

The second one is a copy of the first, but when it will be added on pasting to a different document, it will be re-ided. By itself this does not matter because the first copy of gradient100 will still be there and will be used, and the re-ided second copy will just be unused. However, since after pjrm's change count is small, and since the second document (where I'm pasting) may have few objects yet, the generated id for the second copy of gradient100 will have a small number in the new id. For example, 56. So when it's time to insert the gradient56 from the clipboard, it will clash and be re-ided too - now that's a big problem because the object that uses it will now refer to a wrong gradient.

Sure, making count big enough does not prevent problems still. Clashes will occur until we reimplement the clipboard properly, with not only re-iding but also updating references to re-ided objects. But until then, the original approach which increments count on each added object is much safer, because it starts finding a new id skipping at least as many numbers as there are objects in _all_ loaded documents.

(By the way, Peter, probably your idea in this change was to make ids shorter, but you didn't go far enough in that. You could just as well remove the global count altogether and start counting from 0 each time, until there's no conflict in the current document. This would have approximately the same effect as your change, but the code would become simpler. However, the entire idea behind the global count is to prevent not only the real intra-document conflicts, but also the potential cross-document conflicts, and from this viewpoint it would be still safer to do not only count++ on each added object, but even count+=99 or something like that.)

You may ask, why does clipboard pastes two copies of the same gradient? This is a result of another change made by Mental. Pasting defs_clipboard has this check:

SPObject *exists = doc->getObjectByRepr(repr); if (!exists){ SPRepr *copy = sp_repr_duplicate(repr); sp_repr_add_child(SP_OBJECT_REPR(defs), copy, NULL); sp_repr_unref(copy); }

In 0.40 and before, this worked, i.e. after the first gradient100 was added, the second one would have exists != NULL and will be skipped. This was before getObjectByRepr was implemented via getObjectById and therefore only compared ids. Now it is rewritten differently and this check no longer works (i.e. exists is always NULL). Mental: I'm not sure why (or if) the new behavior is better, I just want to point out that it's different from the old one, and therefore there's a danger that it broke something else as well. Should we restore the old behavior, if only for backwards compatibility?