View Issue Details

IDProjectCategoryView StatusLast Update
0016315ScribusScripterpublic2020-11-15 08:44
Reporterale Assigned Tojghali  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version1.5.6.svn 
Target VersionFixed in Version1.5.7.svn 
Summary0016315: Scripter's duplicateObject(s) should not affect the clipboard
Descriptioncurrently duplicateObject() is just using the copy paste slots.
TagsNo tags attached.
PatchYes

Activities

ale

2020-11-11 18:02

manager   ~0048379

i have a first (working!) draft for correctly duplicating an item...

comments are very welcome.

duplicate-draft.diff (3,991 bytes)
diff --git a/scribus/pageitem.cpp b/scribus/pageitem.cpp
index 8ecb6661d..2367d3e19 100644
--- a/scribus/pageitem.cpp
+++ b/scribus/pageitem.cpp
@@ -72,6 +72,7 @@ for which a new license (GPL+exception) is in place.
 #include "scribusdoc.h"
 #include "scribusview.h"
 #include "scribuswin.h"
+#include "scribusXml.h"
 #include "sctextstream.h"
 #include "selection.h"
 #include "text/storytext.h"
@@ -8749,6 +8750,56 @@ void PageItem::getOldBoundingRect(double *x1, double *y1, double *x2, double *y2
 	}
 }
 
+PageItem* PageItem::duplicate()
+{
+	// DoDrawing = false;
+	// view()->updatesOn(false);
+
+	UndoTransaction activeTransaction;
+	if (UndoManager::undoEnabled())
+		activeTransaction = undoManager->beginTransaction(getUName(), getUPixmap(), Um::MultipleDuplicate, "", Um::IMultipleDuplicate);
+
+	Selection selection(this, false);
+	selection.addItem(this);
+
+	ScriXmlDoc ss;
+	QString BufferS = ss.writeElem(doc(), &selection);
+
+	auto nItems = doc()->Items->count();
+
+	ss.readElem(BufferS, doc(), doc()->currentPage()->xOffset(), doc()->currentPage()->yOffset(), false, true);
+
+	// TODO: what todo if there is no one more item?
+	auto newItem = doc()->Items->at(nItems);
+
+	if (activeTransaction)
+		activeTransaction.commit("", nullptr, "", tr("%1 duplicated").arg(getUName()), nullptr);
+
+	// DoDrawing = true;
+	// view()->updatesOn(true);
+	// m_View->deselectItems(true);
+	// view()->DrawNew();
+	changed();
+	return newItem;
+}
+
 void PageItem::getVisualBoundingRect(double * x1, double * y1, double * x2, double * y2) const
 {
 	double minx =  std::numeric_limits<double>::max();
diff --git a/scribus/pageitem.h b/scribus/pageitem.h
index 4e7dc56ab..5f68e1eb3 100644
--- a/scribus/pageitem.h
+++ b/scribus/pageitem.h
@@ -500,6 +500,8 @@ public: // Start public functions
 
 	//<< ********* Attributes of the item *********
 	//Position
+	PageItem* duplicate();
 	double xPos() const { return m_xPos; }
 	double yPos() const { return m_yPos; }
 	virtual double visualXPos() const;
diff --git a/scribus/plugins/scriptplugin/cmdobj.cpp b/scribus/plugins/scriptplugin/cmdobj.cpp
index fc76020ee..712d4f79c 100644
--- a/scribus/plugins/scriptplugin/cmdobj.cpp
+++ b/scribus/plugins/scriptplugin/cmdobj.cpp
@@ -825,27 +825,20 @@ PyObject *scribus_setcharstyle(PyObject* /* self */, PyObject* args)
 
 PyObject *scribus_duplicateobject(PyObject * /* self */, PyObject *args)
 {
-	char* name = const_cast<char*>("");
-	if (!PyArg_ParseTuple(args, "|es", "utf-8", &name))
-		return nullptr;
 	if (!checkHaveDocument())
 		return nullptr;
 
-	// Is there a special name given? Yes -> add this to selection
-	ScribusMainWindow* currentWin = ScCore->primaryMainWindow();
-	ScribusDoc* currentDoc = currentWin->doc;
+	char* name = const_cast<char*>("");
+	if (!PyArg_ParseTuple(args, "|es", "utf-8", &name))
+		return nullptr;
 
-	PageItem *i = GetUniqueItem(QString::fromUtf8(name));
-	if (i == nullptr)
+	PageItem *pageItem = GetSingleItem(name);
+	if (pageItem == nullptr)
 		return nullptr;
-	currentDoc->m_Selection->clear();
-	currentDoc->m_Selection->addItem(i);
 
-	// do the duplicate
-	currentWin->slotEditCopy();
-	currentWin->slotEditPaste();
+	auto newItem = pageItem->duplicate();
 
-	return PyUnicode_FromString(currentDoc->m_Selection->itemAt(0)->itemName().toUtf8());
+	return PyUnicode_FromString(newItem->itemName().toUtf8());
 }
 
 PyObject *scribus_duplicateobjects(PyObject * /* self */, PyObject *args)
@@ -1071,6 +1064,29 @@ PyObject *scribus_pasteobjects(PyObject * /* self */, PyObject * /*args*/)
 	return pyList;
 }
 
 /*! HACK: this removes "warning: 'blah' defined but not used" compiler warnings
 with header files structure untouched (docstrings are kept near declarations)
 PV */
@@ -1096,6 +1112,7 @@ void cmdobjdocwarnings()
 	  << scribus_getparagraphstyle__doc__
 	  << scribus_getstyle__doc__
 	  << scribus_gettextflowmode__doc__
 	  << scribus_objectexists__doc__
 	  << scribus_pasteobject__doc__
 	  << scribus_pasteobjects__doc__
duplicate-draft.diff (3,991 bytes)

ale

2020-11-11 18:06

manager   ~0048380

for better readability, here is the implementation of PageItem::duplicate and of the scripter command:

```
PageItem* PageItem::duplicate()
{
    // DoDrawing = false;
    // view()->updatesOn(false);

    UndoTransaction activeTransaction;
    if (UndoManager::undoEnabled())
        activeTransaction = undoManager->beginTransaction(getUName(), getUPixmap(), Um::MultipleDuplicate, "", Um::IMultipleDuplicate);

    Selection selection(this, false);
    selection.addItem(this);

    ScriXmlDoc ss;
    QString BufferS = ss.writeElem(doc(), &selection);

    auto nItems = doc()->Items->count();

    ss.readElem(BufferS, doc(), doc()->currentPage()->xOffset(), doc()->currentPage()->yOffset(), false, true);

    // TODO: what todo if there is no one more item?
    auto newItem = doc()->Items->at(nItems);

    if (activeTransaction)
        activeTransaction.commit("", nullptr, "", tr("%1 duplicated").arg(getUName()), nullptr);

    // DoDrawing = true;
    // view()->updatesOn(true);
    // m_View->deselectItems(true);
    // view()->DrawNew();
    changed();
    return newItem;
}
```

```
PyObject *scribus_duplicateobject(PyObject * /* self */, PyObject *args)
{
    if (!checkHaveDocument())
        return nullptr;

    char* name = const_cast<char*>("");
    if (!PyArg_ParseTuple(args, "|es", "utf-8", &name))
        return nullptr;

    PageItem *pageItem = GetSingleItem(name);
    if (pageItem == nullptr)
        return nullptr;

    auto newItem = pageItem->duplicate();

    return PyUnicode_FromString(newItem->itemName().toUtf8());
}
```

some open questions are:

- is there a need for turning off the screen update? probably not for the scripter (it has a command for turning off the update... but if we want to use for other duplicate tasks...)
- can the creation of the new item fail? if yes, is it worth to catch it and return nullptr?

jghali

2020-11-12 20:54

administrator   ~0048389

I fixed the issue with a different approach which allows to fix also duplicateObjects() at the same time.

ale

2020-11-12 21:45

manager   ~0048391

i've spent some time to create this patch, and i think it would have been kind to present your different plans before simply resolving this in a different way.

i skimmed through your patch and it seems to be exactly the contrary of what i wanted to see being implemented in scribus.

i wanted to simplify the code by separating the part that duplicates the item from the code that selects the items and shifts them. on the contrary, you're adding even more complexity.

i'm really not amused by this.
it's really depressing to create patches for scribus.

jghali

2020-11-13 09:48

administrator   ~0048394

Last edited: 2020-11-13 09:57

View 3 revisions

The big problem with your approach is that it add codes (my approach has basically just moved code) and makes things slower. With multiple selections, you must not call ScriXmlDoc::writeElem() /and ScriXmlDoc::readElem() multiple times per copy, but only once for the whole selection. Similarly your per item approach would trigger multiple screen updates unnecessarily with multiple selections. That's why a per item approach is not correct or wanted in this case. In the same way, working at selection level will often allow to remove calls which would be redundant with a per item approach.

ale

2020-11-13 10:24

manager   ~0048395

scribus needs to define a patch review process.

it's really not ok to simply ditch somebody's patch and redo it with no details on the reasons.
even if you have good reasons.
most of all if i was explicitly asking for a review.

i value the comments your have now added and i would have been ready to check if i could have revise my approach to achieve the goals you mentions.

personally, i still think that the duplication code:

- should be more modular
- separate the creation of the item(s) from their placement
- not affect the current selection
- be more straighforward

so, if the screen refreshing is an issue, we might need a way to disable it, not make the single actions a special case of multiple actions (most of all if the multiple actions contain more features that is not relevant to the single one).

in my eyes the code was hard to read (it took me too much time to understand what was happening) and you made even more complex.
and we need to fix that, if we want more people to work on the scribus code base.

and we also urgently need to fix the review process.

jghali

2020-11-13 10:46

administrator   ~0048397

>> so, if the screen refreshing is an issue, we might need a way to disable it, not make the single actions a special case of multiple actions (most of all if the multiple actions contain more features that is not relevant to the single one).

If you had correctly read what I have written, the screen updates are not the only problem in your approach (and we have already a way to disable them, see ScribusDoc::itemSelection_MultipleDuplicate() code). In your case, a per item approach is not wanted because that would also require unnecessary calls to ScriXmlDoc()'s writeElem and readElem(). Those calls are slow and must be performed only once per copy in case of multiple selections.

jghali

2020-11-13 10:48

administrator   ~0048398

Last edited: 2020-11-13 10:49

View 2 revisions

>> scribus needs to define a patch review process.
>> it's really not ok to simply ditch somebody's patch and redo it with no details on the reasons.

In this case, I wrote how to do it on IRC even before you submit that patch. You chose to do it in a different way. Your choice... But do not be astonished I do it in the way I specified afterwards.

ale

2020-11-13 13:15

manager   ~0048404

please re-read the communication on irc.

i don't think that your patch does what i was looking for.

jghali

2020-11-13 23:25

administrator   ~0048409

Selection tempSelection(nullptr, false);
tempSelection.addItem(item1);
tempSelection.addItem(item2);
...
tempSelection.addItem(itemN);

doc->itemSelection_Duplicate(shiftX, shiftY, &tempSelection);

ale

2020-11-15 08:44

manager   ~0048420

i started writing a longer comment on what a review process should look like and how the code in the duplicate functions ask in comments for exactly what i wanted to do (TODO: remove dependency with view).

i give up.

scribus urgently needs a better review process *and* further improve the modularity of the code (and avoid that the core code depends on the UI).

but at this point it's the two maintainers who have to ask for help on it.
i'm too tired of proposing things, seeing them somehow accepted and never implemented.

just one last thing:

currentDoc->m_Selection->addItem(item);

in cmdobj.cpp.

the command *is* modifying the current selection. period.

probably, this was my last patch for some time.

Issue History

Date Modified Username Field Change
2020-11-04 15:35 ale New Issue
2020-11-11 18:02 ale File Added: duplicate-draft.diff
2020-11-11 18:02 ale Note Added: 0048379
2020-11-11 18:06 ale Note Added: 0048380
2020-11-11 18:57 ale Summary duplicateObject() should not affect the clipboard => [PATCH to be reviewed] duplicateObject() should not affect the clipboard
2020-11-11 18:57 ale Patch No => Yes
2020-11-11 20:17 ale Summary [PATCH to be reviewed] duplicateObject() should not affect the clipboard => [PATCH draft] duplicateObject() should not affect the clipboard
2020-11-12 20:53 jghali Summary [PATCH draft] duplicateObject() should not affect the clipboard => Scripter's duplicateObject(s) should not affect the clipboard
2020-11-12 20:54 jghali Assigned To => jghali
2020-11-12 20:54 jghali Status new => resolved
2020-11-12 20:54 jghali Resolution open => fixed
2020-11-12 20:54 jghali Fixed in Version => 1.5.7.svn
2020-11-12 20:54 jghali Note Added: 0048389
2020-11-12 21:45 ale Note Added: 0048391
2020-11-12 21:46 ale Status resolved => feedback
2020-11-12 21:46 ale Resolution fixed => reopened
2020-11-13 09:45 jghali Status feedback => resolved
2020-11-13 09:45 jghali Resolution reopened => fixed
2020-11-13 09:48 jghali Note Added: 0048394
2020-11-13 09:49 jghali Note Edited: 0048394 View Revisions
2020-11-13 09:57 jghali Note Edited: 0048394 View Revisions
2020-11-13 10:24 ale Note Added: 0048395
2020-11-13 10:24 ale Status resolved => feedback
2020-11-13 10:24 ale Resolution fixed => reopened
2020-11-13 10:33 jghali Status feedback => resolved
2020-11-13 10:33 jghali Resolution reopened => fixed
2020-11-13 10:46 jghali Note Added: 0048397
2020-11-13 10:48 jghali Note Added: 0048398
2020-11-13 10:49 jghali Note Edited: 0048398 View Revisions
2020-11-13 13:15 ale Note Added: 0048404
2020-11-13 23:25 jghali Note Added: 0048409
2020-11-15 08:44 ale Note Added: 0048420