View Issue Details

IDProjectCategoryView StatusLast Update
0002569ScribusPlug-inspublic2005-10-05 08:15
Reporterintrigeri Assigned Tosubik  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
PlatformppcOSDebianOS Versionsarge
Product Version1.3.1cvs 
Fixed in Version1.3.2cvs 
Summary0002569: Short-words: punctuation is not handled
Description1.3cvs short-words plugin includes a rc file with some punctuation
signs in it; example:
fr= €,« , », :,

But... it doesn't work, since the regexp used is based on word
boundaries, and... such punctuation signs are not delimited by word
boundaries. And are thus not matched.
Additional InformationThat's why I hacked this plugin (patch attached against today's CVS)
so that it's possible to have, for example, this config file:
fr= cm, mm, hl, %, °, ?, !, :, ;, €,« , »,

There are two problems with my patch :
   * some corner cases are still not managed: '_!_»' will not be
     recognized, for example; but this non-working exceptions pool is
     now far smaller;
   * it's now slower: text has to be parsed 8 times instead of one,
     because of the impossibility to use optional match in the regexp
     (ex: ( )?), due to Qt "captured chars" handling.

I've used this patched plugin without any problem on a 48-pages
booklet.

Please consider applying it to CVS, or at least, tell me what are the
problems with it, since I don't use C++ usually and it's the first
time I use Qt.
TagsNo tags attached.
Patch

Activities

2005-09-16 00:59

 

short-words_cvs20050915.diff (2,019 bytes)   
--- Scribus/scribus/plugins/short-words/parse.cpp	2005-09-11 23:48:35.000000000 +0200
+++ Scribus-work/scribus/plugins/short-words/parse.cpp	2005-09-15 22:32:42.000000000 +0200
@@ -47,7 +47,9 @@
 	QRegExp rx(" ");
 	// cfg
 	Config *cfg = new Config();
-
+	// list of pattern to replace
+	QStringList patternList;
+	
 	// just textframes processed
 	if (aFrame->itemType() != PageItem::TextFrame)
 		return;
@@ -71,21 +73,36 @@
 		unbreak = (*it);
 		// replace ' ' from cfg with '~'
 		unbreak = unbreak.replace(SPACE, UNBREAKABLE_SPACE);
-		//looking for pattern with word boundaries and more chars
-		// replace hell needed too to remove regexp special chars
-		rx.setPattern("(\\b)" + (*it).replace("*", "\\*").replace("+", "\\+").replace("-", "\\-").replace("$", "\\$").replace(".","\\.") + "(\\b)");
-		// replacement loop
+
 		int pos = 0;
-		while ( pos >= 0 )
+
+		patternList = ("(\\b)" + rx.escape(*it) + "(\\b)");
+		patternList += "(\\b)" + rx.escape(*it) + "(\\r)";
+		patternList += "(\\r)" + rx.escape(*it) + "(\\b)";
+		patternList += ("(\\b)" + rx.escape(*it) + "( )");
+		patternList += ("( )" + rx.escape(*it) + "(\\b)");
+		patternList += "( )" + rx.escape(*it) + "(\\r)";
+		patternList += "(\\r)" + rx.escape(*it) + "( )";
+		patternList += ("( )" + rx.escape(*it) + "( )");
+
+		for (QStringList::Iterator patternIt = patternList.begin(); patternIt != patternList.end(); ++patternIt)
 		{
-			pos = rx.search(content, pos);
-			if ( pos >= 0 )
+			//looking for pattern
+			rx.setPattern(*patternIt);
+			// replacement loop
+			pos = 0;
+			while ( pos >= 0 )
 			{
-				//replace + keep word boundaries
-				content.replace(rx, rx.cap(1) + unbreak + rx.cap(2));
-				pos  += rx.matchedLength();
+				pos = rx.search(content, pos);
+				if ( pos >= 0 )
+				{
+					//replace + keep word boundaries et al.
+					content.replace(rx, rx.cap(1) + unbreak + rx.cap(2));
+					pos  += rx.matchedLength();
+				}
 			}
 		}
+
 	}
 	// retrun text into frame
 	for (uint i=0; i<aFrame->MaxChars; ++i)
short-words_cvs20050915.diff (2,019 bytes)   

ringerc

2005-09-16 05:15

reporter   ~0006600

Hi. Thanks for the patch. Petr, who looks after short words, should have a look at it. I'll offer a few quick comments, though.

If you're trying to group without capturing in a QRegExp, you can use the:

(?:something|somethingelse)

syntax. See file:///usr/share/doc/qt-devel-3.3.4/html/qregexp.html .

Good to see the clean escaping.

Also, please document what each regexp snippet is for in the patch. It's extremely annoying to have to read and figure out code that uses regex when you're just trying to follow the general path of some code, and commenting the function of the regex helps a lot.

subik

2005-09-16 06:07

manager   ~0006601

thanks intrigeri, can you comment the code as ringierc says? I have one more wish on you - can you upload your sample file (booklet) without short-words applied, please?

intrigeri

2005-09-16 12:55

reporter   ~0006604

Thanks for all these constructive comments :)

I'll have a look to QRegExp doc (which I had unfortunately not on my computer, and was offline when writting this patch), correct what needs to be corrected, and document the regexp snippets.

subik: I'll send you this privately.

I'll put all this here in 10 days, when I'm back online.

2005-09-30 09:40

 

short-words_cvs20050924.diff (2,485 bytes)   
--- Scribus/scribus/plugins/short-words/parse.cpp	2005-09-11 23:48:35.000000000 +0200
+++ Scribus-work/scribus/plugins/short-words/parse.cpp	2005-09-27 18:27:02.000000000 +0200
@@ -65,27 +65,34 @@
 	for (uint i=0; i<aFrame->MaxChars; ++i)
 		content += aFrame->itemText.at(i)->ch;
 	changes = content.contains(UNBREAKABLE_SPACE);
-	// replace it
+
+	// for every config string, replace its spaces by nbsp's.
 	for (QStringList::Iterator it = shorts.begin(); it != shorts.end(); ++it)
 	{
 		unbreak = (*it);
-		// replace ' ' from cfg with '~'
+		// replace ' ' from cfg with '~' in the replacement string
 		unbreak = unbreak.replace(SPACE, UNBREAKABLE_SPACE);
-		//looking for pattern with word boundaries and more chars
-		// replace hell needed too to remove regexp special chars
-		rx.setPattern("(\\b)" + (*it).replace("*", "\\*").replace("+", "\\+").replace("-", "\\-").replace("$", "\\$").replace(".","\\.") + "(\\b)");
-		// replacement loop
-		int pos = 0;
-		while ( pos >= 0 )
-		{
-			pos = rx.search(content, pos);
-			if ( pos >= 0 )
-			{
-				//replace + keep word boundaries
-				content.replace(rx, rx.cap(1) + unbreak + rx.cap(2));
-				pos  += rx.matchedLength();
-			}
-		}
+
+		// Regexp used to find the config string (*it) in content.
+		// Cheat sheet:
+		//   - \b is a "word boundary"; it matches at a *position* not a *character*
+		//   - \W is a "non-word character"; it matches every character that is neither a letter, nor a
+		//     number, nor '_'; for example, it matches all kind of whitespace (including carriage
+		//     return) and punctuation
+		// Example occurrences when (*it) == "Mr ":
+		//   - "Mr Bla etc." : there's one of the word boundaries of the word "Mr" before the pattern,
+		//     and one of the word boundaries of the word "Bla" after.
+		// Example occurrences when (*it) == " !":
+		//   - "ugly hack ! No." : there's a word boundary before, and a whitespace is matched
+		//      by \W after.
+		//   - "» !
+		//      " : '«' is matched by \W before, newline is matched by \W after.
+	        rx.setPattern("(\\b|\\W)" + rx.escape(*it) + "(\\b|\\W)");
+		// QString::replace works on the whole string in one pass.
+		// On every occurrence of our regexp, \1 and \2 are replaced by what has been matched
+		// (captured characters) in, respectively, the first and second capturing parentheses.
+		content.replace(rx, "\\1" + unbreak + "\\2");
+
 	}
 	// retrun text into frame
 	for (uint i=0; i<aFrame->MaxChars; ++i)
short-words_cvs20050924.diff (2,485 bytes)   

intrigeri

2005-09-30 09:44

reporter   ~0006819

Hi. The patch I just uploaded (short-words_cvs20050924.diff) applies cleanly on today's CVS, and:
   * only parses the text once;
   * has comments for the regexp;
   * removes an unnecesseray loop (no need to rx.search since QString::replace works on the whole string in one pass.

What do you think of it?

intrigeri

2005-09-30 10:23

reporter   ~0006821

Oh, I forgot: the corner cases that were not handled (such as '_!_»') are not correctly dealt with :)

subik

2005-10-01 09:49

manager   ~0006829

intrigerim, thank you for the patch. It's applied in devel CVS - it will be public tomorrow. It works fine here. Your mentioned speed issue is not problem (IMHO) because SW are applied only once (2x, 3x) in document workflow so DTP operator can wait a while ;)

BTW I've modified you patch a bit to fit coding standards.
Good work.

Issue History

Date Modified Username Field Change
2005-09-16 00:59 intrigeri New Issue
2005-09-16 00:59 intrigeri File Added: short-words_cvs20050915.diff
2005-09-16 05:15 ringerc Note Added: 0006600
2005-09-16 05:15 ringerc Status new => acknowledged
2005-09-16 06:03 subik Status acknowledged => assigned
2005-09-16 06:03 subik Assigned To => subik
2005-09-16 06:07 subik Note Added: 0006601
2005-09-16 12:55 intrigeri Note Added: 0006604
2005-09-30 09:40 intrigeri File Added: short-words_cvs20050924.diff
2005-09-30 09:44 intrigeri Note Added: 0006819
2005-09-30 10:23 intrigeri Note Added: 0006821
2005-10-01 09:49 subik Status assigned => resolved
2005-10-01 09:49 subik Fixed in Version => 1.3.1cvs
2005-10-01 09:49 subik Resolution open => fixed
2005-10-01 09:49 subik Note Added: 0006829
2005-10-04 22:28 cbradney Status resolved => closed
2005-10-05 08:15 plinnell Fixed in Version 1.3.1cvs => 1.3.2cvs