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

[Alexander Brock]

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

I added "Edit->Clone->Unlink Clones recursively": It recurses into groups and unlinks every clone it finds, including clones used as clipping paths.

Mc... rewrote the function and I improved it further.

[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]

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

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

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

Preserve clips when unlinking clones

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

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

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

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

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

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

Unmerged revisions

=== modified file 'src/menus-skeleton.h'
--- src/menus-skeleton.h	2016-04-15 14:39:49 +0000
+++ src/menus-skeleton.h	2016-11-04 14:19:19 +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-03 23:28:50 +0000
+++ src/object-set.h	2016-11-04 14:19:19 +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-02 23:08:41 +0000
+++ src/selection-chemistry.cpp	2016-11-04 14:19:19 +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.
@@ -2715,8 +2715,56 @@
             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_){
+        if (SP_IS_GROUP(it)) {
+            std::vector<SPObject*> c = it->childList(false);
+            tmp_set.setList(c);
+            unlinked = tmp_set.unlinkRecursive(true) || unlinked;
+        }
+        tmp_set.set(it);
+        bool has_clip  = false;
+        bool has_mask  = false;
+        Inkscape::URIReference *clip = it->clip_ref;
+        Inkscape::URIReference *mask = it->mask_ref;
+        if ((NULL != clip) && (NULL != clip->getObject())) {
+            tmp_set.unsetMask(true,true);
+            has_clip = true;
+        }
+        if ((NULL != mask) && (NULL != mask->getObject())) {
+            tmp_set.unsetMask(false,true);
+            has_mask = true;
+        }
+        unlinked = tmp_set.unlink(true) || unlinked;
+        if (has_mask)
+            tmp_set.setMask(false,false,true);
+        if (has_clip)
+            tmp_set.setMask(true,false,true);
+    }
+    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"));
+    }
+    return unlinked;
 }
 
 void ObjectSet::cloneOriginal()
@@ -3960,7 +4008,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 +4130,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-02 22:42:43 +0000
+++ src/verbs.cpp	2016-11-04 14:19:19 +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-04 14:19:19 +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,