Re: fix patch for bug 8795

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