Vincent van Ravesteijn wrote:
Steve Litt schreef:
On Thursday 14 May 2009 08:37:19 am Vincent van Ravesteijn - TNW wrote:
Guys,
Where can I hide to shame for writing this piece of code:
Text3.cpp:
465 TextMetrics & tm = bv->textMetrics(this);
466 if (!tm.contains(cur.pit())) {
467 lyx::dispatch(FuncRequest(LFUN_SCREEN_SHOW_CURSOR));
468 tm = bv->textMetrics(this);
469 }
Anyway, I think bug #5888 is now fixed in r29665.
Please check whether the fix is ok and the bug is fixed.
Vincent
Hi Vincent,
I'm not familiar with LyX's constructs, so none of the preceding
looks particularly bad to me, although I can't really understand line
485.
What's wrong with the preceding code?
In line 465, tm is initialized to be a reference to the textmetrics
object that is related to the current view and the current inset.
After dispatching the LFUN_SCREEN_SHOW_CURSOR, the view has most
probably changed, which also means that we need to have a reference to
a new textmetrics object. And that's what I tried to accomplish with
line 468.
However, you can't reassign a reference to refer to a different object.
Short version: References are pointers that don't look like pointers;
better yet, they're like const pointers (as opposed to pointers to
const) that are guaranteed not to be null. So 468 says: Copy the
contents of bv->textMetrics() into the memory location at which tm
points, as if you'd written:
*tm_ptr = ...
changing not the pointer but the content of what it points to. So, as
Vincent said, line 468 does not change which object tm refers to, even
if the syntax looks as if it should.
So it's perfectly good code, just not at all what he wanted to do. And
very, very easy to do, because it looks exactly the same as it would if
tm were not a reference. This is why I often think non-const references
are dangerous. You may have to look well back in the code to know what
effect a line like 468 will have.
So, what is done in line 468 is that the 'old' textmetrics object is
being assigned the values of the new textmetrics object. But, I guess,
these textmetrics objects are sort of temporary objects (I do not
grasp this textmetrics-caching code completely, to be honest), or
maybe copying should be properly implemented.
I think this is right. What I'd guess is happening is that, once the
dispatch has been done, what tm points to is invalid, and so the new
info from bv->textMetrics is being copied into a bad location. In
particular, updateMetrics() clears the cache before it does anything
else, so if that gets called, we're in trouble.
Note that, if something like this were not so, then tm would already
have pointed at the correct TextMetrics information, and the addition of
line 468 would not have been necessary.
Richard
