On Mon, 10 Dec 2001, Lars Gullik Bjønnes wrote:
> Allan Rae <[EMAIL PROTECTED]> writes:
>
> | On Fri, 7 Dec 2001, Juergen Vigna wrote:
> >
> >>
> >> On 07-Dec-2001 Allan Rae wrote:
> >> > Perhaps there is some other thinking behind this apparent discrepency?
> >> > Or I'm still misunderstanding something.
> >>
> >> Well I'm now writing v e r y s l o w l y so you
> >> may understand this time (hopefully):
> >>
> >> We call: unlockInsetInInset(bv, the_locking_inset);
> >>
> >> You complain: Well what if inset != the_locking_inset inside unlockInsetInInset!
> >>
> >> I tell you: How can inset != the_locking_inset IF (and now slowly again) I
> >> c a l l i t w i t h t h e _ l o c k i n g _ i n s e t?
> >>
> >> You tell me: (insert your answer here for the right answer you get 100 points
> >> [ out of 1.000.000])
> >
> | Good question.
> >
> | Now that we've established that the code you wrote has several
> | redundancies I have committed a patch to clean them up -- removing 10
> | lines of code in the process. Three cases of plus one other:
> >
> | - if (the_locking_inset) {
> | - unlockInsetInInset(bv, the_locking_inset);
> | - the_locking_inset = 0;
> | - }
> | + unlockInsetInInset(bv, the_locking_inset);
> >
> | Since unlockInsetInInset does its own check for the_locking_inset and
> | will also do the reset to 0 part.
>
> but this is only correct if inset == the_locking_inset at _all_ times.
inset is the name of one of the functions arguements. For the places
I changed, the arguement (inset) is the_locking_inset. In fact all
calls to unlockInsetInInset in insettabular.C are called with inset ==
the_locking_inset.
unlockInsetInInset can be called by an outer inset though in which
case the arguement "inset" may not equal the_locking_inset -- although
in that case we'd need to be unlocking an inset that is not the
current active cell. It seems we'd need to have code that set the
active cell when a user moves/clicks somewhere in a table, then tried
to unlock the previous table cell (its inset) for this to happen.
> And this is true, we should replace the
>
> if (the_locking_inset == inset)
Unlike some of the other classes "inset" here is the name of a
function arguement _not_ the name of the variable in the class that
holds the real InsetXxxx.
> in insettabular.C with an assert to this effect.
> (and remove the whole second if clause:
>
> if (the_locking_inset->unlockInsetInInset(bv, inset, lr)) {
> ...
> }
>
> So is this true, if so a lot of cleanup can be done, if this is not
> true, the changes you have done appear to be a bit unsafe.
I'd be inclinded to try the assert anyway to try to find out why/how
the inset unlockInsetInInset is asked to unlock isn't the current
active cells inset. Then we may end up with "a lot of cleanup" as you
suggest.
For the instances I have changed the code is equivalent. One place
that does look strange is the second if clause you point out. Nowhere
in that code is the_locking_inset = 0; even though an attempt at
unlocking occurs. No doubt this is the area you thought my changes
made unstable and is the same aspect I was argueing with Jürgen about
although for the cases I was argueing about he pointed out that it
would be the (the_locking_inset == inset) path that would be taken --
and thereby avoided the rest of the problem.
So perhaps now that we have two INTJs querying this code Jürgen might
bless us with his insights.
##
There are also a number of similar places involving insetUnlock()
calls that could use a cleanup -- it seems InsetTabular::insetUnlock()
does too much or perhaps needs a
if (!the_locking_inset) return;
added at the top of the function. I'm looking at the way the code in
InsetTabular::insetButtonPress(BufferView *, int, int, int) is
written. In particular lines 740-749 and to a lesser extent 730-733.
Allan. (ARRae)
