Re: fix patch for bug 8795

Thanks Simon! :)

On Mon, Apr 22, 2013 at 1:30 AM, Simon Larochelle
<[email protected]> wrote:
> Hi Vidhoon,
>
> Your latest patch is working; I committed it to the svn repository.
> Congratulation for your first Abiword patch.
>
> Simon
>
>
> On Fri, Apr 19, 2013 at 1:27 PM, Vidh <[email protected]> wrote:
>> Simon,
>>
>> I was on vacation. So took a gap of few days.
>> I came back and worked on the problems you had indicated in my previous fix.
>>
>> On top of my previous fix, I have made change to detect and prevent
>> creation of empty lines with zero width runs.
>>
>> Regarding the second patch for insertion point position I had
>> uploaded, please note that the insertion point goes beyond the right
>> margin and page width if we keep on adding extra whitespace.
>>
>> Yes, I agree that as long as abiword does not crash and user is not
>> confused this is ok to keep. But in the long run, we might need
>> another patch to prevent the insertion point from going beyond right
>> margin if there is only blank/trailing space beyond the right margin.
>>
>> Thanks for pointing out the purpose of FV_View::isPointLegal.
>>
>> I have attached the latest fix also along with this mail.
>> Kindly revert if there are issues.
>>
>> thanks and regards,
>>
>> On Sun, Mar 24, 2013 at 10:57 PM, Simon Larochelle
>> <[email protected]> wrote:
>>> Hi Vidhoon,
>>>
>>> Your patch is not quite working. It fails in the following two cases:
>>>
>>> 1) Fill a line with extra spaces in the right margin.
>>> Add a word (the word appears on a new line as it should).
>>> Now erase the last word. The second line does not disappear.
>>>
>>> 2) Fill a line with extra spaces in the right margin.
>>> Change the font style to bold (this adds a zero-width FPRUN_FMTMARK run).
>>> Go back one space and add a space. A new line is created.
>>>
>>> Again, you need to make sure that a line does not start with a
>>> zero-width run (except obviously for the first line of a paragraph).
>>>
>>> As for your second patch, I am not convinced that it is needed. Adding
>>> extra spaces at the right of the line is a user error and as long as
>>> what we are doing does not confuse the user and does not cause the
>>> program to crash, that should be ok. Note that FV_View::isPointLegal
>>> is a fonction to test whether the cursor is at a valid position in the
>>> piece table (i.e. if text can be inserted at this position). This
>>> fonction should not be used to control the place where the cursor
>>> appears in the present case.
>>>
>>> Simon
>>>
>>>
>>> On Fri, Mar 22, 2013 at 10:31 AM, Vidh <[email protected]> wrote:
>>>> Hi Simon ,
>>>>
>>>> I understood the case which was failing.
>>>> Now I have worked on addressing those aspects and created new fix patches.
>>>>
>>>> I have uploaded the same in bugzilla.
>>>> Now the fix comprises of 2 patches:
>>>> 1) The line breaker patch avoids creation of new line with blank
>>>> characters and also prevents bumping runs to new line when there is
>>>> trailing space in last run of a line.
>>>>
>>>> 2)After applying the line breaker fix, when there is trailing space,
>>>> new line is not created. Instead white space is kept in the same line
>>>> even as it goes out of doc right margin boundary. But this created
>>>> navigation bug in which the insertion point would also go outside the
>>>> doc boundary.The insertion point fix addresses this problem for
>>>> keyboard navigation and stops insertion point from moving beyond right
>>>> margin boundary when there is white space beyond.
>>>>
>>>> Can you please take a look when you find time and update me?
>>>>
>>>> There is one more pending sub fix needed. this is related to the
>>>> insertion point navigation using mouse.With these two fixes, if the
>>>> cursor (mouse) is used to click at a point in the white space beyond
>>>> the right doc margin, the insertion point is navigated to that
>>>> position. This is not expected and should be fixed.
>>>>
>>>> We can raise another bug for that and track it. Please advise on this aspect.
>>>> thanks!
>>>>
>>>> On Thu, Mar 14, 2013 at 6:56 AM, Simon Larochelle
>>>> <[email protected]> wrote:
>>>>> 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
>>>>
>>>>
>>>>
>>>> --
>>>> Vidhoon Viswanathan
>>>> +91 7760711773
>>
>>
>>
>> --
>> Vidhoon Viswanathan
>> +91 7760711773



-- 
Vidhoon Viswanathan
+91 7760711773