Re: [patch] fixing segfault because of empty coord cache
Citeren Andre Poenitz [EMAIL PROTECTED]:
On Tue, Jun 12, 2007 at 09:18:45AM +0300, Martin Vermeer wrote:
On Mon, Jun 11, 2007 at 11:45:34PM +0200, Andre Poenitz wrote:
On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
BTW I find isActive() not very clear. isHighlyEditable() would
be clearer.
Is there the equivalent of IS_EDITABLE in math?
None that I am aware of.
One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
a single flag for two orthogonal concepts ('can enter the thing' and
'has an associated dialog')
True. Could be two booleans instead.
And one of them is 'isActive()' (which probably {c,sh}ould be renamed)
...to hasText()?
...and the other one hasDialog().
- Martin
Re: [patch] fixing segfault because of empty coord cache
Martin == Martin Vermeer [EMAIL PROTECTED] writes:
And one of them is 'isActive()' (which probably {c,sh}ould be
renamed)
Martin ...to hasText()?
Or isDescendable().
JMarc
Re: [patch] fixing segfault because of empty coord cache
Am 13.06.2007 um 14:02 schrieb Jean-Marc Lasgouttes:
Martin == Martin Vermeer [EMAIL PROTECTED] writes:
And one of them is 'isActive()' (which probably {c,sh}ould be
renamed)
Martin ...to hasText()?
Or isDescendable().
+1
hasText() sounds like !cell(i).empty()
Stefan
PGP.sig
Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
On Wed, Jun 13, 2007 at 02:09:39PM +0200, Stefan Schimanski wrote:
Am 13.06.2007 um 14:02 schrieb Jean-Marc Lasgouttes:
Martin == Martin Vermeer [EMAIL PROTECTED] writes:
And one of them is 'isActive()' (which probably {c,sh}ould be
renamed)
Martin ...to hasText()?
Or isDescendable().
+1
Yes, this is better.
- Martin
Re: [patch] fixing segfault because of empty coord cache
On Wed, Jun 13, 2007 at 02:02:48PM +0200, Jean-Marc Lasgouttes wrote:
Martin == Martin Vermeer [EMAIL PROTECTED] writes:
And one of them is 'isActive()' (which probably {c,sh}ould be
renamed)
Martin ...to hasText()?
Or isDescendable().
Better.
Andre'
Re: [patch] fixing segfault because of empty coord cache
Citeren Andre Poenitz <[EMAIL PROTECTED]>:
On Tue, Jun 12, 2007 at 09:18:45AM +0300, Martin Vermeer wrote:
On Mon, Jun 11, 2007 at 11:45:34PM +0200, Andre Poenitz wrote:
> On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
> > BTW I find isActive() not very clear. isHighlyEditable() would
be clearer.
> > Is there the equivalent of IS_EDITABLE in math?
>
> None that I am aware of.
>
> One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
> a single flag for two orthogonal concepts ('can enter the thing' and
> 'has an associated dialog')
True. Could be two booleans instead.
And one of them is 'isActive()' (which probably {c,sh}ould be renamed)
...to hasText()?
...and the other one hasDialog().
- Martin
Re: [patch] fixing segfault because of empty coord cache
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes:
>> And one of them is 'isActive()' (which probably {c,sh}ould be
>> renamed)
>>
Martin> ...to hasText()?
Or isDescendable().
JMarc
Re: [patch] fixing segfault because of empty coord cache
Am 13.06.2007 um 14:02 schrieb Jean-Marc Lasgouttes:
"Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes:
And one of them is 'isActive()' (which probably {c,sh}ould be
renamed)
Martin> ...to hasText()?
Or isDescendable().
+1
hasText() sounds like !cell(i).empty()
Stefan
PGP.sig
Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
On Wed, Jun 13, 2007 at 02:09:39PM +0200, Stefan Schimanski wrote:
>
> Am 13.06.2007 um 14:02 schrieb Jean-Marc Lasgouttes:
>
> >> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes:
> >
> >>> And one of them is 'isActive()' (which probably {c,sh}ould be
> >>> renamed)
> >>>
> >
> > Martin> ...to hasText()?
> >
> > Or isDescendable().
>
> +1
>
Yes, this is better.
- Martin
Re: [patch] fixing segfault because of empty coord cache
On Wed, Jun 13, 2007 at 02:02:48PM +0200, Jean-Marc Lasgouttes wrote:
> > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes:
>
> >> And one of them is 'isActive()' (which probably {c,sh}ould be
> >> renamed)
> >>
>
> Martin> ...to hasText()?
>
> Or isDescendable().
Better.
Andre'
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 11:45:34PM +0200, Andre Poenitz wrote:
On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
BTW I find isActive() not very clear. isHighlyEditable() would be clearer.
Is there the equivalent of IS_EDITABLE in math?
None that I am aware of.
One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
a single flag for two orthogonal concepts ('can enter the thing' and
'has an associated dialog')
True. Could be two booleans instead.
- Martin
Re: [patch] fixing segfault because of empty coord cache
On Tue, Jun 12, 2007 at 09:18:45AM +0300, Martin Vermeer wrote:
On Mon, Jun 11, 2007 at 11:45:34PM +0200, Andre Poenitz wrote:
On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
BTW I find isActive() not very clear. isHighlyEditable() would be clearer.
Is there the equivalent of IS_EDITABLE in math?
None that I am aware of.
One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
a single flag for two orthogonal concepts ('can enter the thing' and
'has an associated dialog')
True. Could be two booleans instead.
And one of them is 'isActive()' (which probably {c,sh}ould be renamed)
Andre'
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 11:45:34PM +0200, Andre Poenitz wrote:
> On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
> > BTW I find isActive() not very clear. isHighlyEditable() would be clearer.
> > Is there the equivalent of IS_EDITABLE in math?
>
> None that I am aware of.
>
> One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
> a single flag for two orthogonal concepts ('can enter the thing' and
> 'has an associated dialog')
True. Could be two booleans instead.
- Martin
Re: [patch] fixing segfault because of empty coord cache
On Tue, Jun 12, 2007 at 09:18:45AM +0300, Martin Vermeer wrote:
> On Mon, Jun 11, 2007 at 11:45:34PM +0200, Andre Poenitz wrote:
> > On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
> > > BTW I find isActive() not very clear. isHighlyEditable() would be clearer.
> > > Is there the equivalent of IS_EDITABLE in math?
> >
> > None that I am aware of.
> >
> > One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
> > a single flag for two orthogonal concepts ('can enter the thing' and
> > 'has an associated dialog')
>
> True. Could be two booleans instead.
And one of them is 'isActive()' (which probably {c,sh}ould be renamed)
Andre'
Re: [patch] fixing segfault because of empty coord cache
On Sat, Jun 09, 2007 at 09:37:31AM +0300, Martin Vermeer wrote: On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: Stefan == Stefan Schimanski [EMAIL PROTECTED] writes: Stefan Critical bugs don't get less critical by ignorance. If anybody Stefan wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc Hear, hear. This kind of analysis should be in the code, as comment. Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? It's even in InsetBase, and something like that was the intention. Andre'
Re: [patch] fixing segfault because of empty coord cache
I can't tell you. It is conceptionally sound, however, so close to
1.5.0
it might be more prudent to use something like
The old behavior is simpler than I thought. A search for isActive
basically shows that there is no math inset (other than the
InsetMathMBox, which just return true, and InsetMathNest with nargs()
0) which overwrite the method:
BufferView.cpp: if (inset inset-isActive()) {
Cursor.cpp: if (!t-isActive())
Cursor.cpp: if (t-isActive()) {
DocIterator.cpp:if (n n-isActive()) {
insets/Inset.h: virtual bool isActive() const { return nargs() 0; }
insets/Inset.h.orig:virtual bool isActive() const { return nargs
() 0; }
mathed/InsetMathMBox.h: bool isActive() const { return true; }
mathed/InsetMathNest.cpp:bool InsetMathNest::isActive() const
mathed/InsetMathNest.h: bool isActive() const;
So isActive() is just equivalent to nargs() 0 at the place
mentioned in the patch. Hence replacing the nargs() 0 as proposed
is safe.
... !atom-isMathRefInset()
and put a fat FIXME there to ask fopr investigation in quieter times.
don't think it's necessary because of the comment above.
Stefan
PGP.sig
Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Andre == Andre Poenitz [EMAIL PROTECTED] writes: Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? Andre It's even in InsetBase, and something like that was the Andre intention. What is the difference between this and editable() == HIGHLY_EDITABLE? Do we need to have both? JMarc
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 08:56:45AM +0200, Stefan Schimanski wrote:
I can't tell you. It is conceptionally sound, however, so close to
1.5.0
it might be more prudent to use something like
The old behavior is simpler than I thought. A search for isActive
basically shows that there is no math inset (other than the
InsetMathMBox, which just return true, and InsetMathNest with nargs()
0) which overwrite the method:
BufferView.cpp: if (inset inset-isActive()) {
Cursor.cpp: if (!t-isActive())
Cursor.cpp: if (t-isActive()) {
DocIterator.cpp:if (n n-isActive()) {
insets/Inset.h: virtual bool isActive() const { return nargs() 0; }
insets/Inset.h.orig:virtual bool isActive() const { return nargs
() 0; }
mathed/InsetMathMBox.h: bool isActive() const { return true; }
mathed/InsetMathNest.cpp:bool InsetMathNest::isActive() const
mathed/InsetMathNest.h: bool isActive() const;
So isActive() is just equivalent to nargs() 0 at the place
mentioned in the patch. Hence replacing the nargs() 0 as proposed
is safe.
... !atom-isMathRefInset()
and put a fat FIXME there to ask fopr investigation in quieter times.
don't think it's necessary because of the comment above.
Ok then.
Andre'
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 09:47:13AM +0200, Jean-Marc Lasgouttes wrote: Andre == Andre Poenitz [EMAIL PROTECTED] writes: Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? Andre It's even in InsetBase, and something like that was the Andre intention. What is the difference between this and editable() == HIGHLY_EDITABLE? Historically isActive() was math and HIGHLY_EDITABLE was text. Do we need to have both? Not very likely ;-) Andre'
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 08:06:31PM +0200, Andre Poenitz wrote: On Mon, Jun 11, 2007 at 09:47:13AM +0200, Jean-Marc Lasgouttes wrote: Andre == Andre Poenitz [EMAIL PROTECTED] writes: Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? Andre It's even in InsetBase, and something like that was the Andre intention. What is the difference between this and editable() == HIGHLY_EDITABLE? Historically isActive() was math and HIGHLY_EDITABLE was text. Do we need to have both? Not very likely ;-) There's a FIXME for that in Inset.h. BTW I find isActive() not very clear. isHighlyEditable() would be clearer. Is there the equivalent of IS_EDITABLE in math? - Martin
Re: [patch] fixing segfault because of empty coord cache
Martin == Martin Vermeer [EMAIL PROTECTED] writes: Martin BTW I find isActive() not very clear. isHighlyEditable() would Martin be clearer. Is there the equivalent of IS_EDITABLE in math? A ref inset? JMarc
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
BTW I find isActive() not very clear. isHighlyEditable() would be clearer.
Is there the equivalent of IS_EDITABLE in math?
None that I am aware of.
One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
a single flag for two orthogonal concepts ('can enter the thing' and
'has an associated dialog')
Andre'
Re: [patch] fixing segfault because of empty coord cache
On Sat, Jun 09, 2007 at 09:37:31AM +0300, Martin Vermeer wrote: > On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: > > > "Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes: > > > > Stefan> Critical bugs don't get less critical by ignorance. If anybody > > Stefan> wants to know more: > > > > [snip] > > > > Great analysis! > > > > I would say that the fix is correct, but I'd wait for Andre's input. > > > > JMarc > > Hear, hear. This kind of analysis should be in the code, as comment. > > Do I understand correctly that this presupposes that 1) every > mathinset must have an isActive method and 2) it should return > true only if the inset has really accessible cells? It's even in InsetBase, and something like that was the intention. Andre'
Re: [patch] fixing segfault because of empty coord cache
I can't tell you. It is conceptionally sound, however, so close to
1.5.0
it might be more prudent to use something like
The old behavior is simpler than I thought. A search for isActive
basically shows that there is no math inset (other than the
InsetMathMBox, which just return true, and InsetMathNest with nargs()
> 0) which overwrite the method:
BufferView.cpp: if (inset && inset->isActive()) {
Cursor.cpp: if (!t->isActive())
Cursor.cpp: if (t->isActive()) {
DocIterator.cpp:if (n && n->isActive()) {
insets/Inset.h: virtual bool isActive() const { return nargs() > 0; }
insets/Inset.h.orig:virtual bool isActive() const { return nargs
() > 0; }
mathed/InsetMathMBox.h: bool isActive() const { return true; }
mathed/InsetMathNest.cpp:bool InsetMathNest::isActive() const
mathed/InsetMathNest.h: bool isActive() const;
So isActive() is just equivalent to nargs() > 0 at the place
mentioned in the patch. Hence replacing the nargs() > 0 as proposed
is safe.
... && !atom->isMathRefInset()
and put a fat FIXME there to ask fopr investigation in quieter times.
don't think it's necessary because of the comment above.
Stefan
PGP.sig
Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
> "Andre" == Andre Poenitz <[EMAIL PROTECTED]> writes: >> Do I understand correctly that this presupposes that 1) every >> mathinset must have an isActive method and 2) it should return true >> only if the inset has really accessible cells? Andre> It's even in InsetBase, and something like that was the Andre> intention. What is the difference between this and editable() == HIGHLY_EDITABLE? Do we need to have both? JMarc
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 08:56:45AM +0200, Stefan Schimanski wrote:
> >I can't tell you. It is conceptionally sound, however, so close to
> >1.5.0
> >it might be more prudent to use something like
>
> The old behavior is simpler than I thought. A search for isActive
> basically shows that there is no math inset (other than the
> InsetMathMBox, which just return true, and InsetMathNest with nargs()
> > 0) which overwrite the method:
>
> BufferView.cpp: if (inset && inset->isActive()) {
> Cursor.cpp: if (!t->isActive())
> Cursor.cpp: if (t->isActive()) {
> DocIterator.cpp:if (n && n->isActive()) {
> insets/Inset.h: virtual bool isActive() const { return nargs() > 0; }
> insets/Inset.h.orig:virtual bool isActive() const { return nargs
> () > 0; }
> mathed/InsetMathMBox.h: bool isActive() const { return true; }
> mathed/InsetMathNest.cpp:bool InsetMathNest::isActive() const
> mathed/InsetMathNest.h: bool isActive() const;
>
> So isActive() is just equivalent to nargs() > 0 at the place
> mentioned in the patch. Hence replacing the nargs() > 0 as proposed
> is safe.
>
> > ... && !atom->isMathRefInset()
> >
> >and put a fat FIXME there to ask fopr investigation in quieter times.
>
> don't think it's necessary because of the comment above.
Ok then.
Andre'
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 09:47:13AM +0200, Jean-Marc Lasgouttes wrote: > > "Andre" == Andre Poenitz <[EMAIL PROTECTED]> writes: > > >> Do I understand correctly that this presupposes that 1) every > >> mathinset must have an isActive method and 2) it should return true > >> only if the inset has really accessible cells? > > Andre> It's even in InsetBase, and something like that was the > Andre> intention. > > What is the difference between this and editable() == HIGHLY_EDITABLE? Historically isActive() was math and HIGHLY_EDITABLE was text. > Do we need to have both? Not very likely ;-) Andre'
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 08:06:31PM +0200, Andre Poenitz wrote: > On Mon, Jun 11, 2007 at 09:47:13AM +0200, Jean-Marc Lasgouttes wrote: > > > "Andre" == Andre Poenitz <[EMAIL PROTECTED]> writes: > > > > >> Do I understand correctly that this presupposes that 1) every > > >> mathinset must have an isActive method and 2) it should return true > > >> only if the inset has really accessible cells? > > > > Andre> It's even in InsetBase, and something like that was the > > Andre> intention. > > > > What is the difference between this and editable() == HIGHLY_EDITABLE? > > Historically isActive() was math and HIGHLY_EDITABLE was text. > > > Do we need to have both? > > Not very likely ;-) There's a FIXME for that in Inset.h. BTW I find isActive() not very clear. isHighlyEditable() would be clearer. Is there the equivalent of IS_EDITABLE in math? - Martin
Re: [patch] fixing segfault because of empty coord cache
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> BTW I find isActive() not very clear. isHighlyEditable() would Martin> be clearer. Is there the equivalent of IS_EDITABLE in math? A ref inset? JMarc
Re: [patch] fixing segfault because of empty coord cache
On Mon, Jun 11, 2007 at 09:46:27PM +0300, Martin Vermeer wrote:
> BTW I find isActive() not very clear. isHighlyEditable() would be clearer.
> Is there the equivalent of IS_EDITABLE in math?
None that I am aware of.
One of the problems I have with {IS,HIGHLY}_EDITABLE is that it is
a single flag for two orthogonal concepts ('can enter the thing' and
'has an associated dialog')
Andre'
Re: [patch] fixing segfault because of empty coord cache
On Fri, Jun 08, 2007 at 05:03:55PM +0200, Stefan Schimanski wrote: Critical bugs don't get less critical by ignorance. If anybody wants to know more: 1) CommandInset (used e.g. for references in mathed) has two cells to hold information about where it points to. But it only draws a button, not the cells themselves. So accessing the cells' coordinates in the cache during drawSelection segfaults. 2) This all happens only because, after creating the InsetMathRef (derived from CommandInset) with \ref, the cursor is inside of it. Hence, InsetMathNest will try to draw the selection inside the InsetMathRef below the cells. 3) So, why is the cursor inside it? Because it is derived from InsetMathNest and has = 1 cells. The LFUN_SELF_INSERT handler of the hull (in fact in InsetMathNest::doDispatch) takes this as a reason to put the cursor inside. I guess that's not correct, because the cursor- left behavior normally checks the isActive() method before entering an inset. But here isActive() is not called. Good analysis. So I propose the following patch (which fixes #3715). The first hunk is trivial. But I am not sure about the consequences of the second. I think it's the correct way to insert an inset, but maybe there are other inset which depend on the old (wrong) behavior. Comments? I can't tell you. It is conceptionally sound, however, so close to 1.5.0 it might be more prudent to use something like ... !atom-isMathRefInset() and put a fat FIXME there to ask fopr investigation in quieter times. The second part most definitely has a potential smell of brown paperbags... Andre'
Re: [patch] fixing segfault because of empty coord cache
On Fri, Jun 08, 2007 at 05:03:55PM +0200, Stefan Schimanski wrote: > Critical bugs don't get less critical by ignorance. > > If anybody wants to know more: > > 1) CommandInset (used e.g. for references in mathed) has two cells to > hold information about where it points to. But it only draws a > button, not the cells themselves. So accessing the cells' coordinates > in the cache during drawSelection segfaults. > 2) This all happens only because, after creating the InsetMathRef > (derived from CommandInset) with \ref, the cursor is inside of it. > Hence, InsetMathNest will try to draw the selection inside the > InsetMathRef below the cells. > 3) So, why is the cursor inside it? Because it is derived from > InsetMathNest and has >= 1 cells. The LFUN_SELF_INSERT handler of the > hull (in fact in InsetMathNest::doDispatch) takes this as a reason to > put the cursor inside. I guess that's not correct, because the cursor- > left behavior normally checks the isActive() method before entering > an inset. But here isActive() is not called. Good analysis. > So I propose the following patch (which fixes #3715). The first hunk > is trivial. But I am not sure about the consequences of the second. I > think it's the correct way to insert an inset, but maybe there are > other inset which depend on the old (wrong) behavior. Comments? I can't tell you. It is conceptionally sound, however, so close to 1.5.0 it might be more prudent to use something like ... && !atom->isMathRefInset() and put a fat FIXME there to ask fopr investigation in quieter times. The second part most definitely has a potential smell of brown paperbags... Andre'
Re: [patch] fixing segfault because of empty coord cache
On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: Stefan == Stefan Schimanski [EMAIL PROTECTED] writes: Stefan Critical bugs don't get less critical by ignorance. If anybody Stefan wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc Hear, hear. This kind of analysis should be in the code, as comment. Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? - Martin
Re: [patch] fixing segfault because of empty coord cache
Martin Vermeer wrote: On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: Stefan == Stefan Schimanski [EMAIL PROTECTED] writes: Stefan Critical bugs don't get less critical by ignorance. If anybody Stefan wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc Hear, hear. This kind of analysis should be in the code, as comment. Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? Not really. IIRC, the problem with mathed (as opposed to the other insets) is that the metrics are saved in the CoordCache at draw() time as opposed to updateMetrics() time. The real fix is to align this treatment for all insets. Abdel.
Re: [patch] fixing segfault because of empty coord cache
Am 09.06.2007 um 08:37 schrieb Martin Vermeer: On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: Stefan == Stefan Schimanski [EMAIL PROTECTED] writes: Stefan Critical bugs don't get less critical by ignorance. If anybody Stefan wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc Hear, hear. This kind of analysis should be in the code, as comment. Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? 1) You get it for free if you derive from InsetMathNest. It returns true if you have at least one cell. Any inset just derived from InsetMath, but not from InsetMathNest do not need it. 2) Yes, if you use a InsetMathNest with cells you better do not return true if you do not draw the cells. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: > > "Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes: > > Stefan> Critical bugs don't get less critical by ignorance. If anybody > Stefan> wants to know more: > > [snip] > > Great analysis! > > I would say that the fix is correct, but I'd wait for Andre's input. > > JMarc Hear, hear. This kind of analysis should be in the code, as comment. Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? - Martin
Re: [patch] fixing segfault because of empty coord cache
Martin Vermeer wrote: On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: "Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes: Stefan> Critical bugs don't get less critical by ignorance. If anybody Stefan> wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc Hear, hear. This kind of analysis should be in the code, as comment. Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? Not really. IIRC, the problem with mathed (as opposed to the other insets) is that the metrics are saved in the CoordCache at draw() time as opposed to updateMetrics() time. The real fix is to align this treatment for all insets. Abdel.
Re: [patch] fixing segfault because of empty coord cache
Am 09.06.2007 um 08:37 schrieb Martin Vermeer: On Fri, Jun 08, 2007 at 08:22:42PM +0200, Jean-Marc Lasgouttes wrote: "Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes: Stefan> Critical bugs don't get less critical by ignorance. If anybody Stefan> wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc Hear, hear. This kind of analysis should be in the code, as comment. Do I understand correctly that this presupposes that 1) every mathinset must have an isActive method and 2) it should return true only if the inset has really accessible cells? 1) You get it for free if you derive from InsetMathNest. It returns true if you have at least one cell. Any inset just derived from InsetMath, but not from InsetMathNest do not need it. 2) Yes, if you use a InsetMathNest with cells you better do not return true if you do not draw the cells. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Critical bugs don't get less critical by ignorance.
If anybody wants to know more:
1) CommandInset (used e.g. for references in mathed) has two cells to
hold information about where it points to. But it only draws a
button, not the cells themselves. So accessing the cells' coordinates
in the cache during drawSelection segfaults.
2) This all happens only because, after creating the InsetMathRef
(derived from CommandInset) with \ref, the cursor is inside of it.
Hence, InsetMathNest will try to draw the selection inside the
InsetMathRef below the cells.
3) So, why is the cursor inside it? Because it is derived from
InsetMathNest and has = 1 cells. The LFUN_SELF_INSERT handler of the
hull (in fact in InsetMathNest::doDispatch) takes this as a reason to
put the cursor inside. I guess that's not correct, because the cursor-
left behavior normally checks the isActive() method before entering
an inset. But here isActive() is not called.
So I propose the following patch (which fixes #3715). The first hunk
is trivial. But I am not sure about the consequences of the second. I
think it's the correct way to insert an inset, but maybe there are
other inset which depend on the old (wrong) behavior. Comments?
Stefan
Index: src/mathed/CommandInset.h
===
--- src/mathed/CommandInset.h (Revision 18719)
+++ src/mathed/CommandInset.h (Arbeitskopie)
@@ -40,6 +40,9 @@
virtual docstring const screenLabel() const;
///
docstring const commandname() const { return name_; }
+ ///
+ bool isActive() const { return false; }
+
private:
virtual std::auto_ptrInset doClone() const;
Index: src/mathed/InsetMathNest.cpp
===
--- src/mathed/InsetMathNest.cpp(Revision 18719)
+++ src/mathed/InsetMathNest.cpp(Arbeitskopie)
@@ -732,7 +732,7 @@
cur.inMacroMode() cur.macroName() != \\
cur.macroModeClose()) {
MathAtom const atom = cur.prevAtom();
- if (atom-asNestInset() atom-nargs() 0) {
+ if (atom-asNestInset() atom-isActive()) {
cur.posLeft();
cur.pushLeft(*cur.nextInset());
}
Am 04.06.2007 um 23:35 schrieb Jean-Marc Lasgouttes:
Stefan == Stefan Schimanski [EMAIL PROTECTED] writes:
Stefan Hmm, it's a not very complex patch for a critical bug about a
Stefan segfault and nobody has a comment?
Actually I am wondering why suddenly we have these crashes in very
simple operations (like breakParagraph this one). Isn't there
something else hidden?
This one has nothing to do with breakParagraph. It is IMO just non-
defensive programming which strikes back. The InsetMathNest just
assumes that all its cells are drawn (and hence are in the
coordinate cache). But this assumption shouldn't be made. If it is
made, we have to document it and check the derived classes. I am
pretty sure that someof them warm up the cache of all cells as a
consequence of patchworking against crashes in other
InsetMathMethods method (e.g. editXY).
Stefan
PGP.sig
Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Stefan == Stefan Schimanski [EMAIL PROTECTED] writes: Stefan Critical bugs don't get less critical by ignorance. If anybody Stefan wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc
Re: [patch] fixing segfault because of empty coord cache
Critical bugs don't get less critical by ignorance.
If anybody wants to know more:
1) CommandInset (used e.g. for references in mathed) has two cells to
hold information about where it points to. But it only draws a
button, not the cells themselves. So accessing the cells' coordinates
in the cache during drawSelection segfaults.
2) This all happens only because, after creating the InsetMathRef
(derived from CommandInset) with \ref, the cursor is inside of it.
Hence, InsetMathNest will try to draw the selection inside the
InsetMathRef below the cells.
3) So, why is the cursor inside it? Because it is derived from
InsetMathNest and has >= 1 cells. The LFUN_SELF_INSERT handler of the
hull (in fact in InsetMathNest::doDispatch) takes this as a reason to
put the cursor inside. I guess that's not correct, because the cursor-
left behavior normally checks the isActive() method before entering
an inset. But here isActive() is not called.
So I propose the following patch (which fixes #3715). The first hunk
is trivial. But I am not sure about the consequences of the second. I
think it's the correct way to insert an inset, but maybe there are
other inset which depend on the old (wrong) behavior. Comments?
Stefan
Index: src/mathed/CommandInset.h
===
--- src/mathed/CommandInset.h (Revision 18719)
+++ src/mathed/CommandInset.h (Arbeitskopie)
@@ -40,6 +40,9 @@
virtual docstring const screenLabel() const;
///
docstring const & commandname() const { return name_; }
+ ///
+ bool isActive() const { return false; }
+
private:
virtual std::auto_ptr doClone() const;
Index: src/mathed/InsetMathNest.cpp
===
--- src/mathed/InsetMathNest.cpp(Revision 18719)
+++ src/mathed/InsetMathNest.cpp(Arbeitskopie)
@@ -732,7 +732,7 @@
&& cur.inMacroMode() && cur.macroName() != "\\"
&& cur.macroModeClose()) {
MathAtom const atom = cur.prevAtom();
- if (atom->asNestInset() && atom->nargs() > 0) {
+ if (atom->asNestInset() && atom->isActive()) {
cur.posLeft();
cur.pushLeft(*cur.nextInset());
}
Am 04.06.2007 um 23:35 schrieb Jean-Marc Lasgouttes:
"Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes:
Stefan> Hmm, it's a not very complex patch for a critical bug about a
Stefan> segfault and nobody has a comment?
Actually I am wondering why suddenly we have these crashes in very
simple operations (like breakParagraph this one). Isn't there
something else hidden?
This one has nothing to do with breakParagraph. It is IMO just non-
defensive programming which strikes back. The InsetMathNest just
assumes that all its cells are drawn (and hence are in the
coordinate cache). But this assumption shouldn't be made. If it is
made, we have to document it and check the derived classes. I am
pretty sure that someof them warm up the cache of all cells as a
consequence of patchworking against crashes in other
InsetMathMethods method (e.g. editXY).
Stefan
PGP.sig
Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
> "Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes: Stefan> Critical bugs don't get less critical by ignorance. If anybody Stefan> wants to know more: [snip] Great analysis! I would say that the fix is correct, but I'd wait for Andre's input. JMarc
Re: [patch] fixing segfault because of empty coord cache
Am 04.06.2007 um 23:35 schrieb Jean-Marc Lasgouttes: Stefan == Stefan Schimanski [EMAIL PROTECTED] writes: Stefan Hmm, it's a not very complex patch for a critical bug about a Stefan segfault and nobody has a comment? Actually I am wondering why suddenly we have these crashes in very simple operations (like breakParagraph this one). Isn't there something else hidden? This one has nothing to do with breakParagraph. It is IMO just non- defensive programming which strikes back. The InsetMathNest just assumes that all its cells are drawn (and hence are in the coordinate cache). But this assumption shouldn't be made. If it is made, we have to document it and check the derived classes. I am pretty sure that someof them warm up the cache of all cells as a consequence of patchworking against crashes in other InsetMathMethods method (e.g. editXY). Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Am 04.06.2007 um 23:35 schrieb Jean-Marc Lasgouttes: "Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes: Stefan> Hmm, it's a not very complex patch for a critical bug about a Stefan> segfault and nobody has a comment? Actually I am wondering why suddenly we have these crashes in very simple operations (like breakParagraph this one). Isn't there something else hidden? This one has nothing to do with breakParagraph. It is IMO just non- defensive programming which strikes back. The InsetMathNest just assumes that all its cells are drawn (and hence are in the coordinate cache). But this assumption shouldn't be made. If it is made, we have to document it and check the derived classes. I am pretty sure that someof them warm up the cache of all cells as a consequence of patchworking against crashes in other InsetMathMethods method (e.g. editXY). Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Hmm, it's a not very complex patch for a critical bug about a segfault and nobody has a comment? Stefan Any consensus about this one? It still crashes in RC1. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Stefan == Stefan Schimanski [EMAIL PROTECTED] writes: Stefan Hmm, it's a not very complex patch for a critical bug about a Stefan segfault and nobody has a comment? Actually I am wondering why suddenly we have these crashes in very simple operations (like breakParagraph this one). Isn't there something else hidden? JMarc
Re: [patch] fixing segfault because of empty coord cache
Hmm, it's a not very complex patch for a critical bug about a segfault and nobody has a comment? Stefan Any consensus about this one? It still crashes in RC1. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
> "Stefan" == Stefan Schimanski <[EMAIL PROTECTED]> writes: Stefan> Hmm, it's a not very complex patch for a critical bug about a Stefan> segfault and nobody has a comment? Actually I am wondering why suddenly we have these crashes in very simple operations (like breakParagraph this one). Isn't there something else hidden? JMarc
Re: [patch] fixing segfault because of empty coord cache
Any consensus about this one? It still crashes in RC1. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Any consensus about this one? It still crashes in RC1. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Stefan Schimanski wrote: Hi! Here is a patch for a crash that happens due to a cell not in the coord cache during the drawing of the selection. It could be that this is related (and also fixes) http://bugzilla.lyx.org/show_bug.cgi?id=3715 . I believe the problem is when an inset derived from InsetMathNest does not draw all its cells, and hence a cell is not in the coord cache yet. Then the warm up call in InsetMathNest::drawSelection cannot get the cell into the cache and the loop over all cells at the bottom of this very function will trigger an assertion. You can trigger this crash in Beta 3 or 1.5svn like this: New document, Ctrl-M \ref space shift-right With this patch the shift-right seems to have no effect. Haven't checked why. But at least the segfault is gone. Probably because the cell is not in the cache. The patch looks good and safe. Abdel.
Re: [patch] fixing segfault because of empty coord cache
On Wed, May 30, 2007 at 05:56:35PM +0200, Abdelrazak Younes wrote: Stefan Schimanski wrote: Hi! Here is a patch for a crash that happens due to a cell not in the coord cache during the drawing of the selection. It could be that this is related (and also fixes) http://bugzilla.lyx.org/show_bug.cgi?id=3715 . I believe the problem is when an inset derived from InsetMathNest does not draw all its cells, and hence a cell is not in the coord cache yet. Then the warm up call in InsetMathNest::drawSelection cannot get the cell into the cache and the loop over all cells at the bottom of this very function will trigger an assertion. You can trigger this crash in Beta 3 or 1.5svn like this: New document, Ctrl-M \ref space shift-right With this patch the shift-right seems to have no effect. Haven't checked why. But at least the segfault is gone. Probably because the cell is not in the cache. The patch looks good and safe. An interesting question might be: Why was it not in the cache?. Andre'
Re: [patch] fixing segfault because of empty coord cache
Am 30.05.2007 um 20:32 schrieb Andre Poenitz: On Wed, May 30, 2007 at 05:56:35PM +0200, Abdelrazak Younes wrote: Stefan Schimanski wrote: Hi! Here is a patch for a crash that happens due to a cell not in the coord cache during the drawing of the selection. It could be that this is related (and also fixes) http://bugzilla.lyx.org/show_bug.cgi? id=3715 . I believe the problem is when an inset derived from InsetMathNest does not draw all its cells, and hence a cell is not in the coord cache yet. Then the warm up call in InsetMathNest::drawSelection cannot get the cell into the cache and the loop over all cells at the bottom of this very function will trigger an assertion. You can trigger this crash in Beta 3 or 1.5svn like this: New document, Ctrl-M \ref space shift-right With this patch the shift-right seems to have no effect. Haven't checked why. But at least the segfault is gone. Probably because the cell is not in the cache. The patch looks good and safe. An interesting question might be: Why was it not in the cache?. My guess is that the ref was empty and therefore not drawn. Feel free to take your favourite debugger to confirm. But I think that's not too uncommon that a cell is not drawn I think. Also in the MathMacros don't do it if a argument does not appear in the definition. There might be others. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] fixing segfault because of empty coord cache
Stefan Schimanski wrote: Hi! Here is a patch for a crash that happens due to a cell not in the coord cache during the drawing of the selection. It could be that this is related (and also fixes) http://bugzilla.lyx.org/show_bug.cgi?id=3715 . I believe the problem is when an inset derived from InsetMathNest does not draw all its cells, and hence a cell is not in the coord cache yet. Then the warm up call in InsetMathNest::drawSelection cannot get the cell into the cache and the loop over all cells at the bottom of this very function will trigger an assertion. You can trigger this crash in Beta 3 or 1.5svn like this: New document, Ctrl-M \ref With this patch the seems to have no effect. Haven't checked why. But at least the segfault is gone. Probably because the cell is not in the cache. The patch looks good and safe. Abdel.
Re: [patch] fixing segfault because of empty coord cache
On Wed, May 30, 2007 at 05:56:35PM +0200, Abdelrazak Younes wrote: > Stefan Schimanski wrote: > >Hi! > > > >Here is a patch for a crash that happens due to a cell not in the coord > >cache during the drawing of the selection. It could be that this is > >related (and also fixes) http://bugzilla.lyx.org/show_bug.cgi?id=3715 . > > > >I believe the problem is when an inset derived from InsetMathNest does > >not draw all its cells, and hence a cell is not in the coord cache yet. > >Then the warm up call in InsetMathNest::drawSelection cannot get the > >cell into the cache and the loop over all cells at the bottom of this > >very function will trigger an assertion. > > > >You can trigger this crash in Beta 3 or 1.5svn like this: > > > >New document, Ctrl-M \ref > > > >With this patch the seems to have no effect. Haven't > >checked why. But at least the segfault is gone. > > Probably because the cell is not in the cache. The patch looks good and > safe. An interesting question might be: "Why was it not in the cache?". Andre'
Re: [patch] fixing segfault because of empty coord cache
Am 30.05.2007 um 20:32 schrieb Andre Poenitz: On Wed, May 30, 2007 at 05:56:35PM +0200, Abdelrazak Younes wrote: Stefan Schimanski wrote: Hi! Here is a patch for a crash that happens due to a cell not in the coord cache during the drawing of the selection. It could be that this is related (and also fixes) http://bugzilla.lyx.org/show_bug.cgi? id=3715 . I believe the problem is when an inset derived from InsetMathNest does not draw all its cells, and hence a cell is not in the coord cache yet. Then the warm up call in InsetMathNest::drawSelection cannot get the cell into the cache and the loop over all cells at the bottom of this very function will trigger an assertion. You can trigger this crash in Beta 3 or 1.5svn like this: New document, Ctrl-M \ref With this patch the seems to have no effect. Haven't checked why. But at least the segfault is gone. Probably because the cell is not in the cache. The patch looks good and safe. An interesting question might be: "Why was it not in the cache?". My guess is that the ref was empty and therefore not drawn. Feel free to take your favourite debugger to confirm. But I think that's not too uncommon that a cell is not drawn I think. Also in the MathMacros don't do it if a argument does not appear in the definition. There might be others. Stefan PGP.sig Description: Signierter Teil der Nachricht
