View Issue Details

IDProjectCategoryView StatusLast Update
0017189ScribusGeneralpublic2024-04-09 23:06
Reporterale Assigned Tocbradney  
PrioritynormalSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
Product Version1.7.0.svn 
Fixed in Version1.7.0.svn 
Summary0017189: Using a vector instead of a unique_ptr to an array of ints
Descriptioni was curious, what the improvement with make_unique was, and now i wonder if it's not even simpler to just use a std::vector<int>...

https://gitlab.com/scribus/scribus/-/commit/aa91fc1b16f8b94b00ca9cc01d4d667133e2d964

i attach a patch that also uses .at() to make sure that an ill defined OwnPage causes a crash and not a random behavior.

i also noticed that tocMap is a QMap having keys composed by a comma separated string of a 10 chars counter, and two numbers (or strings) .
it does not matter much, since the map will always be small, but it would probably be better to use a std::vector containing a local defined struct that is sorted once the whole list is filled.

is a patch doing that welcome?
in my eyes, it would lead to better readability of the code...
TagsNo tags attached.
PatchYes

Activities

ale

2024-04-02 06:40

manager  

vector.diff (2,289 bytes)   
diff --git a/scribus/tocgenerator.cpp b/scribus/tocgenerator.cpp
index c5f6c7e9a..e10b87250 100644
--- a/scribus/tocgenerator.cpp
+++ b/scribus/tocgenerator.cpp
@@ -83,9 +83,7 @@ void TOCGenerator::generateByAttribute()
 		const PageItem *currentDocItem;
 		QMap<QString, QString> tocMap;
 
-		auto pageCounter = std::make_unique<int[]>(m_doc->DocPages.count());
-		for (int i = 0; i < m_doc->DocPages.count(); ++i)
-			pageCounter[i] = 0;
+		auto pageCounter = std::vector<int>(m_doc->DocPages.count(), 0);
 
 		int pageNumberWidth = QString("%1").arg(m_doc->DocPages.count()).length();
 
@@ -119,7 +117,7 @@ void TOCGenerator::generateByAttribute()
 				//First is the page of the item
 				//Second is an incremented counter for the item so multiple per page works
 				//Third is the section based page number which is actually used in the TOC.
-				QString tocID = QString("%1").arg(pageCounter[currentDocItem->OwnPage]++, 3 , 10, QChar('0'));
+				QString tocID = QString("%1").arg(pageCounter.at(currentDocItem->OwnPage)++, 3 , 10, QChar('0'));
 				QString key = QString("%1,%2,%3").arg(pageID, tocID, sectionID);
 				tocMap.insert(key, objAttr.value);
 			}
@@ -227,9 +225,7 @@ void TOCGenerator::generateByStyle()
 		QMap<QString, QString> styleMap;
 		QMap<QString, TOCPageLocation> pageLocationMap;
 
-		auto pageCounter = std::make_unique<int[]>(m_doc->DocPages.count());
-		for (int i = 0; i < m_doc->DocPages.count(); ++i)
-			pageCounter[i] = 0;
+		auto pageCounter = std::vector<int>(m_doc->DocPages.count(), 0);
 
 		//Set up the gtWriter instance with the selected paragraph style
 		gtWriter writer(false, tocFrame);
@@ -290,7 +286,7 @@ void TOCGenerator::generateByStyle()
 				QString pname(item->itemText.paragraphStyle(i).parentStyle()->name());
 				QString pageID = QString("%1").arg(item->OwnPage + m_doc->FirstPnum, pageNumberWidth);
 				QString sectionID = m_doc->getSectionPageNumberForPageIndex(item->OwnPage);
-				QString tocID = QString("%1").arg(pageCounter[item->OwnPage]++, 3, 10, QChar('0'));
+				QString tocID = QString("%1").arg(pageCounter.at(item->OwnPage)++, 3, 10, QChar('0'));
 				QString key = QString("%1,%2,%3").arg(pageID, tocID, sectionID);
 				paraText.remove(SpecialChars::COLBREAK);
 				paraText.remove(SpecialChars::FRAMEBREAK);
vector.diff (2,289 bytes)   

ale

2024-04-02 17:58

manager   ~0051079

well, a std::array<int> would probably even be better...

cbradney

2024-04-02 21:55

administrator   ~0051080

How about this? Using a QMap to auto sort seems ok to me given the size of a TOC.

cbradney

2024-04-02 21:56

administrator   ~0051081

tocstruct.diff (6,442 bytes)   
Index: tocgenerator.cpp
===================================================================
--- tocgenerator.cpp	(revision 26090)
+++ tocgenerator.cpp	(working copy)
@@ -20,7 +20,7 @@
  ***************************************************************************/
 #include "tocgenerator.h"
 
-#include <memory>
+#include <vector>
 
 #include <QDebug>
 #include <QMap>
@@ -83,9 +83,7 @@
 		const PageItem *currentDocItem;
 		QMap<QString, QString> tocMap;
 
-		auto pageCounter = std::make_unique<int[]>(m_doc->DocPages.count());
-		for (int i = 0; i < m_doc->DocPages.count(); ++i)
-			pageCounter[i] = 0;
+		auto pageCounter = std::vector<int>(m_doc->DocPages.count(), 0);
 
 		int pageNumberWidth = QString("%1").arg(m_doc->DocPages.count()).length();
 
@@ -119,7 +117,7 @@
 				//First is the page of the item
 				//Second is an incremented counter for the item so multiple per page works
 				//Third is the section based page number which is actually used in the TOC.
-				QString tocID = QString("%1").arg(pageCounter[currentDocItem->OwnPage]++, 3 , 10, QChar('0'));
+				QString tocID = QString("%1").arg(pageCounter.at(currentDocItem->OwnPage)++, 3 , 10, QChar('0'));
 				QString key = QString("%1,%2,%3").arg(pageID, tocID, sectionID);
 				tocMap.insert(key, objAttr.value);
 			}
@@ -170,6 +168,13 @@
 		double yPos; // Absolute y position in document
 	};
 
+	struct TOCEntryData
+	{
+		QString entryText;
+		QString entryStyle;
+		TOCPageLocation entryPageLocation;
+	};
+
 	// Collect all text frames including those placed inside groups;
 	QVector<ItemPosInfo> allTextFramePos;
 	allTextFramePos.reserve(100);
@@ -223,14 +228,9 @@
 		if (tocFrame == nullptr)
 			continue;
 		tocFrame->clearContents();
-		QMap<QString, QString> tocMap;
-		QMap<QString, QString> styleMap;
-		QMap<QString, TOCPageLocation> pageLocationMap;
+		QMap<QString, TOCEntryData> tocMap;
+		auto pageCounter = std::vector<int>(m_doc->DocPages.count(), 0);
 
-		auto pageCounter = std::make_unique<int[]>(m_doc->DocPages.count());
-		for (int i = 0; i < m_doc->DocPages.count(); ++i)
-			pageCounter[i] = 0;
-
 		//Set up the gtWriter instance with the selected paragraph style
 		gtWriter writer(false, tocFrame);
 		writer.setUpdateParagraphStyles(false);
@@ -254,12 +254,6 @@
 			int i = item->firstInFrame();
 			while (i <= item->lastInFrame())
 			{
-				int para_no = item->itemText.nrOfParagraph(i);
-				int para_start = item->itemText.startOfParagraph(para_no);
-				int para_end = item->itemText.endOfParagraph(para_no);
-				// qDebug() << "Paragraph Text:" << item->itemText.text(para_start, para_end - para_start);
-				// qDebug() << "Paragraph start/end:" << para_start << para_end;
-
 				//Empty paragraph, continue
 				if (item->itemText.text(i) == SpecialChars::PARSEP)
 				{
@@ -267,8 +261,13 @@
 					++i;
 					continue;
 				}
+				int para_no = item->itemText.nrOfParagraph(i);
+				int para_start = item->itemText.startOfParagraph(para_no);
+				int para_end = item->itemText.endOfParagraph(para_no);
+				QString paraText = item->itemText.text(para_start, para_end - para_start);
+				// qDebug() << "Paragraph Text:" << paraText;
+				// qDebug() << "Paragraph start/end:" << para_start << para_end;
 
-				QString paraText = item->itemText.text(para_start, para_end - para_start);
 				//Paragraph starts before this frame, eg paragraph wrapped into next frame in chain but is not caused by a FRAMEBREAK, continu
 				if (para_start < item->firstInFrame() && !paraText.startsWith(SpecialChars::FRAMEBREAK))
 				{
@@ -290,20 +289,21 @@
 				QString pname(item->itemText.paragraphStyle(i).parentStyle()->name());
 				QString pageID = QString("%1").arg(item->OwnPage + m_doc->FirstPnum, pageNumberWidth);
 				QString sectionID = m_doc->getSectionPageNumberForPageIndex(item->OwnPage);
-				QString tocID = QString("%1").arg(pageCounter[item->OwnPage]++, 3, 10, QChar('0'));
+				QString tocID = QString("%1").arg(pageCounter.at(item->OwnPage)++, 3, 10, QChar('0'));
 				QString key = QString("%1,%2,%3").arg(pageID, tocID, sectionID);
 				paraText.remove(SpecialChars::COLBREAK);
 				paraText.remove(SpecialChars::FRAMEBREAK);
-				for (QList<ToCSetupEntryStyleData>::Iterator tocEntryIterator = tocSetupIt->entryData.begin();
-					 tocEntryIterator != tocSetupIt->entryData.end(); ++tocEntryIterator)
+				for (auto tocEntryIterator = tocSetupIt->entryData.begin(); tocEntryIterator != tocSetupIt->entryData.end(); ++tocEntryIterator)
 				{
 					if ((*tocEntryIterator).styleToFind == pname)
 					{
 						if ((*tocEntryIterator).removeLineBreaks)
 							paraText.remove(SpecialChars::LINEBREAK);
-						tocMap.insert(key, paraText);
-						styleMap.insert(key, (*tocEntryIterator).styleForText);
-						pageLocationMap.insert(key, (*tocEntryIterator).pageLocation);
+						TOCEntryData ted;
+						ted.entryText = paraText;
+						ted.entryStyle = (*tocEntryIterator).styleForText;
+						ted.entryPageLocation = (*tocEntryIterator).pageLocation;
+						tocMap.insert(key, ted);
 					}
 				}
 				i = item->itemText.startOfNextParagraph(i);
@@ -310,16 +310,14 @@
 			}
 		}
 		QString oldTocPage;
-		for (QMap<QString, QString>::Iterator tocIt = tocMap.begin(); tocIt != tocMap.end(); ++tocIt)
+		for (auto tocIt = tocMap.begin(); tocIt != tocMap.end(); ++tocIt)
 		{
-			QString t = tocIt.key();
-			QString t2 = tocIt.value();
 			QString tocPage(tocIt.key().section(',', 2, 2).trimmed());
 			QString tocLine;
 			//Start with text or numbers
-			TOCPageLocation tpl = pageLocationMap.value(tocIt.key());
+			TOCPageLocation tpl = tocMap.value(tocIt.key()).entryPageLocation;
 			if (tpl == End || tpl == NotShown)
-				tocLine = tocIt.value();
+				tocLine = tocMap.value(tocIt.key()).entryText;
 			if (tpl == Beginning && oldTocPage != tocPage)
 				tocLine = tocPage;
 			//Add in the tab for the leaders
@@ -326,7 +324,7 @@
 			tocLine += "\t";
 			//End with text or numbers
 			if (tpl == Beginning)
-				tocLine += tocIt.value();
+				tocLine += tocMap.value(tocIt.key()).entryText;
 			if (tpl == End && oldTocPage != tocPage)
 				tocLine += tocPage;
 			tocLine += "\n";
@@ -333,7 +331,7 @@
 
 			const gtFrameStyle *fstyle = writer.getDefaultStyle();
 			gtParagraphStyle *pstyle = new gtParagraphStyle(*fstyle);
-			pstyle->setName(styleMap.value(tocIt.key()));
+			pstyle->setName(tocMap.value(tocIt.key()).entryStyle);
 			writer.setParagraphStyle(pstyle);
 			writer.append(tocLine);
 		}
tocstruct.diff (6,442 bytes)   

Issue History

Date Modified Username Field Change
2024-04-02 06:40 ale New Issue
2024-04-02 06:40 ale File Added: vector.diff
2024-04-02 06:57 ale Summary using a vector instead of a unique_ptr to an array of ints => [PATCH] using a vector instead of a unique_ptr to an array of ints
2024-04-02 06:57 ale Patch No => Yes
2024-04-02 17:58 ale Note Added: 0051079
2024-04-02 21:55 cbradney Note Added: 0051080
2024-04-02 21:56 cbradney Note Added: 0051081
2024-04-02 21:56 cbradney File Added: tocstruct.diff
2024-04-08 20:31 cbradney Assigned To => cbradney
2024-04-08 20:31 cbradney Status new => resolved
2024-04-08 20:31 cbradney Resolution open => fixed
2024-04-08 20:31 cbradney Fixed in Version => 1.7.0.svn
2024-04-09 23:06 jghali Summary [PATCH] using a vector instead of a unique_ptr to an array of ints => Using a vector instead of a unique_ptr to an array of ints