On Thu, 6 Dec 2001, Juergen Vigna wrote:
>
> On 06-Dec-2001 Allan Rae wrote:
>
> > The use of the_locking_inset there seems haphazard at best. Some
> > places seem to clear the_locking_inset after an unlock call while
> > others don't.
>
> Well sometimes I think you should sleep a bit more before looking at code ;)
> You look at functions and don't see the obvious #:O) (a pitty you didn't
> write this one on Friday I could think of a really amusing exchange of
> emails ;)
Maybe.
> > Code like:
> > if (the_locking_inset) {
> > unlockInsetInInset(bv, the_locking_inset);
> > the_locking_inset = 0;
> > }
>
> Ok you're right here the above can safely be change to only
>
> unlockInsetInInset(bv, the_locking_inset);
>
> > seems to me to be equivalent to:
> >
> > unlockInsetInInset(bv, the_locking_inset);
> > the_locking_inset = 0;
>
> It is indeed!
>
> > and could be written as:
> >
> > if (unlockInsetInInset(bv, the_locking_inset)) {
> > the_locking_inset = 0;
> > }
>
> Nope the if is not neccessary, but yes if you like you could write it as
> that too ;)
>
> > but that isn't really any faster or better but might expose other
> > bugs.
>
> Why? The code in unlockInsetInInset() is quite clear
>
> if (!the_locking_inset) return false;
>
> Tis will just do nothing and the_locking_inset anyway is already 0,
> so we don't have to set it to 0 again, isn't it?
BUT, unlockInsetInInset() only clears the_locking_inset if
the_locking_inset == inset. If instead, the_locking_inset is
something else the code path taken attempts to look at the insets
below it (its children) and then returns true if an inner inset was
successfully unlocked otherwise it returns false.
Your code that unconditionally clears the_locking_inset upon return
from unlockInsetInInset() is effectively saying when false is
returned:
I'm not the locked inset, none of my children is the locked
inset, so someone has stuffed up somewhere. I better just
clear the_locking_inset to be safe.
So you see, by writing:
unlockInsetInInset(bv, the_locking_inset);
the_locking_inset = 0;
You have just cleared what may have been a stray pointer and possibly
hiding a bug -- if it didn't cause a crash during the attempted
unlocking.
For the case where unlockInsetInInset returns true and
the_locking_inset != inset you have said, according to the code in
InsetTabular::localDispatch(), that if the user uses tab, shift-tab,
prior or next to move the cursor between table cells then you want to
ensure that the_locking_inset is cleared -- this is where you
unconditionally clear the_locking_inset. So for this case the code is
saying:
If the_locking_inset was one of my children and it was
successfully unlocked, clear the_locking_inset.
However, when the user appends or deletes a row or column in
InsetTabular::tabularFeatures() you _don't_ ensure that
the_locking_inset is cleared. In this code, it seems to me, you just
hope that the_locking_inset == inset and that everything will be okay
otherwise. If I append a row, and the locked inset is an inset within
the insettext of a cell then surely you'd want to not only unlock the
locked inset but remove record of it being locked from the insettext
_and_ from the tabular cell. At present, as I read it this is not the
case: the insettext's the_locking_inset is cleared by the
unlockInset() call in InsetText::unlockInsetInInset() but the
insettabular's the_locking_inset is left untouched. As a result isn't
it now saying that an inner inset is locked when in fact it isn't?
Perhaps there is some other thinking behind this apparent discrepency?
Or I'm still misunderstanding something.
> Hope this helps you a bit to find some good sleep tonight #:O) otherwise
> have some of the really nice beers you produce there in Australia the also
> help to have a good sleep ;)
Do you still think I need more sleep?
I got 8.5hrs last night but that may not have been enough!
Anyway, this isn't fixing the bug I reported, which is also present in
1.1.6cvs.
Allan. (ARRae)
