Enable simultaneous knotholder and nodepath

(bzr r6291)

7 files changed, 185 insertions, 102 deletions

diff --git a/src/knot-holder-entity.cpp b/src/knot-holder-entity.cpp
index a587a3af0..e2ae5f10e 100644
--- a/src/knot-holder-entity.cpp
+++ b/src/knot-holder-entity.cpp
@@ -56,7 +56,13 @@ KnotHolderEntity::create(SPDesktop *desktop, SPItem *item, KnotHolder *parent, c
 KnotHolderEntity::~KnotHolderEntity()
 {
     /* unref should call destroy */
-    g_object_unref(knot);
+    if (knot) {
+        g_object_unref(knot);
+    } else {
+        // FIXME: This shouldn't occur. Perhaps it is caused by LPE PointParams being knotholder entities, too
+        //        If so, it will likely be fixed with upcoming refactoring efforts.
+        g_return_if_fail(knot);
+    }
 }
 
 void
diff --git a/src/live_effects/parameter/path.cpp b/src/live_effects/parameter/path.cpp
index 0f6a82971..1e470bb14 100644
--- a/src/live_effects/parameter/path.cpp
+++ b/src/live_effects/parameter/path.cpp
@@ -204,7 +204,7 @@ PathParam::param_editOncanvas(SPItem * item, SPDesktop * dt)
         shape_editor->set_item_lpe_path_parameter(item, SP_OBJECT(param_effect->getLPEObj()), param_key.c_str());
     } else {
         // set referred item for editing
-        shape_editor->set_item(ref.getObject());
+        shape_editor->set_item(ref.getObject(), SH_NODEPATH);
     }
 }
 
diff --git a/src/node-context.cpp b/src/node-context.cpp
index 9a4bff88e..a72501af7 100644
--- a/src/node-context.cpp
+++ b/src/node-context.cpp
@@ -162,7 +162,8 @@ sp_node_context_setup(SPEventContext *ec)
     nc->current_state = SP_NODE_CONTEXT_INACTIVE;
 
     if (item) {
-        nc->shape_editor->set_item(item);
+        nc->shape_editor->set_item(item, SH_NODEPATH);
+        nc->shape_editor->set_item(item, SH_KNOTHOLDER);
     }
 
     if (prefs_get_int_attribute("tools.nodes", "selcue", 0) != 0) {
@@ -211,9 +212,11 @@ sp_node_context_selection_changed(Inkscape::Selection *selection, gpointer data)
     SPNodeContext *nc = SP_NODE_CONTEXT(data);
 
     // TODO: update ShapeEditorsCollective instead
-    nc->shape_editor->unset_item();
+    nc->shape_editor->unset_item(SH_NODEPATH);
+    nc->shape_editor->unset_item(SH_KNOTHOLDER);
     SPItem *item = selection->singleItem(); 
-    nc->shape_editor->set_item(item);
+    nc->shape_editor->set_item(item, SH_NODEPATH);
+    nc->shape_editor->set_item(item, SH_KNOTHOLDER);
     nc->shape_editor->update_statusbar();
 }
 
diff --git a/src/nodepath.cpp b/src/nodepath.cpp
index 02e700ac1..3f9754bbb 100644
--- a/src/nodepath.cpp
+++ b/src/nodepath.cpp
@@ -627,6 +627,10 @@ static void update_object(Inkscape::NodePath::Path *np)
         sp_canvas_bpath_set_bpath(SP_CANVAS_BPATH(np->helper_path), helper_curve);
         helper_curve->unref();
     }
+
+    // now that nodepath and knotholder can be enabled simultaneously, we must update the knotholder, too
+    // TODO: this should be done from ShapeEditor!! nodepath should be oblivious of knotholder!
+    np->shape_editor->update_knotholder();
 }
 
 /**
diff --git a/src/selection-chemistry.cpp b/src/selection-chemistry.cpp
index 9695d58e9..eebec434d 100644
--- a/src/selection-chemistry.cpp
+++ b/src/selection-chemistry.cpp
@@ -1683,7 +1683,9 @@ void sp_selection_edit_clip_or_mask(SPDesktop * dt, bool clip)
                         }
 
                         ShapeEditor * shape_editor = SP_NODE_CONTEXT( dt->event_context )->shape_editor;
-                        shape_editor->set_item(SP_ITEM(child));
+                        // TODO: should we set the item for nodepath or knotholder or both? seems to work with both.
+                        shape_editor->set_item(SP_ITEM(child), SH_NODEPATH);
+                        shape_editor->set_item(SP_ITEM(child), SH_KNOTHOLDER);
                         Inkscape::NodePath::Path *np = shape_editor->get_nodepath();
                         if (np) {
                             // take colors from prefs (same as used in outline mode)
diff --git a/src/shape-editor.cpp b/src/shape-editor.cpp
index 91909d850..77419d1d1 100644
--- a/src/shape-editor.cpp
+++ b/src/shape-editor.cpp
@@ -55,35 +55,37 @@ ShapeEditor::ShapeEditor(SPDesktop *dt) {
 }
 
 ShapeEditor::~ShapeEditor() {
-    unset_item();
+    unset_item(SH_KNOTHOLDER);
+    unset_item(SH_NODEPATH);
 }
 
-void ShapeEditor::unset_item() {
-
+void ShapeEditor::unset_item(SubType type, bool keep_knotholder) {
     Inkscape::XML::Node *old_repr = NULL;
 
-    if (this->nodepath) {
-        old_repr = this->nodepath->repr;
-    }
-
-    if (!old_repr && this->knotholder) {
-        old_repr = this->knotholder->repr;
-    }
-
-    if (old_repr) { // remove old listener
-        sp_repr_remove_listener_by_data(old_repr, this);
-        Inkscape::GC::release(old_repr);
-    }
-
-    if (this->nodepath) {
-        this->grab_node = -1;
-        sp_nodepath_destroy(this->nodepath);
-        this->nodepath = NULL;
-    }
+    switch (type) {
+        case SH_NODEPATH:
+            if (this->nodepath) {
+                old_repr = this->nodepath->repr;
+                sp_repr_remove_listener_by_data(old_repr, this);
+                Inkscape::GC::release(old_repr);
 
-    if (this->knotholder) {
-        delete this->knotholder;
-        this->knotholder = NULL;
+                this->grab_node = -1;
+                sp_nodepath_destroy(this->nodepath);
+                this->nodepath = NULL;
+            }
+            break;
+        case SH_KNOTHOLDER:
+            if (this->knotholder) {
+                old_repr = this->knotholder->repr;
+                sp_repr_remove_listener_by_data(old_repr, this);
+                Inkscape::GC::release(old_repr);
+
+                if (!keep_knotholder) {
+                    delete this->knotholder;
+                    this->knotholder = NULL;
+                }
+            }
+            break;
     }
 }
 
@@ -95,25 +97,52 @@ bool ShapeEditor::has_knotholder () {
     return (this->knotholder != NULL);
 }
 
-bool ShapeEditor::has_local_change () {
-    return ((this->nodepath && this->nodepath->local_change) ||
-            (this->knotholder && this->knotholder->local_change != 0));
+void ShapeEditor::update_knotholder () {
+    if (this->knotholder)
+        this->knotholder->update_knots();
 }
 
-void ShapeEditor::decrement_local_change () {
-    if (this->nodepath && this->nodepath->local_change > 0) {
-        this->nodepath->local_change--;
-    } else if (this->knotholder) {
-        this->knotholder->local_change = FALSE;
+bool ShapeEditor::has_local_change (SubType type) {
+    switch (type) {
+        case SH_NODEPATH:
+            return (this->nodepath && this->nodepath->local_change);
+        case SH_KNOTHOLDER:
+            return (this->knotholder && this->knotholder->local_change != 0);
+        default:
+            g_assert_not_reached();
+    }
+}
+
+void ShapeEditor::decrement_local_change (SubType type) {
+    switch (type) {
+        case SH_NODEPATH:
+            if (this->nodepath && this->nodepath->local_change > 0) {
+                this->nodepath->local_change--;
+            }
+            break;
+        case SH_KNOTHOLDER:
+            if (this->knotholder) {
+                this->knotholder->local_change = FALSE;
+            }
+            break;
+        default:
+            g_assert_not_reached();
     }
 }
 
-SPItem *ShapeEditor::get_item () {
+SPItem *ShapeEditor::get_item (SubType type) {
     SPItem *item = NULL;
-    if (this->has_nodepath()) {
-        item = this->nodepath->item;
-    } else if (this->has_knotholder()) {
-        item = SP_ITEM(this->knotholder->item);
+    switch (type) {
+        case SH_NODEPATH:
+            if (this->has_nodepath()) {
+                item = this->nodepath->item;
+            }
+            break;
+        case SH_KNOTHOLDER:
+            if (this->has_knotholder()) {
+                item = this->nodepath->item;
+            }
+            break;
     }
     return item;
 }
@@ -141,24 +170,26 @@ static void shapeeditor_event_attr_changed(Inkscape::XML::Node */*repr*/, gchar
                                            gchar const */*old_value*/, gchar const */*new_value*/,
                                            bool /*is_interactive*/, gpointer data)
 {
-    gboolean changed = FALSE;
+    gboolean changed_np = FALSE;
+    gboolean changed_kh = FALSE;
 
     g_assert(data);
     ShapeEditor *sh = ((ShapeEditor *) data);
 
-    if ( sh->has_knotholder() || ( sh->has_nodepath() && sh->nodepath_edits_repr_key(name) ) )
+    if (sh->has_nodepath() && sh->nodepath_edits_repr_key(name))
     {
-        changed = !sh->has_local_change();
-        sh->decrement_local_change();
+        changed_np = !sh->has_local_change(SH_NODEPATH);
+        sh->decrement_local_change(SH_NODEPATH);
+
     }
 
-    if (changed) {
+    if (changed_np) {
         GList *saved = NULL;
         if (sh->has_nodepath()) {
             saved = sh->save_nodepath_selection();
         }
 
-        sh->reset_item ();
+        sh->reset_item(SH_NODEPATH);
 
         if (sh->has_nodepath() && saved) {
             sh->restore_nodepath_selection(saved);
@@ -166,6 +197,18 @@ static void shapeeditor_event_attr_changed(Inkscape::XML::Node */*repr*/, gchar
         }
     }
 
+
+    if (sh->has_knotholder())
+    {
+        changed_kh = !sh->has_local_change(SH_KNOTHOLDER);
+        sh->decrement_local_change(SH_KNOTHOLDER);
+        if (changed_kh) {
+            // this can happen if an LPEItem's knotholder handle was dragged, in which case we want
+            // to keep the knotholder; in all other cases (e.g., if the LPE itself changes) we delete it
+            sh->reset_item(SH_KNOTHOLDER, !strcmp(name, "d"));
+        }
+    }
+
     sh->update_statusbar(); //TODO: sh->get_container()->update_statusbar();
 }
 
@@ -178,42 +221,52 @@ static Inkscape::XML::NodeEventVector shapeeditor_repr_events = {
 };
 
 
-void ShapeEditor::set_item(SPItem *item) {
-
-    unset_item();
+void ShapeEditor::set_item(SPItem *item, SubType type, bool keep_knotholder) {
+    // this happens (and should only happen) when for an LPEItem having both knotholder and nodepath the knotholder
+    // is adapted; in this case we don't want to delete the knotholder since this freezes the handles
+    unset_item(type, keep_knotholder);
 
     this->grab_node = -1;
 
     if (item) {
-        if (SP_IS_LPE_ITEM(item)) {
-            SPLPEItem *lpeitem = SP_LPE_ITEM(item);
-            Inkscape::LivePathEffect::Effect *lpe = sp_lpe_item_get_current_lpe(lpeitem);
-            if (!(lpe && lpe->isVisible() && lpe->providesKnotholder())) {
-                // only create nodepath if the item either doesn't have an LPE
-                // or the LPE is invisible or it doesn't provide a knotholder itself
-                this->nodepath = sp_nodepath_new(desktop, item,
-                                                 (prefs_get_int_attribute("tools.nodes", "show_handles", 1) != 0));
-            } else if (lpe && lpe->isVisible()) {
-                sp_lpe_item_add_temporary_canvasitems(lpeitem, desktop);
-            }
-        }
-
-        if (this->nodepath) {
-            this->nodepath->shape_editor = this;
-        }
-        this->knotholder = sp_item_knot_holder(item, desktop);
-
-        if (this->nodepath || this->knotholder) {
-            // setting new listener
-            Inkscape::XML::Node *repr;
-            if (this->knotholder)
-                repr = this->knotholder->repr;
-            else
-                repr = SP_OBJECT_REPR(item);
-            if (repr) {
-                Inkscape::GC::anchor(repr);
-                sp_repr_add_listener(repr, &shapeeditor_repr_events, this);
-            }
+        Inkscape::XML::Node *repr;
+        switch(type) {
+            case SH_NODEPATH:
+                if (SP_IS_LPE_ITEM(item)) {
+                    //SPLPEItem *lpeitem = SP_LPE_ITEM(item);
+                    //Inkscape::LivePathEffect::Effect *lpe = sp_lpe_item_get_current_lpe(lpeitem);
+                    //if (!(lpe && lpe->isVisible() && lpe->providesKnotholder())) {
+                        // only create nodepath if the item either doesn't have an LPE
+                        // or the LPE is invisible or it doesn't provide a knotholder itself
+                        this->nodepath = sp_nodepath_new(desktop, item,
+                                                         (prefs_get_int_attribute("tools.nodes", "show_handles", 1) != 0));
+                    //} else if (lpe && lpe->isVisible()) {
+                    //    sp_lpe_item_add_temporary_canvasitems(lpeitem, desktop);
+                    //}
+                }
+                if (this->nodepath) {
+                    this->nodepath->shape_editor = this;
+
+                    // setting new listener
+                    repr = SP_OBJECT_REPR(item);
+                    Inkscape::GC::anchor(repr);
+                    sp_repr_add_listener(repr, &shapeeditor_repr_events, this);
+                }
+                break;
+
+            case SH_KNOTHOLDER:
+                if (!this->knotholder) {
+                    // only recreate knotholder if none is present
+                    this->knotholder = sp_item_knot_holder(item, desktop);
+                }
+                if (this->knotholder) {
+                    this->knotholder->update_knots();
+                    // setting new listener
+                    repr = this->knotholder->repr;
+                    Inkscape::GC::anchor(repr);
+                    sp_repr_add_listener(repr, &shapeeditor_repr_events, this);
+                }
+                break;
         }
     }
 }
@@ -223,7 +276,7 @@ void ShapeEditor::set_item(SPItem *item) {
 */
 void ShapeEditor::set_item_lpe_path_parameter(SPItem *item, SPObject *lpeobject, const char * key)
 {
-    unset_item();
+    unset_item(SH_NODEPATH);
 
     this->grab_node = -1;
 
@@ -250,7 +303,7 @@ void ShapeEditor::set_item_lpe_path_parameter(SPItem *item, SPObject *lpeobject,
 void
 ShapeEditor::set_knotholder(KnotHolder * knot_holder)
 {
-    unset_item();
+    unset_item(SH_KNOTHOLDER);
 
     this->grab_node = -1;
 
@@ -262,17 +315,24 @@ ShapeEditor::set_knotholder(KnotHolder * knot_holder)
 
 /** FIXME: think about this. Is this thing only called when the item needs to be updated?
    Why not make a reload function in NodePath and in KnotHolder? */
-void ShapeEditor::reset_item ()
+void ShapeEditor::reset_item (SubType type, bool keep_knotholder)
 {
-    if ( (this->nodepath) && (IS_LIVEPATHEFFECT(this->nodepath->object)) ) {
-        SPItem * item = this->nodepath->item;
-        SPObject *obj = this->nodepath->object;
-        char * key = g_strdup(this->nodepath->repr_key);
-        set_item_lpe_path_parameter(item, obj, key); // the above checks for nodepath, so it is indeed a path that we are editing
-        g_free(key);
-    } else {
-        SPItem * item = get_item();
-        set_item(item);
+    switch (type) {
+        case SH_NODEPATH:
+            if ( (this->nodepath) && (IS_LIVEPATHEFFECT(this->nodepath->object)) ) {
+                SPItem * item = this->nodepath->item;
+                SPObject *obj = this->nodepath->object;
+                char * key = g_strdup(this->nodepath->repr_key);
+                set_item_lpe_path_parameter(item, obj, key); // the above checks for nodepath, so it is indeed a path that we are editing
+                g_free(key);
+            }            
+            break;
+        case SH_KNOTHOLDER:
+            if (this->knotholder) {
+                SPItem * item = get_item(SH_KNOTHOLDER);
+                set_item(item, SH_KNOTHOLDER, keep_knotholder);
+            }
+            break;
     }
 }
 
@@ -290,7 +350,7 @@ bool ShapeEditor::is_over_stroke (NR::Point event_p, bool remember) {
     if (!this->nodepath)
         return false; // no stroke in knotholder
 
-    SPItem *item = get_item();
+    SPItem *item = get_item(SH_NODEPATH);
 
     //Translate click point into proper coord system
     this->curvepoint_doc = desktop->w2d(event_p);
@@ -339,7 +399,8 @@ void ShapeEditor::add_node_near_point() {
 void ShapeEditor::select_segment_near_point(bool toggle) {
     if (this->nodepath) {
         sp_nodepath_select_segment_near_point(this->nodepath, this->curvepoint_doc, toggle);
-    } else if (this->knotholder) {
+    }
+    if (this->knotholder) {
         // we do not select segments in knotholder... yet?
     }
 }
@@ -382,7 +443,8 @@ void ShapeEditor::curve_drag(gdouble eventx, gdouble eventy) {
         this->curvepoint_event[NR::X] = x;
         this->curvepoint_event[NR::Y] = y;
 
-    } else if (this->knotholder) {
+    }
+    if (this->knotholder) {
         // we do not drag curve in knotholder
     }
 
diff --git a/src/shape-editor.h b/src/shape-editor.h
index 48d008b1f..918102761 100644
--- a/src/shape-editor.h
+++ b/src/shape-editor.h
@@ -27,25 +27,31 @@ class SPDesktop;
 class SPNodeContext;
 class ShapeEditorsCollective;
 
+enum SubType{
+    SH_NODEPATH,
+    SH_KNOTHOLDER
+};
+
 class ShapeEditor {
 public:
 
     ShapeEditor(SPDesktop *desktop);
     ~ShapeEditor();
 
-    void set_item (SPItem *item);
+    void set_item (SPItem *item, SubType type, bool keep_knotholder = false);
     void set_item_lpe_path_parameter(SPItem *item, SPObject *lpeobject, const char * key);
     void set_knotholder(KnotHolder * knot_holder);
-    void reset_item ();
-    void unset_item ();
+    void reset_item (SubType type, bool keep_knotholder = true);
+    void unset_item (SubType type, bool keep_knotholder = false);
 
-    SPItem *get_item ();
+    SPItem *get_item (SubType type);
 
     bool has_nodepath ();
     bool has_knotholder ();
+    void update_knotholder ();
 
-    bool has_local_change ();
-    void decrement_local_change ();
+    bool has_local_change (SubType type);
+    void decrement_local_change (SubType type);
 
     GList *save_nodepath_selection ();
     void restore_nodepath_selection (GList *saved);