View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0017189 | Scribus | General | public | 2024-04-02 06:40 | 2024-09-16 20:54 |
| Reporter | ale | Assigned To | cbradney | ||
| Priority | normal | Severity | minor | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Product Version | 1.7.0.svn | ||||
| Fixed in Version | 1.7.0.svn | ||||
| Summary | 0017189: Using a vector instead of a unique_ptr to an array of ints | ||||
| Description | i 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... | ||||
| Tags | No tags attached. | ||||
| Attached Files | 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);
| ||||
| Patch | Yes | ||||
|
|
well, a std::array<int> would probably even be better... |
|
|
How about this? Using a QMap to auto sort seems ok to me given the size of a TOC. |
|
|
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);
}
|
| 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 |
| 2024-09-16 20:54 | cbradney | Status | resolved => closed |