trunk: swatches are duplicated again on paste and import (rev >= 13139)

[su_v]

It should be possible to re-use the same swatches
- within the same document on copy & paste,
- across multiple documents on paste or import,
without them getting duplicated on import/paste.

1 -- changed behavior with rev >= 13139:

Steps to reproduce with multiple documents:
1) launch current trunk (default (new) prefs, default doc)
2) draw a rectangle (blue fill, black stroke)
3) open 'Fill & Stroke', convert to swatch
4) give the blue swatch a unique name
5) save as 'myswatch-orig.svg'
6) draw a circle (red fill, black stroke)
7) open 'View > Swatches' and apply the blue swatch as fill to the circle
8) delete the rectangle
9) save as 'myswatch-copy.svg'

a) copy & paste across documents:
10) open 'myswatch-orig.svg'
11) open 'myswatch-copy.svg'
12) copy the blue circle in 'myswatch-copy.svg'
13) paste the clipboard content in 'myswatch-orig.svg'
14) open 'Fill &Stroke' and check how many swatches are defined and in use

b) import document:
10) open 'myswatch-orig.svg'
11) import 'myswatch-copy.svg'
12) open 'Fill &Stroke' and check how many swatches are defined and in use

Expected result:
The pasted (or imported) object(s) with the same swatch (name) as already exists in the current document shares the existing definition.

Actual result:
The swatch of pasted (or imported) object(s) is added as a copy of the already existing swatch with the same name.

2 -- Additional changes with rev >= 13145:

Steps to reproduce (basic):
1) launch current trunk (default (new) prefs, default doc)
2) draw a rectangle (blue fill, black stroke)
3) open 'Fill & Stroke', convert to swatch
5) copy & paste the rectangle
6) open 'Fill &Stroke' and check how many swatches are defined and in use

Expected: 1 swatch, used 2x
Actual: 2 swatches, each used 1x

If multiple objects with the same swatch are pasted from the clipboard into a document which already has the same swatch definition as used by the objects on the clipboard, then for each inserted copy a new swatch is created (rev > 13145), though it is not referenced by any object.

Based on tests with archived builds on OS X 10.7.5
- not reproduced with rev <=13138,
- reproduced with rev >= 13139;
this change of behavior is related to the changes from revision 13139 and 13145:
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/13139>
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/13145>

[David Mathog (mathog)]

This patch works on my system.

The bigger issue is that Swatches are kind of strange. They seem to be a type of gradient with no stops. I don't know if that is generally true, but it was true for the test case. If it is not generally true this patch will not work in those cases. Moreover, while there are "what is this?" tests like this:

SP_IS_LINEARGRADIENT(this)
SP_IS_RADIALGRADIENT(this)
SP_IS_MESHGRADIENT(this)

for other types of gradients there is no

SP_IS_SWATCH(this)

so the patch tests for gradients that have no stops and if it sees that assumes they are swatches. For all I know the code might use a radial gradient with no stops for something different than a lineargradient with no stops, which will break this section of code again.

[David Mathog (mathog)]

On further consideration, the patch does not test for color, so it cannot be correct. Do not apply, I will fix it.

[David Mathog (mathog)]

This one takes color (via paintserver) into account. It correctly copies two rectangles with different solid colors without duplicating a swatch or assigning the wrong color.

There is a test for swath, but the syntax is not like the others,it is "isSwatch()".

[David Mathog (mathog)]

The preceding patch was too simple. The more complicated patch crashes inkscape. I'm going to post it so that maybe somebody else can see what is wrong. DO NOT COMMIT IT!

The previous patch, changes_2014_03_28b_swatchonly.patch, failed to test the color beyond comparing the vectors. That was wrong, two swatches could have different vectors and the same color. So sometimes copy/paste a swatch would come up with the wrong color. The attached patch checks the color and reports correctly when they are the same. It works correctly for this test:

inkscape
open swatch1.svg
import swatch2.svg

However, inkscape crashes for this test
inkscape
open swatch1.svg
open swatch2.svg
(in swatch2, select the red rectangle, copy)
(in swatch1, paste)

It correctly changes the target from "red" to "redblue" in change_def_reference() (in id-clash.cpp), as far as gdb or print statements are concerrned, but it then goes on to crash at a call to isSwatch() in sp-paint-server.cpp, and it does that because it is called like this

        if (server && SP_IS_GRADIENT(server) && SP_GRADIENT(server)->getVector()->isSwatch()) {

by

    SPPaintSelector::Mode SPPaintSelector::getModeForStyle(SPStyle const & style, FillOrStroke kind)

(in paint-selector.cpp)

The vector is NULL at that point. It seems that the change to use the existing swatch, which is in change_def_reference() here:

                gchar *new_uri = g_strdup_printf("#%s", to_obj->getId());
                it->elem->getRepr()->setAttribute(it->attr, new_uri);
                g_free(new_uri);

does not make it into the vector pointer. It might not even be all the way into the XML, judging from the one crash save that was recovered (see below). I think this might be happening because in files.cpp the cut/paste code calls SPDocument::importDefs() apparently _before_ it gets the rest of the objects. The import (file) mode loads the entire imported file at once, so it has the entire SVG to work with.

I was able to get a saved crash with:

inkscape
open swatch1.svg
import swatch1.svg
open swatch2.svg
(cut and paste as before)

and it shows in the saved file that "red" was _not_ changed to "redblue", which is odd, because running in the debugger that change was made. This might also have something to do with this change having been made with setAttibute(), because it is an href, whereas the other similar changes, for patterns and such, go instead through sp_style_set_property_url().

[David Mathog (mathog)]

This one retained a link to "red" when only "redblue" was present in defs.

[David Mathog (mathog)]

I think maybe I found the problem. No time to test thoroughly now, but dragging this:

    // copy definitions
    desktop->doc()->importDefs(clipdoc);

in sp_import_document in file.cpp

to below

        pasted_objects = g_slist_prepend(pasted_objects, (gpointer) obj_copy);
    }

in the same function seems to have fixed the crash. With the swatch1 and swatch2 test files a few minutes of testing didn't turn up a glitch in copy paste from window to window or on import.

[David Mathog (mathog)]

Well, this is a bit of a compromise, but at least it seems to mostly work and it doesn't crash.

Tests with swatch1.svg importing itself or cut/paste its own contents from one window to another do not duplicate swatches. This tests what happens when swatches with the same name are combined into one document from two sources.

Tests with swatch1.svg and swatch2.svg, where each has a swatch with the same properties, but with different names, results in all both sets of swatches being present after import or paste. This tests what happens when swatches with the same properties, but different names, are combined into one document from two sources.

In the second set of tests it was possible to detect that, for instance, the "blue" and "blueblack" swatches had exactly the same properties, and when this was detected in SPDocument::importDefs the next line:

                        change_def_references(src, trg);

appeared to be changing the references, but later on a swatch would be referenced with a NULL vector and the program would crash. I could not find a satisfactory fix for that. Moving importDefs after the object import in file_import() seemed to resolve this issue, but it broke gradient reuse.

So for now, the names of the swatches are compared, and they are only considered to be equivalent if the names are exact matches.

The current patch has TODO comments about this exact match/ different name issue.

[David Mathog (mathog)]

changes were merged into those for #1318657 and committed as revision 13417.