View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0016308 | Scribus | Scripter | public | 2020-11-03 15:17 | 2020-11-07 14:52 |
Reporter | ale | Assigned To | ale | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 1.5.6.svn | ||||
Target Version | Fixed in Version | 1.5.6.svn | |||
Summary | 0016308: Readability and feature fixes for getAllObjects() | ||||
Description | - check for an open doc before reading the doc - use the standard kwargs as the function argument - type is an itemType... - use the same lambda for counting the items and for filtering them (which fixes the bug in the second check) - use the same "not set" value for item types and layers (-1) - made the counters local to the place where they are used ... as said, i would have used append instead of insert, which imo would have made the code slightly more readable. i'm adding a test py script and a sla file to be used with it. they can be used as scribus -g -py /tmp/getAllLayerItems.py -- /tmp/layers.sla | ||||
Tags | No tags attached. | ||||
Patch | Yes | ||||
|
getAllLayerItems.py (612 bytes)
getAllObjects.diff (3,912 bytes)
layers.sla (17,267 bytes)diff --git a/scribus/plugins/scriptplugin/cmdgetprop.cpp b/scribus/plugins/scriptplugin/cmdgetprop.cpp index cf8ce5ce4..bc0a186a3 100644 --- a/scribus/plugins/scriptplugin/cmdgetprop.cpp +++ b/scribus/plugins/scriptplugin/cmdgetprop.cpp @@ -9,6 +9,8 @@ for which a new license (GPL+exception) is in place. #include "scribuscore.h" #include "scribusdoc.h" +#include <algorithm> + /* getObjectType(name) */ PyObject *scribus_getobjecttype(PyObject* /* self */, PyObject* args) { @@ -308,69 +310,67 @@ PyObject *scribus_getrotation(PyObject* /* self */, PyObject* args) return PyFloat_FromDouble(static_cast<double>(item->rotation() * -1)); } -PyObject *scribus_getallobjects(PyObject* /* self */, PyObject* args, PyObject *keywds) +PyObject *scribus_getallobjects(PyObject* /* self */, PyObject* args, PyObject *kwargs) { - int type = -1; - uint counter = 0; - uint counter2 = 0; + int itemType = -1; + int layerId = -1; + + if (!checkHaveDocument()) + return nullptr; ScribusDoc* currentDoc = ScCore->primaryMainWindow()->doc; - int pageNr = currentDoc->currentPageNumber(); - char *kwlist[] = { const_cast<char*>("type"), const_cast<char*>("page"), const_cast<char*>("layer"), nullptr}; - char* szLayerName = const_cast<char*>(""); - if (!PyArg_ParseTupleAndKeywords(args, keywds, "|iies", kwlist, &type, &pageNr, "utf-8", &szLayerName)) - return nullptr; + int page = currentDoc->currentPageNumber(); + const int numPages = currentDoc->Pages->count(); - if (!checkHaveDocument()) + char *kwlist[] = { const_cast<char*>("itemType"), const_cast<char*>("page"), const_cast<char*>("layer"), nullptr}; + char* szLayerName = const_cast<char*>(""); + + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|iies", kwlist, &itemType, &page, "utf-8", &szLayerName)) return nullptr; - int numPages = currentDoc->Pages->count(); - if (pageNr < 0 || pageNr >= numPages) + if (page < 0 || page >= numPages) { PyErr_SetString(PyExc_ValueError, QObject::tr("page number is invalid.", "python error").toLocal8Bit().constData()); return nullptr; } - const ScLayer *layer = nullptr; QString layerName = QString::fromUtf8(szLayerName); if (!layerName.isEmpty()) { - layer = currentDoc->Layers.layerByName(layerName); + const auto layer = currentDoc->Layers.layerByName(layerName); if (!layer) { PyErr_SetString(PyExc_ValueError, QObject::tr("layer name is invalid.", "python error").toLocal8Bit().constData()); return nullptr; } + + layerId = layer->ID; } - // have doc already - for (int i = 0; i < currentDoc->Items->count(); ++i) + auto compare = [page, layerId, itemType](PageItem* i) { - PageItem* item = currentDoc->Items->at(i); - if (pageNr != item->OwnPage) - continue; - if ((type != -1) && (item->itemType() != type)) - continue; - if (layer && (layer->ID != item->m_layerID)) - continue; - counter++; - } + return i->OwnPage == page && + (layerId == -1 || i->m_layerID == layerId) && + (itemType == -1 || i->itemType() == itemType); + }; + + const int n = std::count_if(currentDoc->DocItems.begin(), currentDoc->DocItems.end(), compare); - PyObject* pyList = PyList_New(counter); - for (int i = 0; i < currentDoc->Items->count(); ++i) + auto itemsList = PyList_New(n); { - PageItem* item = currentDoc->Items->at(i); - if (pageNr != item->OwnPage) - continue; - if ((type != -1) && (item->itemType() == type)) - continue; - if (layer && (layer->ID != item->m_layerID)) - continue; - PyList_SetItem(pyList, counter2, PyUnicode_FromString(item->itemName().toUtf8())); - counter2++; + int i = 0; + for (const auto& pageItem: currentDoc->DocItems) + { + if (compare(pageItem)) + { + PyList_SetItem(itemsList, i, PyUnicode_FromString(pageItem->itemName().toUtf8())); + i++; + } + } } - return pyList; + + return itemsList; } PyObject *scribus_getobjectattributes(PyObject* /* self */, PyObject* args) |
|
as discussed on irc, if Items only contains the items on the current page without the items in the master page behind it, please replace DocItems by Items. and maybe document the content of DocItems, MasterItems and Items in scribusdoc.h ... |
|
i would be glad if you could fix the other quirks in the code too. - it's not good to duplicate the same (non trivial) test - the argument is called kwargs. really. - do not define counter at the beginning of the function but when they are used (it's not c) i still have the feeling that the implementation i proposed is much more readable and less prone to having bugs (when something is changed). p.s.: ah, and use the same way for testing for layers and itemtype... |
|
I have committed most of the changes with a few variable renaming, noticeably the lambda name which was not really ok imo. |
|
ok. thanks. renaming the lambda is probably fine. i did not spend much time on the name. (and i don't have an habit yet for naming lambdas...) |
Date Modified | Username | Field | Change |
---|---|---|---|
2020-11-03 15:17 | ale | New Issue | |
2020-11-03 15:17 | ale | File Added: getAllLayerItems.py | |
2020-11-03 15:17 | ale | File Added: getAllObjects.diff | |
2020-11-03 15:17 | ale | File Added: layers.sla | |
2020-11-03 15:17 | ale | Summary | readability and feature fixes for getallobjects => [PATCH] readability and feature fixes for getallobjects |
2020-11-03 15:17 | ale | Patch | No => Yes |
2020-11-03 16:04 | ale | Note Added: 0048312 | |
2020-11-04 08:07 | ale | Note Added: 0048323 | |
2020-11-04 08:08 | ale | Note Edited: 0048323 | View Revisions |
2020-11-04 18:43 | jghali | Fixed in Version | => 1.5.6.svn |
2020-11-04 18:43 | jghali | Summary | [PATCH] readability and feature fixes for getallobjects => Readability and feature fixes for getAllObjects |
2020-11-04 18:43 | jghali | Note Added: 0048334 | |
2020-11-04 18:44 | jghali | Summary | Readability and feature fixes for getAllObjects => Readability and feature fixes for getAllObjects() |
2020-11-04 18:44 | jghali | Assigned To | => ale |
2020-11-04 18:44 | jghali | Status | new => resolved |
2020-11-04 18:44 | jghali | Resolution | open => fixed |
2020-11-04 19:53 | ale | Note Added: 0048335 | |
2020-11-07 14:52 | cbradney | Status | resolved => closed |