Re: commit: Fix 7991

Hi Dom,

my main concern is not the performance, but that this commit fixes only
one special case of the bug. I agree that Robert's _ensureInBlock() is
much more robust than the m_bInPara flag; if performances is not an
issue (and I am happy to accept your judgement on that), then let's get
rid off the flag and use this function throughout the importer in the
first instance.

However, I think we should seriously consider moving this function into
the PT/PD_Document and have the append() and insert() methods of the
document do this checking rather than the MSWord importers. This way we
will avoid bugs in other importers as well.

Tomas

Dom Lachowicz wrote:
> Hi Tomas,
>
> I'm going to disagree with you here. This is no more
> nor no less of a hack than using m_bInPara. This is,
> however, a more robust solution because m_bInPara has
> a chance of getting out of sync while this does not.
>
> I also do not believe that this is a "performance
> killer" as you suggest. Checking for the presence of
> an open block when we insert an image is *negligable*
> compared to the time we spend laying out and rendering
> the document.
>
> In short, I believe that m_bInPara is a hack. I
> believe that this is a much more correct and robust
> fix. I believe that we should remove m_bInPara and use
> this in its place. And I believe that this might even
> be an acceptable work-around for bug 6427 where I
> suggest that the PT should lazy-create sections,
> blocks, spans, etc. where it expects them but none
> exist.
>
> Best,
> Dom
>
> --- Tomas Frydrych <tomasfrydrych@yahoo.co.uk> wrote:
>
>
>>
>>Hi Robert,
>>
>>As I indicated to you on the IRC I have concerns
>>about this fix. The
>>orginal code already had a test for the presence of
>>a block via the
>>m_bInPara flag. The problem clearly is that in the
>>given case that flag
>>was set without a block being in place (i.e., it was
>>most likely not
>>reset at some point where it should have been).
>>
>>My concern is that this fix does not deal with the
>>primary reason for
>>the crash which will eventually cause crashes in
>>other places. Pretty
>>much anything we insert into a document has to be
>>preceeded by a block,
>>so this fix deals with just one special case; at the
>>sime time I do not
>>want to call _ensureInBlock() function every time we
>>at present check
>>the m_bInPara flag for performance reasons. What
>>needs to be done in the
>>first instance is to find out where that flag is set
>>from, and how we
>>get from that place to the place where we try to
>>insert our image.
>>
>>While I apprecite the work you put into it, I think
>>this should be
>>reveted and the bug reopened.
>>
>>Tomas
>>
>>
>>Robert Wilhelm wrote:
>>
>>>Fixes 7991 (Word document with two images in
>>
>>Header crashes abiword).
>>
>>>
>>>New function _ensureInBlock(). Fixes 7991.
>>>CVS:
>>>
>>
> ----------------------------------------------------------------------
>
>>>CVS: Enter Log. Lines beginning with `CVS:' are
>>
>>removed automatically
>>
>>>CVS:
>>>CVS: Committing in .
>>>CVS:
>>>CVS: Modified Files:
>>>CVS: ie_imp_MsWord_97.cpp ie_imp_MsWord_97.h
>>>CVS:
>>>
>>
> ----------------------------------------------------------------------
>
>>>
>>>
>
>
>
>
> __________________________________
> Do you Yahoo!?
> Jazz up your holiday email with celebrity designs. Learn more.
> http://celebrity.mail.yahoo.com
>
Received on Mon Jan 3 11:55:44 2005