View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002569 | Scribus | Plug-ins | public | 2005-09-16 00:59 | 2005-10-05 08:15 |
Reporter | intrigeri | Assigned To | subik | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Platform | ppc | OS | Debian | OS Version | sarge |
Product Version | 1.3.1cvs | ||||
Fixed in Version | 1.3.2cvs | ||||
Summary | 0002569: Short-words: punctuation is not handled | ||||
Description | 1.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 Information | That'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. | ||||
Tags | No tags attached. | ||||
Patch | |||||
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) |
|
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. |
|
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? |
|
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) |
|
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? |
|
Oh, I forgot: the corner cases that were not handled (such as '_!_»') are not correctly dealt with :) |
|
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. |
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 |
|
Note Added: 0006600 | |
2005-09-16 05:15 |
|
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 |
|
Fixed in Version | 1.3.1cvs => 1.3.2cvs |