KnotHolder: fix knots selection status when a knot is clicked without Shift

When there are already selected knots, and  _another_ knot is clicked
without the Shift key pressed, the user would expects the previously
selected knots to be unselected immediately, but this does not happen
until the mouse button is released.

Steps to replicate:
  1. Change size of a rect using one corner.
  2. Change the size using the opposite corner (without pressing Shift).

Observed behavior: The first corner looks still selected during grab.

Expected behavior: The first corner gets unelected as soon as the mouse
                   button is pressed..

This happens because the knots selection status is not updated until the
mouse button is released, i.e. in the click/ungrabbed signal handler.

In order to have a more immediate feedback of the knot selection status,
add a "mousedown" signal handler and update the selection status in
there.

While at it also remove the knot_holder local variable in
knot_clicked_handler() which seems to be unnecessary.

4 files changed, 25 insertions, 24 deletions

diff --git a/src/knot-holder-entity.cpp b/src/knot-holder-entity.cpp
index 3664bbc81..9408e7942 100644
--- a/src/knot-holder-entity.cpp
+++ b/src/knot-holder-entity.cpp
@@ -58,6 +58,7 @@ void KnotHolderEntity::create(SPDesktop *desktop, SPItem *item, KnotHolder *pare
     update_knot();
     knot->show();
 
+    _mousedown_connection = knot->mousedown_signal.connect(sigc::mem_fun(*parent_holder, &KnotHolder::knot_mousedown_handler));
     _moved_connection = knot->moved_signal.connect(sigc::mem_fun(*parent_holder, &KnotHolder::knot_moved_handler));
     _click_connection = knot->click_signal.connect(sigc::mem_fun(*parent_holder, &KnotHolder::knot_clicked_handler));
     _ungrabbed_connection = knot->ungrabbed_signal.connect(sigc::mem_fun(*parent_holder, &KnotHolder::knot_ungrabbed_handler));
@@ -66,6 +67,7 @@ void KnotHolderEntity::create(SPDesktop *desktop, SPItem *item, KnotHolder *pare
 
 KnotHolderEntity::~KnotHolderEntity()
 {
+    _mousedown_connection.disconnect();
     _moved_connection.disconnect();
     _click_connection.disconnect();
     _ungrabbed_connection.disconnect();
diff --git a/src/knot-holder-entity.h b/src/knot-holder-entity.h
index e6efaa7e8..5e7868b70 100644
--- a/src/knot-holder-entity.h
+++ b/src/knot-holder-entity.h
@@ -87,6 +87,7 @@ public:
     unsigned int   _ungrab_handler_id;
 
 private:
+    sigc::connection _mousedown_connection;
     sigc::connection _moved_connection;
     sigc::connection _click_connection;
     sigc::connection _ungrabbed_connection;
diff --git a/src/knotholder.cpp b/src/knotholder.cpp
index eba9996eb..7d62ff6a5 100644
--- a/src/knotholder.cpp
+++ b/src/knotholder.cpp
@@ -127,22 +127,17 @@ bool KnotHolder::knot_mouseover() const {
 }
 
 void
-KnotHolder::knot_clicked_handler(SPKnot *knot, guint state)
+KnotHolder::knot_mousedown_handler(SPKnot *knot, guint state)
 {
-    KnotHolder *knot_holder = this;
-    SPItem *saved_item = this->item;
-
     if (!(state & GDK_SHIFT_MASK)) {
         unselect_knots();
     }
-    for(std::list<KnotHolderEntity *>::iterator i = knot_holder->entity.begin(); i != knot_holder->entity.end(); ++i) {
+    for(std::list<KnotHolderEntity *>::iterator i = this->entity.begin(); i != this->entity.end(); ++i) {
         KnotHolderEntity *e = *i;
         if (!(state & GDK_SHIFT_MASK)) {
             e->knot->selectKnot(false);
         }
         if (e->knot == knot) {
-            // no need to test whether knot_click exists since it's virtual now
-            e->knot_click(state);
             if (!(e->knot->flags & SP_KNOT_SELECTED) || !(state & GDK_SHIFT_MASK)){
                 e->knot->selectKnot(true);
             } else {
@@ -150,6 +145,19 @@ KnotHolder::knot_clicked_handler(SPKnot *knot, guint state)
             }
         }
     }
+}
+
+void
+KnotHolder::knot_clicked_handler(SPKnot *knot, guint state)
+{
+    SPItem *saved_item = this->item;
+
+    for(std::list<KnotHolderEntity *>::iterator i = this->entity.begin(); i != this->entity.end(); ++i) {
+        KnotHolderEntity *e = *i;
+        if (e->knot == knot)
+            // no need to test whether knot_click exists since it's virtual now
+            e->knot_click(state);
+    }
 
     {
         SPShape *savedShape = dynamic_cast<SPShape *>(saved_item);
@@ -158,7 +166,7 @@ KnotHolder::knot_clicked_handler(SPKnot *knot, guint state)
         }
     }
 
-    knot_holder->update_knots();
+    this->update_knots();
 
     unsigned int object_verb = SP_VERB_NONE;
 
@@ -266,22 +274,11 @@ KnotHolder::knot_ungrabbed_handler(SPKnot *knot, guint state)
     if (this->released) {
         this->released(this->item);
     } else {
-        if (!(state & GDK_SHIFT_MASK)) {
-            unselect_knots();
-        }
-        for(std::list<KnotHolderEntity *>::iterator i = this->entity.begin(); i != this->entity.end(); ++i) {
-            KnotHolderEntity *e = *i;
-            if (!(state & GDK_SHIFT_MASK)) {
-                e->knot->selectKnot(false);
-            }
-            if (e->knot == knot) {
-                if (!(e->knot->flags & SP_KNOT_SELECTED) || !(state & GDK_SHIFT_MASK)){
-                    e->knot->selectKnot(true);
-                } else {
-                    e->knot->selectKnot(false);
-                }
-            }
-        }
+        // if a point is dragged while not selected, it should select itself,
+        // even if it was just unselected in the mousedown event handler.
+        if (!(knot->flags & SP_KNOT_SELECTED))
+            knot->selectKnot(true);
+
         SPObject *object = (SPObject *) this->item;
 
         // Caution: this call involves a screen update, which may process events, and as a
diff --git a/src/knotholder.h b/src/knotholder.h
index b82ba61f9..05adb2241 100644
--- a/src/knotholder.h
+++ b/src/knotholder.h
@@ -51,6 +51,7 @@ public:
 
     void update_knots();
     void unselect_knots();
+    void knot_mousedown_handler(SPKnot *knot, unsigned int state);
     void knot_moved_handler(SPKnot *knot, Geom::Point const &p, unsigned int state);
     void knot_clicked_handler(SPKnot *knot, unsigned int state);
     void knot_ungrabbed_handler(SPKnot *knot, unsigned int state);