work on the lpe group undo bug. it's not solved, but i think LPE code does everything correct now. I think now it's the interplay between undo-system and LPE that bugs.

(bzr r8839)

4 files changed, 48 insertions, 33 deletions

diff --git a/src/sp-item-group.cpp b/src/sp-item-group.cpp
index 54f62e093..7ac7880a7 100644
--- a/src/sp-item-group.cpp
+++ b/src/sp-item-group.cpp
@@ -43,6 +43,7 @@
 #include "inkscape.h"
 #include "desktop-handles.h"
 #include "selection.h"
+#include "live_effects/effect.h"
 #include "live_effects/lpeobject.h"
 #include "live_effects/lpeobject-reference.h"
 #include "sp-title.h"
@@ -871,7 +872,7 @@ sp_group_perform_patheffect(SPGroup *group, SPGroup *topgroup, bool write)
         } else if (SP_IS_SHAPE(subitem)) {
             SPCurve * c = NULL;
             if (SP_IS_PATH(subitem)) {
-                c = sp_path_get_curve_for_edit(SP_PATH(subitem));
+                c = sp_path_get_original_curve(SP_PATH(subitem));
             } else {
                 c = sp_shape_get_curve(SP_SHAPE(subitem));
             }
@@ -884,6 +885,9 @@ sp_group_perform_patheffect(SPGroup *group, SPGroup *topgroup, bool write)
                     Inkscape::XML::Node *repr = SP_OBJECT_REPR(subitem);
                     gchar *str = sp_svg_write_path(c->get_pathvector());
                     repr->setAttribute("d", str);
+#ifdef GROUP_VERBOSE
+g_message("sp_group_perform_patheffect writes 'd' attribute");
+#endif
                     g_free(str);
                 }
 
diff --git a/src/sp-lpe-item.cpp b/src/sp-lpe-item.cpp
index 49264c684..6b71541e6 100644
--- a/src/sp-lpe-item.cpp
+++ b/src/sp-lpe-item.cpp
@@ -56,7 +56,6 @@ static void sp_lpe_item_remove_child (SPObject * object, Inkscape::XML::Node * c
 
 static void sp_lpe_item_enable_path_effects(SPLPEItem *lpeitem, bool enable);
 
-static void lpeobject_ref_changed(SPObject *old_ref, SPObject *ref, SPLPEItem *lpeitem);
 static void lpeobject_ref_modified(SPObject *href, guint flags, SPLPEItem *lpeitem);
 
 static void sp_lpe_item_create_original_path_recursive(SPLPEItem *lpeitem);
@@ -120,7 +119,7 @@ sp_lpe_item_init(SPLPEItem *lpeitem)
     lpeitem->path_effect_list = new PathEffectList();
     lpeitem->current_path_effect = NULL;
 
-    new (&lpeitem->lpe_modified_connection) sigc::connection();
+    lpeitem->lpe_modified_connection_list = new std::list<sigc::connection>();
 }
 
 static void
@@ -154,8 +153,14 @@ sp_lpe_item_release(SPObject *object)
 {
     SPLPEItem *lpeitem = (SPLPEItem *) object;
 
-    lpeitem->lpe_modified_connection.disconnect();
-    lpeitem->lpe_modified_connection.~connection();
+    // disconnect all modified listeners:
+    for (std::list<sigc::connection>::iterator mod_it = lpeitem->lpe_modified_connection_list->begin();
+         mod_it != lpeitem->lpe_modified_connection_list->end(); ++mod_it)
+    {
+        mod_it->disconnect();
+    }
+    delete lpeitem->lpe_modified_connection_list;
+    lpeitem->lpe_modified_connection_list = NULL;
 
     PathEffectList::iterator it = lpeitem->path_effect_list->begin();
     while ( it != lpeitem->path_effect_list->end() ) {
@@ -165,7 +170,8 @@ sp_lpe_item_release(SPObject *object)
         it = lpeitem->path_effect_list->erase(it);
     }
     // delete the list itself
-    delete SP_LPE_ITEM(object)->path_effect_list;
+    delete lpeitem->path_effect_list;
+    lpeitem->path_effect_list = NULL;
 
     if (((SPObjectClass *) parent_class)->release)
         ((SPObjectClass *) parent_class)->release(object);
@@ -187,6 +193,14 @@ sp_lpe_item_set(SPObject *object, unsigned int key, gchar const *value)
                 // Disable the path effects while populating the LPE list
                  sp_lpe_item_enable_path_effects(lpeitem, false);
 
+                // disconnect all modified listeners:
+                for ( std::list<sigc::connection>::iterator mod_it = lpeitem->lpe_modified_connection_list->begin();
+                      mod_it != lpeitem->lpe_modified_connection_list->end();
+                      ++mod_it)
+                {
+                    mod_it->disconnect();
+                }
+                lpeitem->lpe_modified_connection_list->clear();
                 // Clear the path effect list
                 PathEffectList::iterator it = lpeitem->path_effect_list->begin();
                 while ( it != lpeitem->path_effect_list->end() )
@@ -202,10 +216,7 @@ sp_lpe_item_set(SPObject *object, unsigned int key, gchar const *value)
                     std::string href;
                     while (std::getline(iss, href, ';'))
                     {
-                        Inkscape::LivePathEffect::LPEObjectReference *path_effect_ref = new Inkscape::LivePathEffect::LPEObjectReference(SP_OBJECT(lpeitem));
-                        path_effect_ref->changedSignal().connect(sigc::bind(sigc::ptr_fun(lpeobject_ref_changed), SP_LPE_ITEM(object)));
-                        // Now do the attaching, which emits the changed signal.
-                        // Fixme, it should not do this changed signal and updating before all effects are added to the path_effect_list
+                        Inkscape::LivePathEffect::LPEObjectReference *path_effect_ref = new Inkscape::LivePathEffect::LPEObjectReference(object);
                         try {
                             path_effect_ref->link(href.c_str());
                         } catch (Inkscape::BadURIException e) {
@@ -216,7 +227,11 @@ sp_lpe_item_set(SPObject *object, unsigned int key, gchar const *value)
                         }
 
                         lpeitem->path_effect_list->push_back(path_effect_ref);
-                        if ( !(path_effect_ref->lpeobject && path_effect_ref->lpeobject->get_lpe()) ) {
+                        if ( path_effect_ref->lpeobject && path_effect_ref->lpeobject->get_lpe() ) {
+                            // connect modified-listener
+                            lpeitem->lpe_modified_connection_list->push_back(
+                                                path_effect_ref->lpeobject->connectModified(sigc::bind(sigc::ptr_fun(&lpeobject_ref_modified), lpeitem)) );
+                        } else {
                             // something has gone wrong in finding the right patheffect.
                             g_warning("Unknown LPE type specified, LPE stack effectively disabled");
                             // keep the effect in the lpestack, so the whole stack is effectively disabled but maintained
@@ -407,28 +422,14 @@ sp_lpe_item_update_patheffect (SPLPEItem *lpeitem, bool wholetree, bool write)
 }
 
 /**
- * Gets called when (re)attached to another lpeobject.
- */
-static void
-lpeobject_ref_changed(SPObject *old_ref, SPObject *ref, SPLPEItem *lpeitem)
-{
-    if (old_ref) {
-        sp_signal_disconnect_by_data(old_ref, lpeitem);
-    }
-    if ( IS_LIVEPATHEFFECT(ref) && ref != lpeitem )
-    {
-        lpeitem->lpe_modified_connection.disconnect();
-        lpeitem->lpe_modified_connection = ref->connectModified(sigc::bind(sigc::ptr_fun(&lpeobject_ref_modified), lpeitem));
-        lpeobject_ref_modified(ref, 0, lpeitem);
-    }
-}
-
-/**
- * Gets called when lpeobject repr contents change: i.e. parameter change.
+ * Gets called when any of the lpestack's lpeobject repr contents change: i.e. parameter change in any of the stacked LPEs
  */
 static void
 lpeobject_ref_modified(SPObject */*href*/, guint /*flags*/, SPLPEItem *lpeitem)
 {
+#ifdef SHAPE_VERBOSE
+    g_message("lpeobject_ref_modified");
+#endif
     sp_lpe_item_update_patheffect (lpeitem, true, true);
 }
 
@@ -753,8 +754,9 @@ PathEffectList sp_lpe_item_get_effect_list(SPLPEItem *lpeitem)
 
 Inkscape::LivePathEffect::LPEObjectReference* sp_lpe_item_get_current_lpereference(SPLPEItem *lpeitem)
 {
-    if (!lpeitem->current_path_effect && !lpeitem->path_effect_list->empty())
+    if (!lpeitem->current_path_effect && !lpeitem->path_effect_list->empty()) {
         sp_lpe_item_set_current_path_effect(lpeitem, lpeitem->path_effect_list->back());
+    }
 
     return lpeitem->current_path_effect;
 }
@@ -773,7 +775,6 @@ bool sp_lpe_item_set_current_path_effect(SPLPEItem *lpeitem, Inkscape::LivePathE
 {
     for (PathEffectList::iterator it = lpeitem->path_effect_list->begin(); it != lpeitem->path_effect_list->end(); it++) {
         if ((*it)->lpeobject_repr == lperef->lpeobject_repr) {
-            lpeobject_ref_changed(NULL, (*it)->lpeobject, SP_LPE_ITEM(lpeitem)); // FIXME: explain why this is here?
             lpeitem->current_path_effect = (*it);  // current_path_effect should always be a pointer from the path_effect_list !
             return true;
         }
diff --git a/src/sp-lpe-item.h b/src/sp-lpe-item.h
index 5b6cc241e..4823390de 100644
--- a/src/sp-lpe-item.h
+++ b/src/sp-lpe-item.h
@@ -39,10 +39,10 @@ struct SPLPEItem : public SPItem {
     int path_effects_enabled;
 
     PathEffectList* path_effect_list;
+    std::list<sigc::connection> *lpe_modified_connection_list; // this list contains the connections for listening to lpeobject parameter changes
+
     Inkscape::LivePathEffect::LPEObjectReference* current_path_effect;
     std::vector<Inkscape::Display::TemporaryItem*> lpe_helperpaths;
-
-    sigc::connection lpe_modified_connection;
 };
 
 struct SPLPEItemClass {
diff --git a/src/sp-path.cpp b/src/sp-path.cpp
index a863c12b4..2120ddd64 100644
--- a/src/sp-path.cpp
+++ b/src/sp-path.cpp
@@ -320,6 +320,9 @@ sp_path_write(SPObject *object, Inkscape::XML::Document *xml_doc, Inkscape::XML:
         repr = xml_doc->createElement("svg:path");
     }
 
+#ifdef PATH_VERBOSE
+g_message("sp_path_write writes 'd' attribute");
+#endif
     if ( shape->curve != NULL ) {
         gchar *str = sp_svg_write_path(shape->curve->get_pathvector());
         repr->setAttribute("d", str);
@@ -411,6 +414,10 @@ sp_path_update_patheffect(SPLPEItem *lpeitem, bool write)
     SPPath * const path = (SPPath *) lpeitem;
     Inkscape::XML::Node *repr = SP_OBJECT_REPR(shape);
 
+#ifdef PATH_VERBOSE
+g_message("sp_path_update_patheffect");
+#endif
+
     if (path->original_curve && sp_lpe_item_has_path_effect_recursive(lpeitem)) {
         SPCurve *curve = path->original_curve->copy();
         /* if a path does not have an lpeitem applied, then reset the curve to the original_curve.
@@ -420,6 +427,9 @@ sp_path_update_patheffect(SPLPEItem *lpeitem, bool write)
         bool success = sp_lpe_item_perform_path_effect(SP_LPE_ITEM(shape), curve);
         if (success && write) {
             // could also do SP_OBJECT(shape)->updateRepr();  but only the d attribute needs updating.
+#ifdef PATH_VERBOSE
+g_message("sp_path_update_patheffect writes 'd' attribute");
+#endif
             if ( shape->curve != NULL ) {
                 gchar *str = sp_svg_write_path(shape->curve->get_pathvector());
                 repr->setAttribute("d", str);