View Issue Details

IDProjectCategoryView StatusLast Update
0015419ScribusInternalpublic2019-06-04 20:56
Reporterale Assigned Tojghali  
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
Product Version1.5.5.svn 
Fixed in Version1.5.5.svn 
Summary0015419: small refactoring for scribusXml
Descriptionhere is the patch for one of the change propositions i have mentioned in the mailing list...

- rename cryptic variables
- refactor out a function
TagsNo tags attached.
PatchNo

Activities

ale

2018-09-06 19:00

manager  

no-emg-or-emmap.diff (2,123 bytes)   
diff --git a/scribus/scribusXml.cpp b/scribus/scribusXml.cpp
index 191aa6e77..afb196e1b 100644
--- a/scribus/scribusXml.cpp
+++ b/scribus/scribusXml.cpp
@@ -160,14 +160,9 @@ QString ScriXmlDoc::WriteElem(ScribusDoc *doc, Selection* selection)
 	PageItem *item;
 	QString documentStr = "";
 	item = selection->itemAt(0);
-	QList<PageItem*> emG;
-	QMap<int, PageItem*> emMap;
-	emG.clear();
-	for (int cor = 0; cor < selection->count(); ++cor)
-	{
-		emMap.insert(doc->Items->indexOf(selection->itemAt(cor)), selection->itemAt(cor));
-	}
-	emG = emMap.values();
+
+	auto items = getItemsFromSelection(doc, selection);
+
 	double selectionWidth = 0;
 	double selectionHeight = 0;
 	if (selection->isMultipleSelection())
@@ -198,9 +193,9 @@ QString ScriXmlDoc::WriteElem(ScribusDoc *doc, Selection* selection)
 	retImg.fill( qRgba(0, 0, 0, 0) );
 	ScPainter *painter = new ScPainter(&retImg, retImg.width(), retImg.height(), 1, 0);
 	painter->setZoomFactor(scaleI);
-	for (int em = 0; em < emG.count(); ++em)
+	for (int em = 0; em < items.count(); ++em)
 	{
-		PageItem* embedded = emG.at(em);
+		PageItem* embedded = items.at(em);
 		painter->save();
 		painter->translate(-xp, -yp);
 		embedded->invalid = true;
@@ -234,3 +229,13 @@ ScElemMimeData* ScriXmlDoc::WriteToMimeData(ScribusDoc *doc, Selection *selectio
 	md->setScribusElem(WriteElem(doc, selection));
 	return md;
 }
+
+QList<PageItem*> ScriXmlDoc::getItemsFromSelection(ScribusDoc *doc, Selection* selection)
+{
+	QList<PageItem*> result;
+	QMap<int, PageItem*> items;
+	for (auto item: selection->items()) {
+	   items.insert(doc->Items->indexOf(item), item);
+	}
+	return items.values();
+}
diff --git a/scribus/scribusXml.h b/scribus/scribusXml.h
index 53e51237e..5d0a269c3 100644
--- a/scribus/scribusXml.h
+++ b/scribus/scribusXml.h
@@ -53,9 +53,11 @@ public:
 	
 	static QString WriteElem(ScribusDoc *doc, Selection *selection);
 	static ScElemMimeData* WriteToMimeData(ScribusDoc *doc, Selection *selection);
+private:
+	static QList<PageItem*> getItemsFromSelection(ScribusDoc *doc, Selection* selection);
 };
 
-#endif // _SCRIBUS_CONFIG_
+#endif
 
 
 
no-emg-or-emmap.diff (2,123 bytes)   

ale

2018-09-06 19:02

manager   ~0045436

... even if i don't understand what's the use of first creating a map...

it would probably be simpler to directly create a vector and, then, eventually sort it (if it's really needed...)

jghali

2018-09-07 19:17

administrator   ~0045443

Last edited: 2018-09-07 19:24

>> it would probably be simpler to directly create a vector and, then, eventually sort it (if it's really needed...)

QMap is ordered, ie QMap elements are already sorted by keys. We need to keep elements in stack order here and QMap allows us to do that simply.

jghali

2018-09-07 19:51

administrator   ~0045444

Last edited: 2018-09-07 20:30

Committed with a small change. Be careful when using range for with Qt containers. On such containers range for loops are more efficient if the iterated container is const. If it's not, range for will call non const version of begin() and end() which may detach the container, ie create a copy of its data... which will be pretty slow. For non const containers, Qt's foreach will often be more efficient than C++11 range for.

ale

2018-09-09 13:49

manager   ~0045449

ok, thanks for the correction!
i did not know about it!

for the stack order: i was mostly wondering, why the items are not in the right from the beginning... instead of ordering them each time they get used... but, well, maybe most element are never used so it might be too much effort to keep the whole document ordered (if it's possible at all).

and thanks for committing the patch...

Issue History

Date Modified Username Field Change
2018-09-06 19:00 ale New Issue
2018-09-06 19:00 ale File Added: no-emg-or-emmap.diff
2018-09-06 19:02 ale Note Added: 0045436
2018-09-06 21:11 jghali Category - => Internal
2018-09-07 19:17 jghali Note Added: 0045443
2018-09-07 19:24 jghali Note Edited: 0045443
2018-09-07 19:47 jghali Summary small refactoring in scribusXml => small refactoring for scribusXml
2018-09-07 19:51 jghali Note Added: 0045444
2018-09-07 19:52 jghali Assigned To => jghali
2018-09-07 19:52 jghali Status new => resolved
2018-09-07 19:52 jghali Resolution open => fixed
2018-09-07 19:52 jghali Fixed in Version => 1.5.5.svn
2018-09-07 19:56 jghali Note Edited: 0045444
2018-09-07 20:30 jghali Note Edited: 0045444
2018-09-09 13:49 ale Note Added: 0045449
2019-06-04 20:56 cbradney Status resolved => closed