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 <vidhu2366@gmail.com> 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
> <larochelle.simon.1@gmail.com> 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 <vidhu2366@gmail.com> 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
Received on Thu Mar 14 02:26:33 2013