[PATCH] Fix for fdo#30550 Character count without spaces

classic Classic list List threaded Threaded
12 messages Options
Korrawit Pruegsanusak Korrawit Pruegsanusak
Reply | Threaded
Open this post in threaded view
|

[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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

0001-Fix-for-fdo-30550.patch (1K) Download Attachment
John LeMoyne Castle John LeMoyne Castle
Reply | Threaded
Open this post in threaded view
|

Re: [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_cast<String&>(m_Text);

The latter needs to be modified to include (or be immediately followed by) Cedric's fix:

- String& rTextCopy = const_cast<String&>(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]

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_cast<String&>(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
John LeMoyne Castle John LeMoyne Castle
Reply | Threaded
Open this post in threaded view
|

Re: [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 <m.t.johnsson@gmail.com>
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:
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
Tor Lillqvist Tor Lillqvist
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix for fdo#30550 Character count without spaces

In reply to this post by Korrawit Pruegsanusak
> 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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
cbosdonnat cbosdonnat
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix for fdo#30550 Character count without spaces

Hi Tor,

On Tue, 2011-06-21 at 23:59 -0600, Tor Lillqvist wrote:
> > 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.)

Did you backport also the fixes mentioned by John?

--
Cédric Bosdonnat
LibreOffice hacker
http://documentfoundation.org
OOo Eclipse Integration developer
http://cedric.bosdonnat.free.fr

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Tor Lillqvist Tor Lillqvist
Reply | Threaded
Open this post in threaded view
|

Re: [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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Korrawit Pruegsanusak Korrawit Pruegsanusak
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix for fdo#30550 Character count without spaces

Hello all,

On Wed, Jun 22, 2011 at 12:59, Tor Lillqvist <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
cbosdonnat cbosdonnat
Reply | Threaded
Open this post in threaded view
|

Re: [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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Korrawit Pruegsanusak Korrawit Pruegsanusak
Reply | Threaded
Open this post in threaded view
|

Re: [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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
John LeMoyne Castle John LeMoyne Castle
Reply | Threaded
Open this post in threaded view
|

Re: [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
Korrawit Pruegsanusak Korrawit Pruegsanusak
Reply | Threaded
Open this post in threaded view
|

Re: [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
<[hidden email]> 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_cast<String&>(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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
cbosdonnat cbosdonnat
Reply | Threaded
Open this post in threaded view
|

Re: [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
> <[hidden email]> 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_cast<String&>(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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice