Re: fix patch for bug 8795

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 <vidhu2366@gmail.com> 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
> <larochelle.simon.1@gmail.com> 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 <vidhu2366@gmail.com> 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
>>> <larochelle.simon.1@gmail.com> 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 <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
>>>
>>>
>>>
>>> --
>>> Vidhoon Viswanathan
>>> +91 7760711773
>
>
>
> --
> Vidhoon Viswanathan
> +91 7760711773
Received on Sun Apr 21 22:00:51 2013