View Issue Details

IDProjectCategoryView StatusLast Update
0015941ScribusGeneralpublic2019-12-08 21:24
Reporterale Assigned Toale  
PrioritynormalSeverityfeatureReproducibilityN/A
Status closedResolutionfixed 
Product Version1.5.6.svn 
Fixed in Version1.5.6.svn 
Summary0015941: Use the "name (#)" pattern when duplicating the master pages
Descriptiona former (resolved) ticket explained why it would be good to avoid "Copy of name" for renaming styles.

the same applies for master pages...
TagsNo tags attached.
PatchYes

Activities

ale

2019-11-13 11:50

manager   ~0047064

i suggest to

- create functions for checking for standard names and rename
- if possible avoid message boxes when something does not work (most of all when it's impossible that something can happen...)

ale

2019-11-13 12:50

manager   ~0047065

- also "simplifies" the checks for new master pages.
- needs 0015940 to be applied for getUniqueName().
- it won't show message boxes anymore when conflicts arises but, depending on the context, will use the "name (#)" pattern or fall back to the old name.
- adds functions for checking for reserved names.
- contains the suggestion to move all masterPages logic from the dialog and scribusdoc to a new masterpage.h|cpp.
masterpages-conflicting-names.diff (13,912 bytes)   
diff --git a/scribus/ui/pagepalette_masterpages.cpp b/scribus/ui/pagepalette_masterpages.cpp
index 24f2b4569..1ff60ec58 100644
--- a/scribus/ui/pagepalette_masterpages.cpp
+++ b/scribus/ui/pagepalette_masterpages.cpp
@@ -16,6 +16,8 @@ for which a new license (GPL+exception) is in place.
 #include <QMessageBox>
 #include <QScopedPointer>
 #include <QSpacerItem>
+#include <QString>
+#include <QStringList>
 #include <QToolButton>
 #include <QToolTip>
 #include <QVBoxLayout>
@@ -34,6 +36,7 @@ for which a new license (GPL+exception) is in place.
 #include "scribusdoc.h"
 #include "scribusview.h"
 #include "selection.h"
+#include "util.h"
 #include "util_os.h"
 
 
@@ -151,11 +154,7 @@ void PagePalette_MasterPages::deleteMasterPage()
 	{
 		m_currentPage = delItem->text();
 
-		if ((m_currentPage == CommonStrings::masterPageNormal)
-			|| (m_currentPage == CommonStrings::trMasterPageNormal)
-			|| (m_currentPage == CommonStrings::trMasterPageNormalLeft)
-			|| (m_currentPage == CommonStrings::trMasterPageNormalMiddle)
-			|| (m_currentPage == CommonStrings::trMasterPageNormalRight))
+		if (isReservedName(m_currentPage))
 			continue;
 
 		QString extraWarn = "";
@@ -201,43 +200,27 @@ void PagePalette_MasterPages::deleteMasterPage()
 
 void PagePalette_MasterPages::duplicateMasterPage()
 {
-	int copyC = 1;
-	QString potentialMasterPageName(m_currentPage);
-	while (m_doc->MasterNames.contains(potentialMasterPageName))
-		potentialMasterPageName = tr("Copy #%1 of %2").arg(copyC++).arg(m_currentPage);
-
-	QScopedPointer<NewTm> dia(new NewTm(this, tr("&Name:"), tr("New Master Page"), m_doc, potentialMasterPageName));
+	QString name = getUniqueName(m_currentPage, m_doc->MasterNames);
+	QScopedPointer<NewTm> dia(new NewTm(this, tr("&Name:"), tr("New Master Page"), m_doc, name));
 	if (!dia->exec())
 		return;
 
 	if (m_doc->appMode == modeEditClip)
 		m_view->requestMode(submodeEndNodeEdit);
-	QString masterPageName = dia->Answer->text();
-	bool masterPageNameWrong = m_doc->MasterNames.contains(masterPageName);
-	masterPageNameWrong |= (masterPageName == CommonStrings::masterPageNormal);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormal);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalLeft);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalMiddle);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalRight);
-	masterPageNameWrong |=  masterPageName.isEmpty();
-	while (masterPageNameWrong)
-	{
-		if (!dia->exec())
-			return;
-		masterPageName = dia->Answer->text();
-		masterPageNameWrong = m_doc->MasterNames.contains(masterPageName);
-		masterPageNameWrong |= (masterPageName == CommonStrings::masterPageNormal);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormal);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalLeft);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalMiddle);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalRight);
-		masterPageNameWrong |=  masterPageName.isEmpty();
-	}
+
+	name = dia->Answer->text();
+
+	if (name.isEmpty())
+		return;
+
+	name = getNonReservedName(name);
+	name = getUniqueName(name, m_doc->MasterNames);
+
 	PrefsManager& prefsManager = PrefsManager::instance();
 	int inde = m_doc->MasterNames[m_currentPage];
 	int nr = m_doc->Pages->count();
 	ScPage* from = m_doc->Pages->at(inde);
-	ScPage* destination = m_doc->addMasterPage(nr, masterPageName);
+	ScPage* destination = m_doc->addMasterPage(nr, name);
 	if (m_doc->pagePositioning() != singlePage)
 	{
 		int lp = dia->Links->currentIndex();
@@ -279,8 +262,8 @@ void PagePalette_MasterPages::duplicateMasterPage()
 	}
 	//#8321 : incorrect selection of master page on new mp creation/duplication
 	//m_doc->setCurrentPage(destination);
-	updateMasterPageList(masterPageName);
-	selectMasterPage(masterPageName);
+	updateMasterPageList(name);
+	selectMasterPage(name);
 	uint oldItems = m_doc->Items->count();
 	uint end2 = m_doc->MasterItems.count();
 	int GrMax = m_doc->GroupCounter;
@@ -312,8 +295,8 @@ void PagePalette_MasterPages::duplicateMasterPage()
 	for (uint a = end2; a < end3; ++a)
 	{
 		PageItem *newItem = m_doc->MasterItems.at(a);
-		int masterPageIndex = m_doc->MasterNames[masterPageName];
-		newItem->setMasterPage(masterPageIndex, masterPageName);
+		int masterPageIndex = m_doc->MasterNames[name];
+		newItem->setMasterPage(masterPageIndex, name);
 	}
 	from->guides.copy(&destination->guides);
 	m_doc->GroupCounter = GrMax + 1;
@@ -325,7 +308,6 @@ void PagePalette_MasterPages::duplicateMasterPage()
 
 void PagePalette_MasterPages::newMasterPage()
 {
-	QString masterPageName;
 	int nr = m_doc->Pages->count();
 
 	QScopedPointer<NewTm> dia(new NewTm(this, tr("Name:"), tr("New MasterPage"), m_doc, tr("New Master Page %1").arg(nr)));
@@ -334,28 +316,16 @@ void PagePalette_MasterPages::newMasterPage()
 
 	if (m_doc->appMode == modeEditClip)
 		m_view->requestMode(submodeEndNodeEdit);
-	masterPageName = dia->Answer->text();
-	bool masterPageNameWrong = m_doc->MasterNames.contains(masterPageName);
-	masterPageNameWrong |= (masterPageName == CommonStrings::masterPageNormal);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormal);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalLeft);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalMiddle);
-	masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalRight);
-	masterPageNameWrong |=  masterPageName.isEmpty();
-	while (masterPageNameWrong)
-	{
-		if (!dia->exec())
-			return;
-		masterPageName = dia->Answer->text();
-		masterPageNameWrong = m_doc->MasterNames.contains(masterPageName);
-		masterPageNameWrong |= (masterPageName == CommonStrings::masterPageNormal);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormal);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalLeft);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalMiddle);
-		masterPageNameWrong |= (masterPageName == CommonStrings::trMasterPageNormalRight);
-		masterPageNameWrong |=  masterPageName.isEmpty();
-	}
-	m_doc->setCurrentPage(m_doc->addMasterPage(nr, masterPageName));
+
+	auto name = dia->Answer->text();
+
+	if (name.isEmpty())
+		return;
+
+	name = getNonReservedName(name);
+	name = getUniqueName(name, m_doc->MasterNames);
+
+	m_doc->setCurrentPage(m_doc->addMasterPage(nr, name));
 	if (m_doc->pagePositioning() != singlePage)
 	{
 		int lp = dia->Links->currentIndex();
@@ -367,10 +337,10 @@ void PagePalette_MasterPages::newMasterPage()
 			lp++;
 		m_doc->Pages->at(nr)->LeftPg = lp;
 	}
-	updateMasterPageList(masterPageName);
+	updateMasterPageList(name);
 	//#8321 : incorrect selection of master page on new mp creation/duplictation
-	//m_view->showMasterPage(m_doc->MasterNames[masterPageName]);
-	selectMasterPage(masterPageName);
+	//m_view->showMasterPage(m_doc->MasterNames[name]);
+	selectMasterPage(name);
 	m_view->reformPages();
 }
 
@@ -477,21 +447,16 @@ void PagePalette_MasterPages::updateMasterPageList(QString masterPageName)
 	for (QMap<QString,int>::Iterator it = m_doc->MasterNames.begin(); it != m_doc->MasterNames.end(); ++it)
 	{
 		QString mpName = it.key();
-//		qDebug() << mpName;
+
 		if (it.key() == CommonStrings::masterPageNormal)
 			mpName = CommonStrings::trMasterPageNormal;
 		QListWidgetItem* mpItem = new QListWidgetItem(mpName);
-		if ((mpName != CommonStrings::masterPageNormal)  && (mpName != CommonStrings::trMasterPageNormal) &&
-			(mpName != CommonStrings::masterPageNormalLeft)  && (mpName != CommonStrings::trMasterPageNormalLeft) &&
-			(mpName != CommonStrings::masterPageNormalMiddle) && (mpName != CommonStrings::trMasterPageNormalMiddle) &&
-			(mpName != CommonStrings::masterPageNormalRight)  && (mpName != CommonStrings::trMasterPageNormalRight))
-		{
+
+		if (!isReservedName(mpName))
 			mpItem->setFlags(mpItem->flags() |= Qt::ItemIsEditable);
-		}
 		else
-		{
 			mpItem->setFlags(mpItem->flags() &= ~Qt::ItemIsEditable);
-		}
+
 		mpItem->setData(Qt::UserRole, it.key());
 		masterPageListBox->addItem(mpItem);
 	}
@@ -508,45 +473,48 @@ void PagePalette_MasterPages::updateMasterPageList(QString masterPageName)
 
 void PagePalette_MasterPages::renameMasterPage(QListWidgetItem * item)
 {
-	bool success = true;
 	QString oldName(item->data(Qt::UserRole).toString());
 	QString newName(item->text());
+
 	if (oldName == newName)
 		return;
 
-	bool sigBlocked = masterPageListBox->blockSignals(true);
-	if ((newName == CommonStrings::masterPageNormal) || (newName == CommonStrings::trMasterPageNormal) ||
-		(newName == CommonStrings::masterPageNormalLeft)   || (newName == CommonStrings::trMasterPageNormalLeft) ||
-		(newName == CommonStrings::masterPageNormalMiddle) || (newName == CommonStrings::trMasterPageNormalMiddle) ||
-		(newName == CommonStrings::masterPageNormalRight)  || (newName == CommonStrings::trMasterPageNormalRight))
+	// FIXME: the checks in the old version of this function
+	// and the ones in m_doc->renameMasterPage() did not match.
+	// we should move all MasterPage related code to a new masterpage.h
+	// file and remove code from scribusdoc.cpp and from here.
+	// at the end, the code below should be:
+	//
+	// if (m_doc->m_masterPage->rename(oldName, newName))
+	// {
+	//		bool sigBlocked = masterPageListBox->blockSignals(true);
+	//		item->setData(Qt::UserRole, newName);
+	//		masterPageListBox->blockSignals(sigBlocked);
+	//		updateMasterPageList(newName);
+	// }
+	// else
+	// {
+	//		bool sigBlocked = masterPageListBox->blockSignals(true);
+	//		item->setText(getTranslatedReservedName(oldName));
+	//		masterPageListBox->blockSignals(sigBlocked);
+	// }
+
+	if (newName.isEmpty() || isReservedName(newName) ||
+		!m_doc->MasterNames.contains(oldName) ||
+		m_doc->MasterNames.contains(newName))
 	{
-		ScMessageBox::information( this, tr("Unable to Rename Master Page"), tr("The Normal page is not allowed to be renamed."), QMessageBox::Ok );
-		success = false;
+		bool sigBlocked = masterPageListBox->blockSignals(true);
+		item->setText(oldName);
+		masterPageListBox->blockSignals(sigBlocked);
 	}
-	if (newName.isEmpty())
-		success = false;
-
-	if (success)
-		success = m_doc->renameMasterPage(oldName, newName);
-	if (success)
+	else
 	{
+		bool sigBlocked = masterPageListBox->blockSignals(true);
+		m_doc->renameMasterPage(oldName, newName);
 		item->setData(Qt::UserRole, newName);
 		masterPageListBox->blockSignals(sigBlocked);
 		updateMasterPageList(newName);
-		return;
 	}
-
-	QString masterPageName = oldName;
-	if (masterPageName == CommonStrings::masterPageNormal)
-		masterPageName = CommonStrings::trMasterPageNormal;
-	if (masterPageName == CommonStrings::masterPageNormalLeft)
-		masterPageName = CommonStrings::trMasterPageNormalLeft;
-	if (masterPageName == CommonStrings::masterPageNormalMiddle)
-		masterPageName = CommonStrings::trMasterPageNormalMiddle;
-	if (masterPageName == CommonStrings::masterPageNormalRight)
-		masterPageName = CommonStrings::trMasterPageNormalRight;
-	item->setText(masterPageName);
-	masterPageListBox->blockSignals(sigBlocked);
 }
 
 
@@ -565,3 +533,43 @@ void PagePalette_MasterPages::changeEvent(QEvent *e)
 
 	QWidget::changeEvent(e);
 }
+
+// FIXME: move to a new masterpages.h
+bool PagePalette_MasterPages::isReservedName(const QString& name) const
+{
+	return name == CommonStrings::masterPageNormal ||
+		name == CommonStrings::trMasterPageNormal ||
+		name == CommonStrings::masterPageNormalLeft ||
+		name == CommonStrings::trMasterPageNormalLeft ||
+		name == CommonStrings::masterPageNormalMiddle ||
+		name == CommonStrings::trMasterPageNormalMiddle ||
+		name == CommonStrings::masterPageNormalRight ||
+		name == CommonStrings::trMasterPageNormalRight;
+}
+
+// FIXME: move to a new masterpages.h
+QString PagePalette_MasterPages::getNonReservedName(const QString& name) const
+{
+	const QStringList reservedNames = {
+		CommonStrings::masterPageNormal,
+		CommonStrings::trMasterPageNormal,
+		CommonStrings::trMasterPageNormalLeft,
+		CommonStrings::trMasterPageNormalMiddle,
+		CommonStrings::trMasterPageNormalRight
+	};
+	return getUniqueName(name, reservedNames);
+}
+
+// FIXME: move to a new masterpages.h
+QString PagePalette_MasterPages::getTranslatedReservedName(const QString& name) const
+{
+	if (name == CommonStrings::masterPageNormal)
+		return CommonStrings::trMasterPageNormal;
+	if (name == CommonStrings::masterPageNormalLeft)
+		return CommonStrings::trMasterPageNormalLeft;
+	if (name == CommonStrings::masterPageNormalMiddle)
+		return CommonStrings::trMasterPageNormalMiddle;
+	if (name == CommonStrings::masterPageNormalRight)
+		return CommonStrings::trMasterPageNormalRight;
+	return name;
+}
diff --git a/scribus/ui/pagepalette_masterpages.h b/scribus/ui/pagepalette_masterpages.h
index 67e20077c..9939e4b60 100644
--- a/scribus/ui/pagepalette_masterpages.h
+++ b/scribus/ui/pagepalette_masterpages.h
@@ -51,7 +51,14 @@ protected:
 	void connectSignals();
 	void disconnectSignals();
 
-	virtual void changeEvent(QEvent *e);
+	void changeEvent(QEvent *e) override;
+
+	bool isReservedName(const QString& name) const;
+	QString getNonReservedName(const QString& name) const;
+	/*!
+	 * \retval the current name or, if it's a standard one, the translated version
+	*/
+	QString getTranslatedReservedName(const QString& name) const;
 
 private slots:
 	void duplicateMasterPage();

Issue History

Date Modified Username Field Change
2019-11-13 11:49 ale New Issue
2019-11-13 11:50 ale Note Added: 0047064
2019-11-13 12:50 ale File Added: masterpages-conflicting-names.diff
2019-11-13 12:50 ale Note Added: 0047065
2019-11-13 12:50 ale Summary use the "name (#)" pattern when duplicating the master pages => [PATCH] use the "name (#)" pattern when duplicating the master pages
2019-11-13 12:50 ale Patch No => Yes
2019-11-13 12:52 ale Assigned To => ale
2019-11-13 12:52 ale Status new => assigned
2019-11-13 22:24 cbradney Status assigned => resolved
2019-11-13 22:24 cbradney Fixed in Version => 1.5.6.svn
2019-11-13 22:24 cbradney Resolution open => fixed
2019-11-13 22:27 jghali Summary [PATCH] use the "name (#)" pattern when duplicating the master pages => Use the "name (#)" pattern when duplicating the master pages
2019-12-08 21:24 cbradney Status resolved => closed