Re: fix patch for bug 8795

Vidhoon,

I can still reproduce the bug after applying your patch (that is, if I
insert enough spaces at the end of a paragraph, I can get the
FPRUN_ENDOFPARAGRAPH to jump to the next line). Your analysis of the
code seems right. You just need to make sure that a line does not
start with either a blank run (a run composed only of spaces) or a
zero-width run.

Simon

On Wed, Mar 13, 2013 at 2:43 PM, Vidh <[email protected]> wrote:
> Let me explain briefly how I have tried to fixed this bug.
>
> my understanding:
> ================
> fb_LineBreaker::breakParagraph has two parts.
> The first part processes each run in each line of paragraph and:
>         1)detects lines that have exceeded max width
>         2)identifies the run at which the width offense happens
>
> In the second half, the line is broken at the 'identified run'.
> This is accomplished by '_breakTheLineAtLastRunToKeep'
>
> _breakTheLineAtLastRunToKeep:
> If the given line does not end at m_pLastRunToKeep then
> this method inserts the excess run into the next line or IF THERE IS
> NO NEXT LINE IT CREATES A 'NEW LINE'.
> From the rear end of the line, it bumps each run out and checks if it
> has processed till m_pLastRunToKeep.
>
> Now coming to my fix,
> ==================
> I simply defer the bumping of excess runs in the given line to NEW
> LINE if there is trailing space in the given line.
> This deferral exhausts the trailing space and then expires.
> Until trailing space is exhaused, the NEW LINE will be empty
> (countRuns()=0) and gets deleted in the next iteration of
> breakParagraph. (lines 340-350 in fb_LineBreaker.cpp)
>
> Some debug traces reflecting the creation and deletion of this empty line:
>
> DEBUG: PD_Document Insert span | | pos 18
> ---->DEBUG: !!!! Generated Line 90a6d00
> ---->DEBUG: Deleting line 90a6d00 having runs 0
> DEBUG: Doing DocSection Update layout (section 0x8e57000)
> DEBUG: ASFRENT: unified _draw call for a total of 1 previous calls.
> DEBUG: PD_Document Insert span | | pos 19
> ---->DEBUG: !!!! Generated Line 921f400
> ---->DEBUG: Deleting line 921f400 having runs 0
> DEBUG: Doing DocSection Update layout (section 0x8e57000)
> DEBUG: ASFRENT: unified _draw call for a total of 1 previous calls.
>
> I have not deferred creation of new line when there is trailing space
> (It is not straightforward and will require the function to be re
> written). I have only deferred population of the new line with runs.
>
>
> Simon,
> Are you talking about those empty lines that get created and deleted?
> Please excuse if I am misunderstanding your comments. (am a beginner
> trying to get familiar with terminologies)
>
> I did some analysis based on what I understood from your comments.
> I can find that all runs belong to the original line and the dummy
> newline (which  is empty & gets deleted immediately as explained
> above) is having no runs at all.
>
> DEBUG: PD_Document Insert span |n| pos 10
> ---->DEBUG: bRunIsNonBlank true line 92a43a8
> ---->DEBUG: bRunIsNonBlank true line 92a43a8
> ---->DEBUG: bRunIsNonBlank true line 92a43a8
> ---->DEBUG: !!!! Generated Line 8cf2170
> ---->DEBUG: Deleting line 8cf2170 having runs 0
> DEBUG: Doing DocSection Update layout (section 0x9282ec8)
> DEBUG: ASFRENT: unified _draw call for a total of 1 previous calls.
> DEBUG: PD_Document Insert span |g| pos 11
> ---->DEBUG: bRunIsNonBlank true line 92a43a8
> ---->DEBUG: bRunIsNonBlank true line 92a43a8
> ---->DEBUG: bRunIsNonBlank true line 92a43a8
> ---->DEBUG: !!!! Generated Line 8cf2378
> ---->DEBUG: Deleting line 8cf2378 having runs 0
> DEBUG: Doing DocSection Update layout (section 0x9282ec8)
> DEBUG: ASFRENT: unified _draw call for a total of 1 previous calls.
>
> Can you guide me further based on this information ?
>
> I have also attached few screenshots showing results from my machine
> for illustration with a simple test case. Please let me know if I am
> going the right way.
>
> thanks for your time and inputs.
>
>
> On Wed, Mar 13, 2013 at 7:50 PM, Simon Larochelle
> <[email protected]> wrote:
>> Hi Vidhoon,
>>
>> I tested your patch. Unfortunately, it does not seem to fix the bug. I
>> still get an extra line with zero width runs (FP_FMTMARK and
>> FPRUN_ENDOFPARAGRAPH). You should look into modifying the
>> bRunIsNonBlank test.
>>
>> Regards,
>>
>> Simon
>>
>> On Sun, Mar 10, 2013 at 6:44 PM, Vidh <[email protected]> wrote:
>>> Hi all,
>>>
>>> I am Vidhoon, a newbie developer at Abiword. :)
>>> I am looking forward to participate in GSOC 2013.
>>>
>>> I worked on bug 8795:
>>> http://bugzilla.abisource.com/show_bug.cgi?id=8795
>>>
>>> I have created a first draft fix patch for the same with some detailed comments.
>>> I am also copy pasting the patch for easier reference at the end of
>>> this mail body.
>>>
>>> Please provide your valuable suggestions so that I can improve this
>>> patch and get it merged.
>>> I could not find any template mails in mailing list archives to send bug fixes.
>>> So excuse me if I am not following any procedures. Please indicate the
>>> same and I will oblige.
>>>
>>> Pointers to coding conventions (some I was able to note from existing
>>> code and pick automatically) are also welcome to improve on this first
>>> draft patch.
>>>
>>> Index: text/fmt/xp/fb_LineBreaker.cpp
>>> ===================================================================
>>> --- text/fmt/xp/fb_LineBreaker.cpp      (revision 32687)
>>> +++ text/fmt/xp/fb_LineBreaker.cpp      (working copy)
>>> @@ -37,7 +37,8 @@
>>>         m_pFirstRunToKeep(NULL),
>>>         m_pLastRunToKeep(NULL),
>>>         m_iMaxLineWidth(0),
>>> -       m_iWorkingLineWidth(0)
>>> +       m_iWorkingLineWidth(0),
>>> +       m_iTrailingSpace(-1)
>>>  {
>>>         xxx_UT_DEBUGMSG(("fb_LineBreaker %x created \n",this));
>>>  }
>>> @@ -630,7 +631,7 @@
>>>                         UT_ASSERT(pNewLine);    // TODO check for outofmem
>>>                         pNextLine = pNewLine;
>>>                         m_iMaxLineWidth = pNextLine->getMaxWidth();
>>> -                       xxx_UT_DEBUGMSG(("!!!! Generated a new Line \n"));
>>> +                       xxx_UT_DEBUGMSG(("!!!! Generated a new Line \n"));
>>>                 }
>>>                 else
>>>                 {
>>> @@ -648,7 +649,43 @@
>>>                 while (pRunToBump && pLine->getNumRunsInLine() &&
>>> (pLine->getLastRun() != m_pLastRunToKeep))
>>>                 {
>>>                         UT_ASSERT(pRunToBump->getLine() == pLine);
>>> -                       xxx_UT_DEBUGMSG(("RunToBump %x Type %d Offset %d Length %d
>>> \n",pRunToBump,pRunToBump->getType(),pRunToBump->getBlockOffset(),pRunToBump->getLength()));
>>> +                       if(pRunToBump->getType() == FPRUN_ENDOFPARAGRAPH )
>>> +                       {
>>> +                               xxx_UT_DEBUGMSG(("Trailing space present in prev run
>>> %d\n",pRunToBump->getPrevRun()->findTrailingSpaceDistance()));
>>> +/*
>>> +FIX for bug 8795
>>> +================
>>> +if EOP then
>>> +       if prev run has trailing white space then
>>> +               1)store actual trailing space
>>> +                2)delay run bump till trailing space is exhausted
>>> +        else
>>> +                bump contents into newline
>>> +*/
>>> +
>>> +
>>> if(pRunToBump->getPrevRun()->findTrailingSpaceDistance())
>>> +                               {
>>> +                                       if(m_iTrailingSpace == -1)
>>> +                                       {
>>> +                                               xxx_UT_DEBUGMSG(("backing up trailing space"));
>>> +                                               m_iTrailingSpace = pRunToBump->getPrevRun()->findTrailingSpaceDistance();
>>> +                                       }
>>> +                                       if(pLine->getFilledWidth() - m_iTrailingSpace < m_iMaxLineWidth)
>>> +                                       {
>>> +                                               xxx_UT_DEBUGMSG(("Filledwidth %d TrailingSpace %d FW-TS %d
>>> MaxWidth %d",pLine->getFilledWidth(),m_iTrailingSpace,pLine->getFilledWidth()
>>> - m_iTrailingSpace, m_iMaxLineWidth));
>>> +                                               break;
>>> +                                        }
>>> +                                       else
>>> +                                       {
>>> +                                               xxx_UT_DEBUGMSG(("trailing space over.. should now start newline"));
>>> +                                                m_iTrailingSpace = -1;
>>> +                                       }
>>> +                                }
>>> +
>>> +
>>> +                       }
>>> +
>>> +                       UT_DEBUGMSG(("RunToBump %x Type %d Offset %d Length %d
>>> \n",pRunToBump,pRunToBump->getType(),pRunToBump->getBlockOffset(),pRunToBump->getLength()));
>>>                         if(!pLine->removeRun(pRunToBump))
>>>                         {
>>>  //
>>> @@ -673,8 +710,12 @@
>>>                         xxx_UT_DEBUGMSG(("Next runToBump %x \n",pRunToBump));
>>>                 }
>>>         }
>>> -
>>> -       UT_ASSERT((!m_pLastRunToKeep) || (pLine->getLastRun() == m_pLastRunToKeep));
>>> +/*
>>> +below assert not applicable now since when trailing space
>>> +is present the current line is not broken immediately and
>>> +delayed till trailing space is exhausted by non blank characters
>>> +*/
>>> +       //UT_ASSERT((!m_pLastRunToKeep) || (pLine->getLastRun() ==
>>> m_pLastRunToKeep));
>>>  #if DEBUG
>>>         pLine->assertLineListIntegrity();
>>>         if(pNextLine)
>>> Index: text/fmt/xp/fb_LineBreaker.h
>>> ===================================================================
>>> --- text/fmt/xp/fb_LineBreaker.h        (revision 32687)
>>> +++ text/fmt/xp/fb_LineBreaker.h        (working copy)
>>> @@ -54,6 +54,7 @@
>>>
>>>         UT_sint32       m_iMaxLineWidth;
>>>         UT_sint32       m_iWorkingLineWidth;
>>> +       UT_sint32       m_iTrailingSpace;
>>>  };
>>>
>>>  #endif /* FB_LINEBREAKER_H */
>>>
>>>
>>> regards,
>>> --
>>> Vidhoon Viswanathan
>>> +91 7760711773
>
>
>
> --
> Vidhoon Viswanathan
> +91 7760711773