Merge lp:~inkscape+alexander/inkscape/clones into lp:~inkscape.dev/inkscape/trunk

[Alexander Brock]

Add "Edit->Clone->Unlink Clones recursively" function (ObjectSet::unlinkRecursive)

I removed all the gratuitous spacing differences.

[Mc (mc...)]

Looks mostly fine, but I'm not a big fan of the _functions which seems to duplicate the code a bit and double number of functions. I'd prefer a ", bool flash = true" or something like that in the already present function and "if(doflash) desktop->messageStack()->flash(whatever)"

I'll re-read the main new function during the week-end and extensively test it.

[Mc (mc...)]

Ok, so with the small change I did to the codebase, here's what I think should be done:

-> add a "bool skip_undo" flag to ::setMask(), ::unsetMask(), and ::unlink() similar to what's done in other methods in the file. (Also add it to your recursive method).

-> make a ObjectSet with no SPDesktop (just the SPDocument) containing the selection, so that it does not flash recursively.

-> make all the calls with the skip_undo flag on your ObjectSet.

review: Needs Resubmitting (code, refactor)

[Alexander Brock (brock-alexander)]

> Ok, so with the small change I did to the codebase, here's what I think should
> be done:
>
> -> add a "bool skip_undo" flag to ::setMask(), ::unsetMask(), and ::unlink()
> similar to what's done in other methods in the file. (Also add it to your
> recursive method).
>
> -> make a ObjectSet with no SPDesktop (just the SPDocument) containing the
> selection, so that it does not flash recursively.
>
> -> make all the calls with the skip_undo flag on your ObjectSet.

I tried to merge your changes with my branch, failed and restarted from scratch.
I added an unlinkRecursive method to ObjectSet, added the bool skip_undo where I needed it and force-pushed the code to the old location (lp:~inkscape+alexander/inkscape/clones).

[Mc (mc...)]

Good !
I did a merge proposal to your branch to propose a simpler version of the recursive function, though it can be improved and further simplified by putting the unclip/reclip business in the unlink() function where it probably belongs.
If you find the courage, you can also propose unit tests to test the function (see testfiles/src/object-set-test.cpp )

[Tavmjong Bah (tavmjong-free)]

Please get rid of gratuitous spacing differences. It makes it much harder to see what is really being changed.

[Alexander Brock (brock-alexander)]

@mc...: Thank you for rewriting the unlinkRecursive function. I checked out your version, tested it, found some problems and fixed them:

1.

unlinked = unlinked || tmp_set.unlinkRecursive(true);

From the C++ standard: "Unlike |, || guarantees left-to-right evaluation; moreover, the second operand is not evaluated if the first operand evaluates to true."

So if unlinked is already true then the unlinkRecursive function is not called, preventing groups of clones from beeing unlinked. I changed it:

unlinked = tmp_set.unlinkRecursive(true) || unlinked;

2.

if(SP_IS_GROUP(it)) {
......
} else if(dynamic_cast<SPUse*>(it) || dynamic_cast<SPText*>(it) || dynamic_cast<SPTRef*>(it)) {

This causes two problems: First, if an Object is a group masked (or clipped) by a clone that clone would not be unlinked.
Second, if the object is not SPUse, SPText or SPTRef it still is necessary to run the code, it might be clipped or masked by a clone of some object.
I removed the else if so the code runs unconditionally.

3.

unlinked = unlinked || tmp_set.unlink();

This causes calls to DocumentUndo::done (also see 1.), so I changed it:

unlinked = tmp_set.unlink(true) || unlinked;

[lp:~inkscape+alexander/inkscape/clones]

15206. By Alexander Brock <email address hidden> on 2016-11-04

Preserve clips when unlinking clones

[Alexander Brock (brock-alexander)]

The current behaviour for "Edit->Clone->unlink" is to remove the clip of every selected clone. I changed to to first remove the clip, then unlink (possibly both, the object and the clip) and then reapply the clip (or mask).

[lp:~inkscape+alexander/inkscape/clones]

15207. By Alexander Brock <email address hidden> on 2016-11-04

Make ObjectSet::unlink() work for clones which are both clipped and masked

[Alexander Brock (brock-alexander)]

I changed the ObjectSet::unlink function so it also works if a clone is both clipped and masked. Also I removed that part of the code from the ObjectSet::unlinkRecursive function to avoid code duplication.

[lp:~inkscape+alexander/inkscape/clones]

15208. By Alexander Brock <email address hidden> on 2016-11-06

Remove duplicate code from unlinkRecursive() and fix issue with clones of groups

[Alexander Brock (brock-alexander)]

I removed the duplicated code for handling objects clipped by clones from ObjectSet::unlinkRecursive() and fixed a n issue with the function unlinking clones of groups but not recursing into the unlinked group afterwards. Both issues kindly reported by Mc..., thank you.

[lp:~inkscape+alexander/inkscape/clones]

15209. By Alexander Brock <email address hidden> on 2016-11-06

Fix selection issue with ObjectSet::unlinkRecursive() and add tests for it.

=== modified file 'src/menus-skeleton.h'
--- src/menus-skeleton.h	2016-04-15 14:39:49 +0000
+++ src/menus-skeleton.h	2016-11-06 21:37:32 +0000
@@ -66,6 +66,7 @@
 "           <verb verb-id=\"EditClone\" />\n"
 "           <verb verb-id=\"DialogClonetiler\" />\n"
 "           <verb verb-id=\"EditUnlinkClone\" />\n"
+"           <verb verb-id=\"EditUnlinkCloneRecursive\" />\n"
 "           <verb verb-id=\"EditRelinkClone\" />\n"
 "           <verb verb-id=\"EditCloneSelectOriginal\" />\n"
 "           <verb verb-id=\"EditCloneOriginalPathLPE\" />\n"

=== modified file 'src/object-set.h'
--- src/object-set.h	2016-11-06 15:33:01 +0000
+++ src/object-set.h	2016-11-06 21:37:32 +0000
@@ -330,7 +330,19 @@
     void deleteItems();
     void duplicate(bool suppressDone = false, bool duplicateLayer = false);
     void clone();
-    void unlink();
+
+    /**
+     * @brief Unlink all directly selected clones.
+     * @param skip_undo If this is set to true the call to DocumentUndo::done is omitted.
+     * @return True if anything was unlinked, otherwise false.
+     */
+    bool unlink(const bool skip_undo = false);
+    /**
+     * @brief Recursively unlink any clones present in the current selection,
+     * including clones which are used to clip other objects, groups of clones etc.
+     * @return true if anything was unlinked, otherwise false.
+     */
+    bool unlinkRecursive(const bool skip_undo = false);
     void relink();
     void cloneOriginal();
     void cloneOriginalPathLPE();
@@ -376,7 +388,7 @@
     void createBitmapCopy();
     void setMask(bool apply_clip_path, bool apply_to_layer = false, bool skip_undo = false);
     void editMask(bool clip);
-    void unsetMask(bool apply_clip_path);
+    void unsetMask(const bool apply_clip_path, const bool skip_undo = false);
     void setClipGroup();
     
     // moves

=== modified file 'src/selection-chemistry.cpp'
--- src/selection-chemistry.cpp	2016-11-06 15:33:01 +0000
+++ src/selection-chemistry.cpp	2016-11-06 21:37:32 +0000
@@ -2653,12 +2653,12 @@
 }
 
 
-void ObjectSet::unlink()
+bool ObjectSet::unlink(const bool skip_undo)
 {
     if (isEmpty()) {
         if(desktop())
             desktop()->messageStack()->flash(Inkscape::WARNING_MESSAGE, _("Select <b>clones</b> to unlink."));
-        return;
+        return false;
     }
 
     // Get a copy of current selection.
@@ -2669,41 +2669,59 @@
     for (auto i=items_.rbegin();i!=items_.rend();++i){
         SPItem *item = *i;
 
-        if (dynamic_cast<SPText *>(item)) {
-            SPObject *tspan = sp_tref_convert_to_tspan(item);
-
-            if (tspan) {
-                item->requestDisplayUpdate(SP_OBJECT_MODIFIED_FLAG);
+        ObjectSet tmp_set(document());
+        tmp_set.set(item);
+        Inkscape::URIReference *clip = item->clip_ref;
+        Inkscape::URIReference *mask = item->mask_ref;
+        if ((NULL != clip) && (NULL != clip->getObject())) {
+            tmp_set.unsetMask(true,true);
+            unlinked = tmp_set.unlink(true) || unlinked;
+            tmp_set.setMask(true,false,true);
+            new_select.push_back(tmp_set.singleItem());
+        }
+        else if ((NULL != mask) && (NULL != mask->getObject())) {
+            tmp_set.unsetMask(false,true);
+            unlinked = tmp_set.unlink(true) || unlinked;
+            tmp_set.setMask(false,false,true);
+            new_select.push_back(tmp_set.singleItem());
+        }
+        else {
+            if (dynamic_cast<SPText *>(item)) {
+                SPObject *tspan = sp_tref_convert_to_tspan(item);
+
+                if (tspan) {
+                    item->requestDisplayUpdate(SP_OBJECT_MODIFIED_FLAG);
+                }
+
+                // Set unlink to true, and fall into the next if which
+                // will include this text item in the new selection
+                unlinked = true;
             }
 
-            // Set unlink to true, and fall into the next if which
-            // will include this text item in the new selection
-            unlinked = true;
-        }
-
-        if (!(dynamic_cast<SPUse *>(item) || dynamic_cast<SPTRef *>(item))) {
-            // keep the non-use item in the new selection
-            new_select.push_back(item);
-            continue;
-        }
-
-        SPItem *unlink = NULL;
-        SPUse *use = dynamic_cast<SPUse *>(item);
-        if (use) {
-            unlink = use->unlink();
-            // Unable to unlink use (external or invalid href?)
-            if (!unlink) {
+            if (!(dynamic_cast<SPUse *>(item) || dynamic_cast<SPTRef *>(item))) {
+                // keep the non-use item in the new selection
                 new_select.push_back(item);
                 continue;
             }
-        } else /*if (SP_IS_TREF(use))*/ {
-            unlink = dynamic_cast<SPItem *>(sp_tref_convert_to_tspan(item));
-            g_assert(unlink != NULL);
+
+            SPItem *unlink = NULL;
+            SPUse *use = dynamic_cast<SPUse *>(item);
+            if (use) {
+                unlink = use->unlink();
+                // Unable to unlink use (external or invalid href?)
+                if (!unlink) {
+                    new_select.push_back(item);
+                    continue;
+                }
+            } else /*if (SP_IS_TREF(use))*/ {
+                unlink = dynamic_cast<SPItem *>(sp_tref_convert_to_tspan(item));
+                g_assert(unlink != NULL);
+            }
+
+            unlinked = true;
+            // Add ungrouped items to the new selection.
+            new_select.push_back(unlink);
         }
-
-        unlinked = true;
-        // Add ungrouped items to the new selection.
-        new_select.push_back(unlink);
     }
 
     if (!new_select.empty()) { // set new selection
@@ -2715,8 +2733,42 @@
             desktop()->messageStack()->flash(Inkscape::ERROR_MESSAGE, _("<b>No clones to unlink</b> in the selection."));
     }
 
-    DocumentUndo::done(document(), SP_VERB_EDIT_UNLINK_CLONE,
-                       _("Unlink clone"));
+    if (!skip_undo) {
+        DocumentUndo::done(document(), SP_VERB_EDIT_UNLINK_CLONE,
+                           _("Unlink clone"));
+    }
+    return unlinked;
+}
+
+bool ObjectSet::unlinkRecursive(const bool skip_undo) {
+    if (isEmpty()){
+        if (desktop())
+            desktop()->messageStack()->flash(Inkscape::WARNING_MESSAGE, _("Select <b>clones</b> to unlink."));
+        return false;
+    }
+    bool unlinked = false;
+    ObjectSet tmp_set(document());
+    std::vector<SPItem*> items_(items().begin(), items().end());
+    for (auto& it:items_) {
+        tmp_set.set(it);
+        unlinked = tmp_set.unlink(true) || unlinked;
+        it = tmp_set.singleItem();
+        if (SP_IS_GROUP(it)) {
+            std::vector<SPObject*> c = it->childList(false);
+            tmp_set.setList(c);
+            unlinked = tmp_set.unlinkRecursive(true) || unlinked;
+        }
+    }
+    if (!unlinked) {
+        if(desktop())
+            desktop()->messageStack()->flash(Inkscape::ERROR_MESSAGE, _("<b>No clones to unlink</b> in the selection."));
+    }
+    if (!skip_undo) {
+        DocumentUndo::done(document(), SP_VERB_EDIT_UNLINK_CLONE_RECURSIVE,
+                                           _("Unlink clone recursively"));
+    }
+    setList(items_);
+    return unlinked;
 }
 
 void ObjectSet::cloneOriginal()
@@ -3960,7 +4012,7 @@
     }
 }
 
-void ObjectSet::unsetMask(bool apply_clip_path) {
+void ObjectSet::unsetMask(const bool apply_clip_path, const bool skip_undo) {
     SPDocument *doc = document();
     Inkscape::XML::Document *xml_doc = doc->getReprDoc();
 
@@ -4082,10 +4134,12 @@
     // rebuild selection
     addList(items_to_select);
 
-    if (apply_clip_path) {
-        DocumentUndo::done(doc, SP_VERB_OBJECT_UNSET_CLIPPATH, _("Release clipping path"));
-    } else {
-        DocumentUndo::done(doc, SP_VERB_OBJECT_UNSET_MASK, _("Release mask"));
+    if (!skip_undo) {
+        if (apply_clip_path) {
+            DocumentUndo::done(doc, SP_VERB_OBJECT_UNSET_CLIPPATH, _("Release clipping path"));
+        } else {
+            DocumentUndo::done(doc, SP_VERB_OBJECT_UNSET_MASK, _("Release mask"));
+        }
     }
 }
 

=== modified file 'src/verbs.cpp'
--- src/verbs.cpp	2016-11-06 15:33:01 +0000
+++ src/verbs.cpp	2016-11-06 21:37:32 +0000
@@ -1009,6 +1009,9 @@
         case SP_VERB_EDIT_UNLINK_CLONE:
             dt->selection->unlink();
             break;
+        case SP_VERB_EDIT_UNLINK_CLONE_RECURSIVE:
+            dt->selection->unlinkRecursive();
+            break;
         case SP_VERB_EDIT_RELINK_CLONE:
             dt->selection->relink();
             break;
@@ -2530,6 +2533,8 @@
                  N_("Create a clone (a copy linked to the original) of selected object"), INKSCAPE_ICON("edit-clone")),
     new EditVerb(SP_VERB_EDIT_UNLINK_CLONE, "EditUnlinkClone", N_("Unlin_k Clone"),
                  N_("Cut the selected clones' links to the originals, turning them into standalone objects"), INKSCAPE_ICON("edit-clone-unlink")),
+    new EditVerb(SP_VERB_EDIT_UNLINK_CLONE_RECURSIVE, "EditUnlinkCloneRecursive", N_("Unlink Clones _recursively"),
+                 N_("Unlink all clones in the selection, even if they are in groups."), INKSCAPE_ICON("edit-clone-unlink")),
     new EditVerb(SP_VERB_EDIT_RELINK_CLONE, "EditRelinkClone", N_("Relink to Copied"),
                  N_("Relink the selected clones to the object currently on the clipboard"), NULL),
     new EditVerb(SP_VERB_EDIT_CLONE_SELECT_ORIGINAL, "EditCloneSelectOriginal", N_("Select _Original"),

=== modified file 'src/verbs.h'
--- src/verbs.h	2016-10-08 06:16:53 +0000
+++ src/verbs.h	2016-11-06 21:37:32 +0000
@@ -83,6 +83,7 @@
     SP_VERB_EDIT_DUPLICATE,
     SP_VERB_EDIT_CLONE,
     SP_VERB_EDIT_UNLINK_CLONE,
+    SP_VERB_EDIT_UNLINK_CLONE_RECURSIVE,
     SP_VERB_EDIT_RELINK_CLONE,
     SP_VERB_EDIT_CLONE_SELECT_ORIGINAL,
     SP_VERB_EDIT_CLONE_ORIGINAL_PATH_LPE,

=== modified file 'testfiles/src/object-set-test.cpp'
--- testfiles/src/object-set-test.cpp	2016-11-02 23:08:41 +0000
+++ testfiles/src/object-set-test.cpp	2016-11-06 21:37:32 +0000
@@ -105,6 +105,25 @@
     ObjectSet* set2;
 };
 
+#define SP_IS_CLONE(obj) (dynamic_cast<const SPUse*>(obj) != NULL)
+
+bool containsClone(ObjectSet* set) {
+    for (auto it : set->items()) {
+        if (SP_IS_CLONE(it)) {
+            return true;
+        }
+        if (SP_IS_GROUP(it)) {
+            ObjectSet tmp_set(set->document());
+            std::vector<SPObject*> c = it->childList(false);
+            tmp_set.setList(c);
+            if (containsClone(&tmp_set)) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
 TEST_F(ObjectSetTest, Basics) {
     EXPECT_EQ(0, set->size());
     set->add(A);
@@ -429,6 +448,125 @@
     SetUpTestCase();
 }
 
+TEST_F(ObjectSetTest, unlinkRecursiveBasic) {
+    // This is the same as the test (ObjectSetTest, Ops), but with unlinkRecursive instead of unlink.
+    set->set(r1.get());
+    set->add(r2.get());
+    set->add(r3.get());
+    EXPECT_FALSE(containsClone(set));
+    set->duplicate();
+    EXPECT_FALSE(containsClone(set));
+        EXPECT_EQ(9, _doc->getRoot()->children.size());//metadata, defs, namedview, and those 3x2 rects.
+    EXPECT_EQ(3, set->size());
+    EXPECT_FALSE(set->includes(r1.get()));
+    set->deleteItems();
+    EXPECT_FALSE(containsClone(set));
+    EXPECT_TRUE(set->isEmpty());
+    set->add(r1.get());
+    set->add(r2.get());
+    set->add(r3.get());
+    EXPECT_FALSE(containsClone(set));
+    set->group();//r1-3 are now invalid (grouping makes copies)
+    r1.release();
+    r2.release();
+    r3.release();
+    EXPECT_FALSE(containsClone(set));
+    EXPECT_EQ(4, _doc->getRoot()->children.size());
+    EXPECT_EQ(1, set->size());
+    set->ungroup();
+    EXPECT_FALSE(containsClone(set));
+    EXPECT_EQ(6, _doc->getRoot()->children.size());
+    EXPECT_EQ(3, set->size());
+    /* Uncomment this when toNextLayer is made desktop-independent
+    set->group();
+    set2->add(set->singleItem()->childList(false)[0]);
+    EXPECT_EQ(3, set->singleItem()->children.size());
+    EXPECT_EQ(4, _doc->getRoot()->children.size());
+    set2->popFromGroup();
+    EXPECT_EQ(2, set->singleItem()->children.size());
+    EXPECT_EQ(5, _doc->getRoot()->children.size());
+    set->ungroup();
+    set->add(set2->singleItem());
+    */
+    set->clone();
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(9, _doc->getRoot()->children.size());
+    EXPECT_EQ(3, set->size());
+    EXPECT_NE(nullptr, dynamic_cast<SPUse*>(*(set->items().begin())));
+    EXPECT_EQ(nullptr, dynamic_cast<SPRect*>(*(set->items().begin())));
+    set->unlinkRecursive();
+    EXPECT_FALSE(containsClone(set));
+    EXPECT_EQ(9, _doc->getRoot()->children.size());
+    EXPECT_EQ(3, set->size());
+    EXPECT_EQ(nullptr, dynamic_cast<SPUse*>(*(set->items().begin())));
+    EXPECT_NE(nullptr, dynamic_cast<SPRect*>(*(set->items().begin())));
+    set->clone(); //creates 3 clones
+    EXPECT_TRUE(containsClone(set));
+    set->clone(); //creates 3 clones of clones
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(15, _doc->getRoot()->children.size());
+    EXPECT_EQ(3, set->size());
+    EXPECT_NE(nullptr, dynamic_cast<SPUse*>( ((SPUse*)(*(set->items().begin())))->get_original()));//"original is a Use"
+    set->unlinkRecursive(); //clone of clone of rect -> rect
+    EXPECT_FALSE(containsClone(set));
+    EXPECT_EQ(nullptr, dynamic_cast<SPUse*>(*(set->items().begin())));
+    EXPECT_NE(nullptr, dynamic_cast<SPRect*>(*(set->items().begin())));
+    set->clone();
+    EXPECT_TRUE(containsClone(set));
+    set->set(*(set->items().begin()));
+    set->cloneOriginal();//get clone original
+    EXPECT_EQ(18, _doc->getRoot()->children.size());
+    EXPECT_EQ(1, set->size());
+    EXPECT_NE(nullptr, dynamic_cast<SPRect*>(*(set->items().begin())));
+    TearDownTestCase();
+    SetUpTestCase();
+}
+
+TEST_F(ObjectSetTest, unlinkRecursiveAdvanced) {
+    set->set(r1.get());
+    set->add(r2.get());
+    set->add(r3.get());
+    set->group();//r1-3 are now invalid (grouping makes copies)
+    r1.release();
+    r2.release();
+    r3.release();
+    EXPECT_FALSE(containsClone(set));
+    EXPECT_EQ(1, set->size());
+    SPItem* original = set->singleItem();
+    set->clone();
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(1, set->size());
+    set->add(original);
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(2, set->size());
+    set->group();
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(1, set->size());
+    original = set->singleItem();
+    set->clone();
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(1, set->size());
+    set->add(original);
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(2, set->size());
+    set->group();
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(1, set->size());
+    original = set->singleItem();
+    set->clone();
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(1, set->size());
+    set->add(original);
+    EXPECT_TRUE(containsClone(set));
+    EXPECT_EQ(2, set->size());
+    set->unlinkRecursive();
+    EXPECT_FALSE(containsClone(set));
+    EXPECT_EQ(2, set->size());
+
+    TearDownTestCase();
+    SetUpTestCase();
+}
+
 TEST_F(ObjectSetTest, ZOrder) {
     //sp_object_compare_position_bool == true iff "r1<r2" iff r1 is "before" r2 in the file, ie r1 is lower than r2
     EXPECT_TRUE(sp_object_compare_position_bool(r1.get(),r2.get()));