Subject: fl_BlockLayout::findPointCoords
From: Jesper Skov (jskov@redhat.com)
Date: Sat May 20 2000 - 11:54:44 CDT
Hi there
I've just spent some time trying to figure out how to fix a bug
causing an assertion. I don't think there's a register Bugzilla entry
for this, so briefly:
open abiword
enter "1234"
cursor left twice
break page (control-return)
**** (1) Assert ****
**** (1) FP_RUN_NOT != containsOffset(iOffset) at fp_Run.cpp:1222 ****
**** (1) Continue ? (y/n) [y] : n
Basically, I just crashed on the sources and there are a gazillion
things I don't understand, so please bear with me and my (stupid&lazy)
questions.
My questions are about the existing implementation of
fl_BlockLayout::findPointCoords. At the bottom I have an alternative
implementation, but since I don't understand the existing properly,
it's probably broken in various interesting subtle ways.
What I hope to get out of this is better understanding of this
particular part of AbiWord. I'll try to add some stuff to
AbiWord_LayoutDesign.abw (and as comments) if I ever find out what's
going on.
OK, below the existing implementation of the function with my comments
and questions in []. FAOD I'm not criticizing - I'm just asking stupid
questions :)
fp_Run* fl_BlockLayout::findPointCoords(PT_DocPosition iPos, UT_Bool bEOL, UT_sint32& x, UT_sint32& y, UT_sint32& height)
{
// find the run which has this position inside it.
PT_DocPosition dPos = getPosition();
UT_ASSERT(iPos >= dPos);
if (!m_pFirstLine || !m_pFirstRun)
{
// when we have no formatting information, can't find anything
return NULL;
}
const UT_uint32 iRelOffset = iPos - dPos;
fp_Run* pRun = m_pFirstRun;
while (pRun)
{
UT_uint32 iWhere = pRun->containsOffset(iRelOffset);
[special code to handle something in this loop which would be handled
in the next loop anyway. Complicates the code - performance reasons?]
if (FP_RUN_JUSTAFTER == iWhere)
{
if(pRun->getNext())
{
if(pRun->getNext()->containsOffset(iRelOffset) == FP_RUN_INSIDE)
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun->getNext();
}
}
}
if (FP_RUN_INSIDE == iWhere)
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
else if (bEOL && (FP_RUN_JUSTAFTER == iWhere))
{
fp_Run* pNext = pRun->getNext();
fp_Line* pNextLine = NULL;
if (pNext)
pNextLine = pNext->getLine();
if (pNextLine != pRun->getLine())
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
}
pRun = pRun->getNext();
}
pRun = m_pFirstRun;
while (pRun)
{
[Except for the JUSTAFTER:INSIDE assumption and the special "try
harder" semantics this loop looks like the above. I would consider
merging them if, if the JUSTAFTER:INSIDE code needs to be like what's
in the above loop.]
UT_uint32 iWhere = pRun->containsOffset(iRelOffset);
if ((FP_RUN_JUSTAFTER == iWhere))
{
[This is what's causing the assertion: it is assumed that if
JUSTAFTER, the following element must be INSIDE - that's not the case
with the pagebreak which always returns NORUN]
fp_Run* nextRun = pRun->getNext();
if (nextRun)
{
nextRun->lookupProperties();
nextRun->findPointCoords(iRelOffset, x, y, height);
[nextRun->lookupProperties? Some assumption about the state of a run
which is different compared to the loop above?
Also, nextRun is the one providing the coordinates, but pRun is
returned. Another correct subtlety or a bug?]
}
else
{
pRun->findPointCoords(iRelOffset, x, y, height);
}
return pRun;
}
if (!pRun->getNext())
{
// this is the last run, we're not going to get another chance, so try harder
if (iRelOffset > (pRun->getBlockOffset() + pRun->getLength()))
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
}
pRun = pRun->getNext();
}
if (iRelOffset < m_pFirstRun->getBlockOffset())
{
m_pFirstRun->findPointCoords(iRelOffset, x, y, height);
return m_pFirstRun;
}
[This loop confuses me. First off, Runs can always contain point
(because they're text-only atm - something that'll change in
the future?)
Second, again the get-coords-from-nextRun-but-return-pRun.
If this code is ever executed it always returns the coords returned by
the second run in the block - assuming it doesn't assert like a
pageBreak would
]
pRun = m_pFirstRun;
while (pRun)
{
if (pRun->canContainPoint())
{
fp_Run* nextRun = pRun->getNext();
if (nextRun)
{
nextRun->findPointCoords(iRelOffset, x, y, height);
}
else
{
pRun->findPointCoords(iRelOffset, x, y, height);
}
return pRun;
}
pRun = pRun->getNext();
}
UT_ASSERT(UT_SHOULD_NOT_HAPPEN);
return NULL;
}
Any of my questions make sense?
Anyhu, here's my rewrite:
fp_Run* fl_BlockLayout::findPointCoords(PT_DocPosition iPos, UT_Bool bEOL, UT_sint32& x, UT_sint32& y, UT_sint32& height)
{
// find the run which has this position inside it.
PT_DocPosition dPos = getPosition();
UT_ASSERT(iPos >= dPos);
if (!m_pFirstLine || !m_pFirstRun)
{
// when we have no formatting information, can't find anything
return NULL;
}
const UT_uint32 iRelOffset = iPos - dPos;
fp_Run* pRun = m_pFirstRun;
while (pRun)
{
UT_uint32 iWhere = pRun->containsOffset(iRelOffset);
fp_Run* pNext = pRun->getNext();
if (FP_RUN_INSIDE == iWhere)
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
if (FP_RUN_JUSTAFTER == iWhere && bEOL)
{
fp_Line* pNextLine = NULL;
if (pNext)
pNextLine = pNext->getLine();
if (pNextLine != pRun->getLine())
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
}
pRun = pNext;
}
pRun = m_pFirstRun;
while (pRun)
{
UT_uint32 iWhere = pRun->containsOffset(iRelOffset);
fp_Run* pNext = pRun->getNext();
if ((FP_RUN_JUSTAFTER == iWhere))
{
if(pNext && pNext->containsOffset(iRelOffset) == FP_RUN_INSIDE)
{
pNext->lookupProperties();
pNext->findPointCoords(iRelOffset, x, y, height);
return pNext;
}
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
if (!pNext)
{
// this is the last run, we're not going to
// get another chance, so try harder
if (iRelOffset > (pRun->getBlockOffset() + pRun->getLength()))
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
}
pRun = pNext;
}
if (iRelOffset < m_pFirstRun->getBlockOffset())
{
m_pFirstRun->findPointCoords(iRelOffset, x, y, height);
return m_pFirstRun;
}
pRun = m_pFirstRun;
while (pRun)
{
if (pRun->canContainPoint())
{
pRun->findPointCoords(iRelOffset, x, y, height);
return pRun;
}
pRun = pRun->getNext();
}
UT_ASSERT(UT_SHOULD_NOT_HAPPEN);
return NULL;
}
It prevents the assertion and seems from minimal testing to be
working. I still think the last loop could be removed, and the two
first loops could probably be merged.
Still, when moving the cursor from 2-<pagebreak>->3 and then from 3->4
the page moves a bit. Like the original coordinate returned is not
correct - which it probably isn't since it's the coordinate from the
Run containing '2' while the cursor is on the page containing Run '3'.
Also the cursor doesn't get redrawn when moving between the chars on
the second page. A ghost image sometimes get stub between 3 and 4. I
don't know if it's related to this change.
Thanks,
Jesper
This archive was generated by hypermail 2b25 : Sat May 20 2000 - 11:54:52 CDT