View Issue Details

IDProjectCategoryView StatusLast Update
0016308ScribusScripterpublic2020-11-07 14:52
Reporterale Assigned Toale  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version1.5.6.svn 
Target VersionFixed in Version1.5.6.svn 
Summary0016308: 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
TagsNo tags attached.
PatchYes

Activities

ale

2020-11-03 15:17

manager  

getAllLayerItems.py (612 bytes)
getAllObjects.diff (3,912 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)
getAllObjects.diff (3,912 bytes)
layers.sla (17,267 bytes)

ale

2020-11-03 16:04

manager   ~0048312

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 ...

ale

2020-11-04 08:07

manager   ~0048323

Last edited: 2020-11-04 08:08

View 2 revisions

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...

jghali

2020-11-04 18:43

administrator   ~0048334

I have committed most of the changes with a few variable renaming, noticeably the lambda name which was not really ok imo.

ale

2020-11-04 19:53

manager   ~0048335

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...)

Issue History

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