Merge lp:~ado-papas/inkscape/bug_167419 into lp:~inkscape.dev/inkscape/trunk

[Adonis Papaderos]

Fix some issues with linked offsets. Changed the rendering of the offset to take into account the transformation of the source object, as is the case with the use element, and copied the move compensation logic from uses to offsets. There is a speed penalty though because of this, since I cannot find a reliable method to predict whether there is a change to the source object data or a transformation applied to it.

The second patch removes an optimization I implemented using the transform event, since this event is not emitted on undo and redo actions.

[Adonis Papaderos (ado-papas)]

Forced recalculation of shape after transform event.
Removed usage of the SP_OBJECT_REPR macro.

review: Needs Resubmitting

[su_v (suv-lp)]

see latest comments in bug #167419
<https://bugs.launchpad.net/inkscape/+bug/167419>

review: Approve

[su_v (suv-lp)]

The diff from this branch fails to apply to current trunk r9951 (after the GSoC C++-ificiation merge and cleanup in r9946).

I do not know if (and how) the changes (r9945..r9951) affect the proposed code itself or if 'patch' just can't determine the correct position of the changed lines within the modified files.

review: Needs Fixing

[Alexandre Prokoudine (alexandre-prokoudine)]

Adonis, do you think you could find some time to update your patch? It would be most appreciated. I'm using linked offsets all the time and some bugs really drive me nuts :)

[jazzynico (jazzynico)]

The current diff (updated 2011-03-10) now applies correctly to the trunk.

[jazzynico (jazzynico)]

Last diff tested successfully on the trunk. All linked bugs seem to be fixed.

review: Approve

=== modified file 'src/selection-chemistry.cpp'
--- src/selection-chemistry.cpp	2011-02-21 07:59:34 +0000
+++ src/selection-chemistry.cpp	2011-03-10 18:26:15 +0000
@@ -1379,7 +1379,7 @@
          * Same for textpath if we are also doing ANY transform to its path: do not touch textpath,
          * letters cannot be squeezed or rotated anyway, they only refill the changed path.
          * Same for linked offset if we are also moving its source: do not move it. */
-        if (transform_textpath_with_path || transform_offset_with_source) {
+        if (transform_textpath_with_path) {
             // Restore item->transform field from the repr, in case it was changed by seltrans.
             item->readAttr( "transform" );
         } else if (transform_flowtext_with_frame) {
@@ -1394,7 +1394,7 @@
                     }
                 }
             }
-        } else if (transform_clone_with_original) {
+        } else if (transform_clone_with_original || transform_offset_with_source) {
             // We are transforming a clone along with its original. The below matrix juggling is
             // necessary to ensure that they transform as a whole, i.e. the clone's induced
             // transform and its move compensation are both cancelled out.
@@ -1408,7 +1408,7 @@
             Geom::Affine t_inv = t.inverse();
             Geom::Affine result = t_inv * item->transform * t;
 
-            if ((prefs_parallel || prefs_unmoved) && affine.isTranslation()) {
+            if (transform_clone_with_original && (prefs_parallel || prefs_unmoved) && affine.isTranslation()) {
                 // we need to cancel out the move compensation, too
 
                 // find out the clone move, same as in sp_use_move_compensate
@@ -1426,6 +1426,19 @@
                     item->doWriteTransform(item->getRepr(), move, &t, compensate);
                 }
 
+            } else if (transform_offset_with_source && (prefs_parallel || prefs_unmoved) && affine.isTranslation()){
+                Geom::Affine parent = item->transform;
+                Geom::Affine offset_move = parent.inverse() * t * parent;
+
+                if (prefs_parallel) {
+                    Geom::Affine move = result * offset_move * t_inv;
+                    item->doWriteTransform(item->getRepr(), move, &move, compensate);
+
+                } else if (prefs_unmoved) {
+                    Geom::Affine move = result * offset_move;
+                    item->doWriteTransform(item->getRepr(), move, &t, compensate);
+                }
+
             } else {
                 // just apply the result
                 item->doWriteTransform(item->getRepr(), result, &t, compensate);

=== modified file 'src/sp-offset.cpp'
--- src/sp-offset.cpp	2011-02-22 09:17:44 +0000
+++ src/sp-offset.cpp	2011-03-10 18:26:15 +0000
@@ -1029,30 +1029,37 @@
 {
     Inkscape::Preferences *prefs = Inkscape::Preferences::get();
     guint mode = prefs->getInt("/options/clonecompensation/value", SP_CLONE_COMPENSATION_PARALLEL);
-    if (mode == SP_CLONE_COMPENSATION_NONE) return;
+
+    SPItem *item = SP_ITEM(self);
 
     Geom::Affine m(*mp);
-    if (!(m.isTranslation())) return;
+    if (!(m.isTranslation()) || mode == SP_CLONE_COMPENSATION_NONE) {
+        self->sourceDirty=true;
+        item->requestDisplayUpdate(SP_OBJECT_MODIFIED_FLAG);
+        return;
+    }
 
     // calculate the compensation matrix and the advertized movement matrix
-    SPItem *item = SP_ITEM(self);
-
-    Geom::Affine compensate;
+    item->readAttr("transform");
+
+    Geom::Affine t = self->transform;
+    Geom::Affine offset_move = t.inverse() * m * t;
+
     Geom::Affine advertized_move;
-
-    if (mode == SP_CLONE_COMPENSATION_UNMOVED) {
-        compensate = Geom::identity();
+    if (mode == SP_CLONE_COMPENSATION_PARALLEL) {
+        offset_move = offset_move.inverse() * m;
+        advertized_move = m;
+    } else if (mode == SP_CLONE_COMPENSATION_UNMOVED) {
+        offset_move = offset_move.inverse();
         advertized_move.setIdentity();
-    } else if (mode == SP_CLONE_COMPENSATION_PARALLEL) {
-        compensate = m;
-        advertized_move = m;
     } else {
         g_assert_not_reached();
     }
 
-    item->transform *= compensate;
+    self->sourceDirty=true;
 
     // commit the compensation
+    item->transform *= offset_move;
     item->doWriteTransform(item->getRepr(), item->transform, &advertized_move);
     item->requestDisplayUpdate(SP_OBJECT_MODIFIED_FLAG);
 }
@@ -1075,12 +1082,13 @@
 }
 
 static void
-sp_offset_source_modified (SPObject */*iSource*/, guint /*flags*/, SPItem *item)
+sp_offset_source_modified (SPObject */*iSource*/, guint flags, SPItem *item)
 {
     SPOffset *offset = SP_OFFSET(item);
     offset->sourceDirty=true;
-    refresh_offset_source(offset);
-    ((SPShape *) offset)->setShape ();
+    if (flags & (SP_OBJECT_MODIFIED_FLAG | SP_OBJECT_CHILD_MODIFIED_FLAG)) {
+        offset->requestDisplayUpdate(SP_OBJECT_MODIFIED_FLAG);
+    }
 }
 
 static void
@@ -1111,6 +1119,15 @@
     orig->LoadPathVector(curve->get_pathvector());
     curve->unref();
 
+    if (!item->transform.isIdentity()) {
+        gchar const *t_attr = item->getRepr()->attribute("transform");
+        if (t_attr) {
+            Geom::Affine t;
+            if (sp_svg_transform_read(t_attr, &t)) {
+                orig->Transform(t);
+            }
+        }
+    }
 
     // Finish up.
     {

=== modified file 'src/splivarot.cpp'
--- src/splivarot.cpp	2011-02-22 00:01:57 +0000
+++ src/splivarot.cpp	2011-03-10 18:26:15 +0000
@@ -1468,6 +1468,7 @@
         if ( updating ) {
 
 			//XML Tree being used directly here while it shouldn't be
+            item->doWriteTransform(item->getRepr(), transform);
             char const *id = item->getRepr()->attribute("id");
             char const *uri = g_strdup_printf("#%s", id);
             repr->setAttribute("xlink:href", uri);
@@ -1486,11 +1487,7 @@
 
         SPItem *nitem = (SPItem *) sp_desktop_document(desktop)->getObjectByRepr(repr);
 
-        if ( updating ) {
-            // on conserve l'original
-            // we reapply the transform to the original (offset will feel it)
-            item->doWriteTransform(item->getRepr(), transform);
-        } else {
+        if ( !updating ) {
             // delete original, apply the transform to the offset
             item->deleteObject(false);
             nitem->doWriteTransform(repr, transform);