Pattern spam when adjusting a pattern in text.

[Tavmjong Bah]

When a pattern is adjusted in a text object, multiple patterns are created in a chain, each containing only a transformation and a reference to the previous pattern.

PatternKnotHolderEntityScale::knot_set() calls
  sp_item_adjust_pattern() which calls
    sp_pattern_clone_if_necessary()

      This routine looks at the hrefcount for the pattern.
      If it is greater than 1 then a new pattern is created.
      For shapes hrefcount is always 1 but for text it is 3
      or more. For each <tspan>, hrefcount is incremented
      by 2.

One problem I found here is that if a new pattern is needed, sp_repr_css_change_recursive() is called. This adds an unnecessary style attribute to tspans. Bingo, I thought. But changing this to sp_repr_css_change() didn't fix the problem. It appears that internally, a style attribute is created for each <tspan>.

The same thing happens with gradients but as the code structure is completely different for gradients they don't suffer from the same problem (there is no equivalent to sp_pattern_clone_if_necessary()).

[su_v (suv-lp)]

Reproduced with Inkscape 0.46, 0.47, 0.48 and 0.48+devel r9888 on OS X 10.5.8.

Same pattern spam happens when assigning a pattern as fill to a group (e.g of shapes) and adjusting it using the node tool.

[Adonis Papaderos (ado-papas)]

This patch copies the logic from gradients. It clones the pattern if the href count is greater than the number of descendants using this pattern.

[su_v (suv-lp)]

Patch tested with Inkscape 0.48+devel r9899 on OS X 10.5.8
(compared to 0.48.0 and 0.48+devel r9901 without patch):

While it prevents pattern definition spam (on text as well as groups), it changes how the same pattern is defined for new objects:
1) Without patch, applying a (stock) pattern that is already used in the document to a new object resets the transform parameters. With the patch, the pattern uses the last used transforms for that pattern in the document and cannot be reset to the original parameters.
2) Same happens when changing the fill attribute of a pattern-filled object to solid and back to pattern again:
Without the patch the parameters are reset (same pattern though). With the patch, the same pattern is reapplied, but with the last used pattern transforms (globally) of that specific pattern.

I don't think this side-effect was intentional - could you investigate if this can be prevented? OTOH - possibly it indicates a solution for bug #370049 “Pattern fill type setting is lost when selecting another fill type”.

Would you agree on this expected behavior?
1) pattern applied to new object: default parameters (unless style is copied from another object)
2) pattern reapplied to object (after switching fill type): reuse prior parameters (if possible)

[Adonis Papaderos (ado-papas)]

@~suv Unfortunately I cannot reproduce the behaviour you are describing without the patch. From what I understand, I can create a shape (A), assign a pattern to it and transform this pattern. After that every shape I create (B,C,...) that uses this pattern gets the transformations of (A) as default. I don't know how to reset it though. Could you give me an example (ie for dummies) I can follow in order to understand the differences you mentioned?

I am afraid that adding the feature requested in bug 370049 needs a different approach. Making Fill & Stroke remember last values of gradients and patterns is more likely a feature of the dialog.

Sure, I agree with you 100% on both points. I'll take a look to see what changes must be made to get the behaviour of point 1. As of point 2, I think that since a migration to gtkmm of Fill & Stroke is in progress, it is better to add it as a feature to the new code.

[su_v (suv-lp)]

Seems that I have walked into the trap of a bad 'steps to reproduce' I repeated in different versions to compare the result:

1) create text
2) assign pattern
3) edit pattern handles
4) create object
5) assign same pattern to object

in this case, without patch, the transforms are reset (step 5), but this doesn't hold up with normal objects (i.e. my test case was too closely tied to the pattern spam case). You are right, with regular objects or shapes (step 1 and 4), the pattern transform is never reset when reusing a pattern for new objects in the same document.

Sorry for the noise ;)

[su_v (suv-lp)]

(I used the 'Checkerboard' stock pattern for the text object in above 'steps to reproduce')

[Adonis Papaderos (ado-papas)]

This is the previous patch modified to match the behaviour described by ~suv on comment #3 point 1.

[su_v (suv-lp)]

@Adonis - thx for the second version: it seems to work fine with regard to text (and groups), and implements my earlier proposal (default pattern parameters for new objects) besides fixing the pattern definition "spam". Since it is a change to the current behavior, I'm not sure if it should be part of this fix (as mentioned I did make a mistake when I first commented your initial patch).

@Tav - did you have a chance to test both versions? What do you think about the second one (changing current behavior when assigning an already used pattern to other objects)?

[Tavmjong Bah (tavmjong-free)]

I've tested the second version on 0.48. It looks good to me. It has the behavior I would expect.

There are two small problems I see, but I think they are probably separate issues:

The first is that when you Paste Style from an object with a pattern to another object, the two <pattern> objects are copied. I would think that at least the first (where the pattern is defined) and probably the second (which scales/rotates the first pattern) would not be copied.

The second is that you can not directly edit a pattern assigned to a group. You can only edit the pattern for each object in the group one at a time.

I think it is fine to apply the patch to both 0.48 and trunk.

Thanks Adonis!

[su_v (suv-lp)]

> The second is that you can not directly edit a pattern assigned to a group.

It works with the shape tools, but not the node tool - seems the same issue as it was with text objects, see
<http://article.gmane.org/gmane.comp.graphics.inkscape.devel/35286> and
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/9868>

[Krzysztof Kosinski (tweenk)]

Adonis' patch committed in trunk 9920.

[jazzynico (jazzynico)]

Also committed in the 0.48.x branch, revision 9733.

[su_v (suv-lp)]

Proposing to keep this report open/in progress: both issues as mentioned by Tav in comment #9 are still open:
1) allow shared pattern definitions when style is pasted (referencing the same pattern href)
2) make patterns applied to a group editable with the node tool (it's odd if one needs to use a shape tool to have access to the pattern handles of a group)

[su_v (suv-lp)]

Follow-up reports filed:

Bug #686244 “Don't duplicate pattern definition with 'Paste Style'”
<https://bugs.launchpad.net/inkscape/+bug/686244>

Bug #686245 “Make pattern assigned to group editable with the node tool”
<https://bugs.launchpad.net/inkscape/+bug/686245>

[su_v (suv-lp)]

New follow-up report:
Bug #693927 “pattern-spam on pattern-edit if clone of the pattern-filled object exists”
<https://bugs.launchpad.net/inkscape/+bug/693927>