View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0016315 | Scribus | Scripter | public | 2020-11-04 15:35 | 2021-03-23 06:25 |
Reporter | ale | Assigned To | jghali | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 1.5.6.svn | ||||
Fixed in Version | 1.5.7.svn | ||||
Summary | 0016315: Scripter's duplicateObject(s) should not affect the clipboard | ||||
Description | currently duplicateObject() is just using the copy paste slots. | ||||
Tags | No tags attached. | ||||
Patch | Yes | ||||
|
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__ |
|
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? |
|
I fixed the issue with a different approach which allows to fix also duplicateObjects() at the same time. |
|
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. |
|
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. |
|
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. |
|
>> 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. |
|
>> 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. |
|
please re-read the communication on irc. i don't think that your patch does what i was looking for. |
|
Selection tempSelection(nullptr, false); tempSelection.addItem(item1); tempSelection.addItem(item2); ... tempSelection.addItem(itemN); doc->itemSelection_Duplicate(shiftX, shiftY, &tempSelection); |
|
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. |
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 | |
2020-11-13 09:57 | jghali | Note Edited: 0048394 | |
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 | |
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 | |
2021-03-23 06:25 | cbradney | Status | resolved => closed |