View Issue Details

IDProjectCategoryView StatusLast Update
0011163ScribusInternalpublic2016-04-05 19:59
Reporterale Assigned To 
PrioritynormalSeverityfeatureReproducibilityN/A
Status newResolutionopen 
Product Version1.5.0svn 
Summary0011163: [PATCH] Simplify and extend QString FormatsManager::extensionListForFormat()
DescriptionFor the Epub plugin I need a list of extensions attached to a file format.
Instead of splitting the result of the current extensionListForFormat(), I've reimplemented it to both return a QStringList or a QString, depending on the number of parameters.

I've taken the chance to massively simplify the existing method and use the tools already provided by QMap and QString.
I don't think that there should be any performance concerns that would justify to reimplement value(), join() and trimmed() (if ever the current implementation was faster than the one of mine).

As noted in the comment, in the future, I think that the method returning a QString should be renamed and only be used for the "*.jpg *.jpeg" type of lists.
This would further simplify it and it's really not that hard for the caller to do extensionListForFormat(1).join("|"). IMO it's even much more readable then the current way (setting the second parameter to 1 or any other number...)
Tagsepub
PatchYes

Activities

ale

2012-11-11 12:10

manager  

util_formats.patch (2,838 bytes)   
Index: scribus/util_formats.cpp
===================================================================
--- scribus/util_formats.cpp	(revision 17837)
+++ scribus/util_formats.cpp	(working copy)
@@ -172,44 +172,29 @@
 	return a + b + ";;" +c;
 }
 
+QStringList FormatsManager::extensionListForFormat(int type)
+{
+	QStringList result;
+	if ((JPEG & type) && !m_supportedImageFormats.contains(QByteArray("jpg")))
+		return result;
+	if ((GIF & type) && !m_supportedImageFormats.contains(QByteArray("gif")))
+		return result;
+	if (m_fmts.contains(type))
+		result = m_fmts.value(type);
+	return result;
+}
+
 QString FormatsManager::extensionListForFormat(int type, int listType)
 {
-	QString nameMatch;
-	QString separator(listType==0 ? " *." : "|");
-	QMapIterator<int, QStringList> it(m_fmts);
-	bool first=true;
-	int n=0;
-	while (it.hasNext())
-	{
-		it.next();
-		if (type & it.key())
-		{
-			//Just in case the Qt used doesn't support jpeg or gif
-			if ((JPEG & it.key()) && !m_supportedImageFormats.contains(QByteArray("jpg")))
-				continue;
-			if ((GIF & it.key()) && !m_supportedImageFormats.contains(QByteArray("gif")))
-				continue;
-			if (first)
-				first=false;
-			QStringListIterator itSL(it.value());
-			while (itSL.hasNext())
-			{
-				if (listType==0)
-					nameMatch += separator;
-				nameMatch += itSL.next();
-				if (listType==1 && itSL.hasNext())
-					nameMatch += separator;
-			}
-		}
-		++n;
-		if (listType==1 && it.hasNext() && nameMatch.length()>0 && !nameMatch.endsWith(separator))
-			nameMatch += separator;
-	}
-	if (listType==0 && nameMatch.startsWith(" "))
-		nameMatch.remove(0,1);
-	if (listType==1 && nameMatch.endsWith("|"))
-		nameMatch.chop(1);
-	return nameMatch;
+	// TODO: This method should probably be renamed and only used to return the "*." type of lists
+	// For the "piped ones" it's probably easier and faster to directly call the method above and do a join
+	// (ale/20121111)
+	QString result;
+	QString separator(listType == 0 ? " *." : "|");
+	result = extensionListForFormat(type).join(separator).trimmed();
+	if (listType == 0)
+		result = "*." + result;
+	return result;
 }
 
 void FormatsManager::fileTypeStrings(int type, QString& formatList, QString& formatText, QString& formatAll, bool lowerCaseOnly)
Index: scribus/util_formats.h
===================================================================
--- scribus/util_formats.h	(revision 17837)
+++ scribus/util_formats.h	(working copy)
@@ -121,6 +121,8 @@
 		//! Returns in the form of "EPS (*.eps *.EPS *.epsf *.EPSF *.epsi *.EPSI)"
 		QString extensionsForFormat(int type);
 		
+		//! Returns a list of extensions matching the type
+ 		QStringList extensionListForFormat(int type);
 		//! Returns in the form of "*.eps *.epsf *.epsi" or "eps|epsf|epsi"
  		QString extensionListForFormat(int type, int listType);
 		
util_formats.patch (2,838 bytes)   

ale

2012-11-11 14:06

manager   ~0029189

i finally got the right indenting...

jghali

2012-11-25 12:35

administrator   ~0029269

The patch won't work in the case where a set of various format is passed as input parameters. See for eg paintmanager.cpp line 1049 :

extensionListForFormat(FormatsManager::EPS|FormatsManager::PS|FormatsManager::AI, 0)

So the while loop cannot be avoided...

ale

2012-11-27 08:29

manager   ~0029282

looks like a good way to make simple things complex and complex things simple...

in my eyes, the method should be simplified and eventually a second method should be added for the more complex ones (using the first method and then merging the results).

Issue History

Date Modified Username Field Change
2012-11-11 12:04 ale New Issue
2012-11-11 12:04 ale File Added: util_formats.patch
2012-11-11 12:08 ale File Deleted: util_formats.patch
2012-11-11 12:08 ale File Added: util_formats.patch
2012-11-11 12:10 ale File Deleted: util_formats.patch
2012-11-11 12:10 ale File Added: util_formats.patch
2012-11-11 14:06 ale Note Added: 0029189
2012-11-25 12:35 jghali Note Added: 0029269
2012-11-27 08:29 ale Note Added: 0029282
2014-10-24 23:00 Kunda Patch => Yes
2016-04-05 19:59 Kunda Tag Attached: epub
2016-04-05 19:59 Kunda Category General => Internal