Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Hi John, Korrawit, On Wed, 2011-07-06 at 22:51 +0700, Korrawit Pruegsanusak wrote: On Fri, Jun 24, 2011 at 14:38, John LeMoyne Castle lemoyne.cas...@gmail.com wrote: -- you (and others) cannot reproduce fdo#37584 (redlined text disappears) in 3-3 or 3-3-3 because my patch referenced in your original post [OP] is not in the 3-3 branches. Hmm? I think it's caused by casting var: String rTextCopy = const_castString(m_Text); which Cedric fixed in http://cgit.freedesktop.org/libreoffice/writer/commit/?id=135cf4fdbec71e8d93edc0339e8617d50766f151 Anyway, if it can't be reproduced, so let it be. (should not have further fix for -3-3) You're both right... blame me for leading you into error because I haven't had a look at the 3-3 branch code before writing my first email. The copy mentioned by Korrawit isn't in the 3-3 branch... which explains why the bug can't be reproduced there ;) -- Cédric Bosdonnat LibreOffice hacker http://documentfoundation.org OOo Eclipse Integration developer http://cedric.bosdonnat.free.fr ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Hello John, all, First, sorry for my late reply. On Fri, Jun 24, 2011 at 14:38, John LeMoyne Castle lemoyne.cas...@gmail.com wrote: -- you (and others) cannot reproduce fdo#37584 (redlined text disappears) in 3-3 or 3-3-3 because my patch referenced in your original post [OP] is not in the 3-3 branches. Hmm? I think it's caused by casting var: String rTextCopy = const_castString(m_Text); which Cedric fixed in http://cgit.freedesktop.org/libreoffice/writer/commit/?id=135cf4fdbec71e8d93edc0339e8617d50766f151 Anyway, if it can't be reproduced, so let it be. (should not have further fix for -3-3) -- you and others cannot reproduce 'leading quote as word' in your 3-3 build because the patch you submitted with OP has fixed that problem in 3-3 since 2011-06-21. Your OP patch is not yet in 3-3-3 official build so the leading quote problem still exists there. Sorry, but currently I can't test it. I'm using another computer. I'll come back to this topic again. (fdo#33774) Or, please someone could help confirm this? I think that the two patches given in your last message are already in 3-3(-3). No, they both don't. Please have a look at: http://cgit.freedesktop.org/libreoffice/writer/log/sw/source/core/txtnode/txtedt.cxx?h=libreoffice-3-3 and http://cgit.freedesktop.org/libreoffice/writer/log/sw/source/core/txtnode/txtedt.cxx which the former is in -3-3 branch, the latter is in master. Note that there is *no* Thomas's patch in -3-3. (See the date). Also, http://cgit.freedesktop.org/libreoffice/libs-gui/log/i18npool/source/breakiterator/breakiteratorImpl.cxx?h=libreoffice-3-3 and http://cgit.freedesktop.org/libreoffice/libs-gui/log/i18npool/source/breakiterator/breakiteratorImpl.cxx give same result. I think these two patches are committed after branching of -3-3, so there aren't included. Best Regards, -- Korrawit Pruegsanusak ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Hi Korrawit, My take is: -- you (and others) cannot reproduce fdo#37584 (redlined text disappears) in 3-3 or 3-3-3 because my patch referenced in your original post [OP] is not in the 3-3 branches. -- you and others cannot reproduce 'leading quote as word' in your 3-3 build because the patch you submitted with OP has fixed that problem in 3-3 since 2011-06-21. Your OP patch is not yet in 3-3-3 official build so the leading quote problem still exists there. A word count problem that may still exist in 3-3(-3) is that the count may be off when a selection ends in the middle of a word. Given that all of 3-3(-3) is supposed to be stable and the case where selection ends in mid-word is an edge case (not the common use case of count doc or entire section)... I think that your elegant clipping=true fix already in 3-3 is the most that should go into 3-3-3. I repeat, *IF* my patch referenced in the OP goes into the 3-3(-3) area then Cedric's fix should follow immediately. (That patch was intended as a fix of the selection ends in mid-word issue and code cleanup.) I think that the two patches given in your last message are already in 3-3(-3). The Thomas Lange patch http://cgit.freedesktop.org/libreoffice/libs-gui/commit/?id=cb9aa439fbb0a85829b1e61e292b1553512b0cb5 is already in 3.3(-3) and this is the patch that removed the deep copy at the SwScanner level so that the deep copy in Cedric's fix is now required at the outer CountWords (sp?) level. From following branch lines in QGit I think this tl patch (dated 2010-12-08) got merged into 3-4 in late winter ~Feb 2011 and that's when redlining went south in 3-4. Or so I think after following the link you posted and searching 'word count' and switching branches and scrolling past hundreds of commits in QGit and ... I hope this is clearer than my last post. I am *NOT* an expert in configuration or even in word count but I have been studying the question of 'what is where? and when did it get there?' with an eye for both qa and dev. I have tried to expose my thinking here in the hopes that it may help you and that I will get corrected as needed, *not* because it is expert opinion. Again, I hope this helps, LeMoyne -- View this message in context: http://nabble.documentfoundation.org/PATCH-Fix-for-fdo-30550-Character-count-without-spaces-tp3092043p3103177.html Sent from the Dev mailing list archive at Nabble.com. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Hello Cedric, Tor, John, all,
Yesterday, @vilpan (in irc) and I tested on official 3.3.3 build and
can reproduce fdo#33774 ('leading quote as word') But we both can't
reproduce fdo#37584. So we should fix only fdo#33774.
However, I've found two patches about this in -3-4 and master:
http://cgit.freedesktop.org/libreoffice/writer/commit/?id=e257f93f6b1c55040c9d62e4d08aefa9407a7379
http://cgit.freedesktop.org/libreoffice/libs-gui/commit/?id=cb9aa439fbb0a85829b1e61e292b1553512b0cb5
which I'm not sure which patch would be correct for -3-3 (or both? or none?)
Because my -3-3 build can't reproduce this issue.
Best Regards,
--
Korrawit Pruegsanusak
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Did you backport also the fixes mentioned by John? No, the mail thread was otherwise too confusing to me... --tml ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Hello all, On Wed, Jun 22, 2011 at 12:59, Tor Lillqvist tlillqv...@novell.com wrote: Thanks, committed and pushed to 3-3. (With a slightly improved commit message.) Thanks :-) On Wed, Jun 22, 2011 at 19:48, Tor Lillqvist tlillqv...@novell.com wrote: Did you backport also the fixes mentioned by John? No, the mail thread was otherwise too confusing to me... As I understand, John suggest us to get two of posted patch to -3-3. But I cannot reproduce neither of the issue he mentioned (in my own -3-3 build) So, maybe we should not commit them? Two issues John mentioned are: 1. fdo#37584: redline text disappear 2. 'leading quote as word' Best Regards, -- Korrawit Pruegsanusak ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
On Wed, 2011-06-22 at 20:58 +0700, Korrawit Pruegsanusak wrote: Two issues John mentioned are: 1. fdo#37584: redline text disappear I would be really surprised if that one didn't appear... -- Cédric Bosdonnat LibreOffice hacker http://documentfoundation.org OOo Eclipse Integration developer http://cedric.bosdonnat.free.fr ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Hello all, Attached patch is a fix for fdo#30550 in comment 11-12 This is specific to -3-3 only, because it was later fixed in -3-4 and master by http://cgit.freedesktop.org/libreoffice/writer/commit/?id=335534df4946437a12cd3c18b4a24beee188317b by John LeMoyne Castle, but not in -3-3. So, it would be good to have it in -3-3 and/or 3-3-4. Thanks again for Petr, Michael, and Caolán, who make my build and debug successful. :-) Please feel free to comment. Best Regards, -- Korrawit Pruegsanusak From a2baa5016abf76f2a4ecf165db6b9379badcc963 Mon Sep 17 00:00:00 2001 From: Korrawit Pruegsanusak detective.conan.1...@gmail.com Date: Wed, 22 Jun 2011 00:30:51 +0700 Subject: [PATCH] Fix for fdo#30550 This patch is a fix for fdo#30550 in comment 11-12. Instantiating aScanner should set bClip to be true. This will execute line #792 of this file and make boundary correct. Released under LGPLv3+/MPL --- sw/source/core/txtnode/txtedt.cxx |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sw/source/core/txtnode/txtedt.cxx b/sw/source/core/txtnode/txtedt.cxx index cc03200..a7a2182 100644 --- a/sw/source/core/txtnode/txtedt.cxx +++ b/sw/source/core/txtnode/txtedt.cxx @@ -1908,7 +1908,7 @@ void SwTxtNode::CountWords( SwDocStat rStat, const String aScannerText( aExpandText ); SwScanner aScanner( *this, aScannerText, 0, pConversionMap, i18n::WordType::WORD_COUNT, -(xub_StrLen)nExpandBegin, (xub_StrLen)nExpandEnd ); +(xub_StrLen)nExpandBegin, (xub_StrLen)nExpandEnd, true ); const rtl::OUString aBreakWord( CH_TXTATR_BREAKWORD ); -- 1.7.0.4 ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Hi Korrawit, Cedric, all ... A first, quick reply is that this patch of mine contains the dread redline wipeout: + // make a copy of the text + String rTextCopy = const_castlt;Stringamp;gt;(m_Text); The latter needs to be modified to include (or be immediately followed by) Cedric's fix: - String rTextCopy = const_castlt;Stringamp;gt;(m_Text); + String rTextCopy = m_Text.Copy( ); which is reattached here from an earlier post on dev-list -- [REVIEW] fix for fdo#37584 [(Redlines Erased By Word Count) == 3.4 Blocker] http://nabble.documentfoundation.org/file/n3093766/0001-fdo-37584-Make-a-real-copy-of-the-text-where-to-coun.patch 0001-fdo-37584-Make-a-real-copy-of-the-text-where-to-coun.patch I distinctly recall thinking that the word count process was making an extra copy of the string in this upper word count level and the lower text node/iterator levels. It was my stupid mistake to do this 'optimization' in word count where no effect on the document text is a mandatory requirement -- in fact my 'extra copy' idea may have been a misreading of what went on further down in the code. Very much better to not do the shallow copy, thereby keeping redlined and hidden text. I also note that the shallow copy was already in place at the time of the particular patch attached to the OP. There is no deep copy in the lines replaced by the OP patch dated 2010.11.02: - String aOldStr( m_Text ); - String rCastStr = const_castlt;Stringamp;gt;(m_Text); There were a few other commits in the word count area in late October - early November 2010. Perhaps there is another missing patch? I will use this area as a case for practice with git log/rev-list and lo-commit-stat.pl At the very least, I will search commit logs for patches by myself and Mattias Johnsson. Many thanks for bringing this up Korrawit. LeMoyne -- jlc -- View this message in context: http://nabble.documentfoundation.org/PATCH-Fix-for-fdo-30550-Character-count-without-spaces-tp3092043p3093766.html Sent from the Dev mailing list archive at Nabble.com. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Attached is another patch (in libs-gui repo) that is in 3.4 but not in 3.3
found with lo-commit-stat using:
root/bin $perl lo-commit-stat --log-dir='./' --log-suffix='mjWCRL-3.3'
--rev-list ../ --until='2010-12-01' --author='Johnsson'
origin/libreoffice-3-4 ^origin/libreoffice-3-3
and shown more directly by rev-list using:
root/clone/libs-gui $ git rev-list --after='2010-10-01'
--before='2011-01-01' --pretty=medium origin/libreoffice-3-4
^origin/libreoffice-3-3 ../../bin/libs-gui-rev3.4to3.3.log
A search through the result for 'Mattias' gives:
commit cb9aa439fbb0a85829b1e61e292b1553512b0cb5
Author: Mattias Johnsson lt;m.t.johns...@gmail.comgt;
Date: Thu Nov 4 23:25:02 2010 +1100
An opening quote should not be counted as a word by word count tool
Patch file:
http://nabble.documentfoundation.org/file/n3094191/0001-An-opening-quote-should-not-be-counted-as-a-word-by-.patch
0001-An-opening-quote-should-not-be-counted-as-a-word-by-.patch
There is only one change in the patch:
+++ b/i18npool/source/breakiterator/breakiteratorImpl.cxx
...
+{UBLOCK_GENERAL_PUNCTUATION, UBLOCK_GENERAL_PUNCTUATION,
ScriptType::LATIN},
...
This addition to the (character class definitions?) stuff used by the break
iterator elegantly fixes the leading quote as separate word problem. The
power of that single line left me:
-- wondering why only the leading quote and not the trailing quote went as
a separate word (breakIterator wouldn't tell me ;-), and
-- wondering about any other effects that this insertion may have.
That said, if the symptom you are trying to fix is the 'leading quote as
separate word' then AFAIK this patch by Mattias Johnsson fixed that in the
master of late last year and in 3.4 from its birth to today.
From a similar progression in clone/writer, it *looks like* all of Mattias'
word count patches have been applied to 3.3. Since, 3.3 is counting with
and wthout spaces then Mattias writer patch was applied there (and I do
believe there is just one in writer ==
339eee93fd2a888b541eac4e7576d7091dfd1639). I do not have a code config
pointed at 3.3 so I cannot look directly at the word count routines there.
I am still at a loss as to why the redlining doesn't erase in 3.3 and does
in 3.4, given that the shallow copy + whiting out redlined text appears to
predate my patch and it appears to be present in 3.3 from looking at the
patches in 3.4. Perhaps 3.4 contains other changes or my testing is off or
there is a later deep/full copy or ...
Whatever else happens,
1-- If my patch attached to the OP goes into 3.3 then it should
***definitely be followed by Cedric's fix*** attached to my first response.
2-- Mattias' patch inserted above fixes the 'leading quote as word' problem.
Hope this helps
LeMoyne
--
View this message in context:
http://nabble.documentfoundation.org/PATCH-Fix-for-fdo-30550-Character-count-without-spaces-tp3092043p3094191.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Fix for fdo#30550 Character count without spaces
Attached patch is a fix for fdo#30550 in comment 11-12 Thanks, committed and pushed to 3-3. (With a slightly improved commit message.) --tml ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
