Re: [DIAGNOSIS] Re: crash in trunk
On 04/24/2011 06:02 AM, Edwin Leuven wrote: Richard Heck: What the usual calls to updateBuffer() do is always make the macro context a ParIterator that points to the first paragraph in the current inset. (See e.g. the calls at InsetText:682,695.) ok. makes me wonder though why it is necessary to set the macro context explicitly. I have wondered this too, but I ran into this same sort of problem in the XHTML output routine in InsetMathHull. To make the preview image, I needed a DocIterator pointing right at that inset. This is not something it seems to be easy to construct on the fly, so I had to arrange for the information to be cached, in effect, via the recordLocation() routine. I think the macro context must be similar. So I'm attaching a few patches: 1. updateBuffer.diff solves the general problem mentioned. 2. x2a.patch is what seems to be a working version of x2.patch. 3. x3a.diff, which is a version of x3.patch that fixes the problem in the old code. I'm not sure what I prefer here, but (1) seems necessary no matter what. I guess (3) is more cautious, and if there aren't other issues for which (2) is needed, maybe that's what makes sense. the main difference between (2) and (3) is the label updating in updateBuffer? And all the other things updateBuffer does, though it's not doing them properly here, since e.g. any counters wouldn't be properly set. This probably doesn't matter for LaTeX export, but it does mean that none of these strategies would work reliably for XHTML export. The difference is that, in the LaTeX case, we do not rely much upon these sorts of internal settings, whereas we very much do in the other case. i am working on my document with (1) + (3) here, and have encountered no problems so far OK. Let's wait until Monday and see if anyone else has any input. I guess you and I are agreed on (3). thanks for helping out with the tricky stuff Of course. Richard
Re: [DIAGNOSIS] Re: crash in trunk
Richard Heck : > What the usual calls to updateBuffer() do is always > make the macro context a ParIterator that points to > the first paragraph in the current inset. (See e.g. > the calls at InsetText:682,695.) ok. makes me wonder though why it is necessary to set the macro context explicitly. > So I'm attaching a few patches: > > 1. updateBuffer.diff solves the general problem mentioned. > 2. x2a.patch is what seems to be a working version of x2.patch. > 3. x3a.diff, which is a version of x3.patch that fixes the > problem in the > old code. > > I'm not sure what I prefer here, but (1) seems necessary no > matter what. I > guess (3) is more cautious, and if there aren't other issues > for which (2) > is needed, maybe that's what makes sense. the main difference between (2) and (3) is the label updating in updateBuffer? i am working on my document with (1) + (3) here, and have encountered no problems so far thanks for helping out with the tricky stuff ed.
Re: [DIAGNOSIS] Re: crash in trunk
Il 23/04/2011 19:52, Edwin Leuven ha scritto: Richard Heck wrote: I guess the other idea would be to recurse through all the insets. attached, and no more crashes. i think i prefer this solution (although i am not sure we need to add the code to insettext) ed. FYI, I just added a test-case for this bug development/autotests/tabular-footnote-in.txt in the autotests machinery (ok, probably used only by myself :-) ), so this can remain in the set of regression tests for the future (after some fix will have been committed). T.
Re: [DIAGNOSIS] Re: crash in trunk
On 04/23/2011 02:07 PM, John McCabe-Dansted wrote: On Sat, Apr 23, 2011 at 11:45 PM, Edwin Leuven wrote: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 In case this is relevant, this assertion also shows up in: http://gmatht.homelinux.net/xp/keytest/html_out/out/t18//html/1303123297.html Thanks for the tip, but it looks unrelated. Richard
Re: [DIAGNOSIS] Re: crash in trunk
Regarding the x2.patch, the problem is that par_iterator_begin() does
not give us a "complete" DocIterator (one starting with the Buffer's own
InsetText, etc), which is what we need. I am constantly running into
this problem!
Regarding the other patch, adding the code to InsetText just seemed like
the easiest way to get the recursion going. Other ways would be fine
with me.
But I think there are some problems in the original code about how the
macro context is set. What the usual calls to updateBuffer() do is
always make the macro context a ParIterator that points to the first
paragraph in the current inset. (See e.g. the calls at
InsetText:682,695.) The existing code sets it to the first paragraph of
the *original* inset---the one that gets split or whatever---which just
seems like it has to be wrong.
In trying to fix both these issues, I found out that
cellInset(cell)->getText(0)->macrocontextPosition()
is actually an EMPTY DocIterator. This is likely due to the fact that
the Buffer has been cloned (at least here). That seems like an
independent problem and one that very much needs to be fixed. It's easy
to do that by adding an updateBuffer() call to the makeLaTeXFile()
routine, and it seems like a good idea to do that anyway, even if we're
not cloning the Buffer. The point is that, if you enter a footnote, say,
and then View>PDF, there won't be an updateBuffer() call before then,
and there probably needs to be.
So I'm attaching a few patches:
1. updateBuffer.diff solves the general problem mentioned.
2. x2a.patch is what seems to be a working version of x2.patch.
3. x3a.diff, which is a version of x3.patch that fixes the problem in
the old code.
I'm not sure what I prefer here, but (1) seems necessary no matter what.
I guess (3) is more cautious, and if there aren't other issues for which
(2) is needed, maybe that's what makes sense.
Richard
Index: src/Buffer.cpp
===
--- src/Buffer.cpp (revision 38484)
+++ src/Buffer.cpp (working copy)
@@ -1346,7 +1346,9 @@
// Don't move this behind the parent_buffer=0 code below,
// because then the macros will not get the right "redefinition"
// flag as they don't see the parent macros which are output before.
- updateMacros();
+ // NOTE This probably should be an OutputUpdate, but that is not
+ // significant at the moment for LaTeX.
+ updateBuffer(UpdateMaster, InternalUpdate);
// fold macros if possible, still with parent buffer as the
// macros will be put in the prefix anyway.
Index: src/insets/InsetTabular.cpp
===
--- src/insets/InsetTabular.cpp (revision 38484)
+++ src/insets/InsetTabular.cpp (working copy)
@@ -522,7 +522,7 @@
DocIterator dit = doc_iterator_begin(&head.buffer(), &head);
for (; dit; dit.forwardChar())
- if (dit.inTexted() && dit.depth()==1
+ if (dit.inTexted() && dit.depth() == 1
&& dit.paragraph().find(align_d, false, false, dit.pos()))
break;
@@ -2484,14 +2484,19 @@
&& cellInfo(cell).decimal_width != 0) {
// copy cell and split in 2
InsetTableCell head = InsetTableCell(*cellInset(cell).get());
- head.getText(0)->setMacrocontextPosition(
-cellInset(cell)->getText(0)->macrocontextPosition());
head.setBuffer(buffer());
+ DocIterator dit = cellInset(cell)->getText(0)->macrocontextPosition();
+ dit.pop_back();
+ dit.push_back(CursorSlice (head));
+ ParIterator pit(dit);
+ buffer().updateBuffer(pit, OutputUpdate);
bool hassep = false;
InsetTableCell tail = splitCell(head, column_info[c].decimal_point, hassep);
- tail.getText(0)->setMacrocontextPosition(
-head.getText(0)->macrocontextPosition());
- tail.setBuffer(head.buffer());
+ tail.setBuffer(buffer());
+ dit.pop_back();
+ dit.push_back(CursorSlice(tail));
+ pit = ParIterator(dit);
+ buffer().updateBuffer(pit, OutputUpdate);
head.latex(os, newrp);
os << '&';
tail.latex(os, newrp);
@@ -3465,30 +3470,37 @@
mi.base.bv->textMetrics(tabular.cellInset(cell)->getText(0));
// determine horizontal offset because of decimal align (if necessary)
- int decimal_hoffset = 0;
int decimal_width = 0;
if (tabular.getAlignment(cell) == LYX_ALIGN_DECIMAL) {
-// make a copy which we will split in 2
-InsetTableCell head = InsetTableCell(*tabular.cellInset(cell).get());
-head.getText(0)->setMacrocontextPosition(
- tabular.cellInset(cell)->getText(0)->macrocontextPosition());
-head.setBuffer(tabular.buffer());
-// split in 2 and calculate width of each part
-bool hassep = false;
-InsetTableCell tail =
- splitCell(head, tabular.column_info[c].decimal_point, hassep);
-tail.getText(0)->setMacrocontextPosition(
- head.getText(0)->macrocontextPosition());
-tail.setBuffer(head.buffer());
-Dimension dim1;
-head.metrics(m, dim1);
-decimal_hoffset = dim1.width();
-if (hassep) {
+
Re: [DIAGNOSIS] Re: crash in trunk
On Sat, Apr 23, 2011 at 11:45 PM, Edwin Leuven wrote: > ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false > VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 In case this is relevant, this assertion also shows up in: http://gmatht.homelinux.net/xp/keytest/html_out/out/t18//html/1303123297.html -- John C. McCabe-Dansted
Re: [DIAGNOSIS] Re: crash in trunk
Richard Heck wrote: > I guess the other idea would be to recurse > through all the insets. attached, and no more crashes. i think i prefer this solution (although i am not sure we need to add the code to insettext) ed. x3.patch Description: Binary data
Re: [DIAGNOSIS] Re: crash in trunk
Richard Heck wrote:
> Can you post this version of the patch
> and I'll try to have a look?
attached
> I'm wondering where these comparisons are
> being made and whether we need to do
> something to the cursor. But I'm really
> just guessing here.
very frustating this poking in the dark...
> I guess the other idea would be to recurse
> through all the insets. Something like (pseudocode):
>
> InsetText::setMacroContextRecursive()
> {
> text_->setMacrocontextPosition(...); // what goes here?
> foreach inset in text_ {
> if (inset.asInsetText())
> inset.asInsetText()->setMacroContextRecursive();
> }
> }
>
> and then call this from InsetTabular.
>
> Frankly, I'm guessing at this point, and
> I never seem to know where to get
> the DocIterator I need. Hence the "..." above.
from the code in buffer.h it seems updateBuffer also sets the initial
pariter for ...
> I'm running in and out of the house at the
> moment but can play with this some later.
thanks!
ed.
x2.patch
Description: Binary data
Re: [DIAGNOSIS] Re: crash in trunk
On 04/23/2011 11:45 AM, Edwin Leuven wrote:
I wonder if we could try:
ParagraphList::const_iterator pit =
tail.getText(0)->paragraphs().begin();
buffer().updateBuffer(pit, OutputUpdate);
i am not sure i completely follow you, but when i use
buffer().updateBuffer(par_iterator_begin(tail), OutputUpdate);
instead of the code above, the crashes are gone (also with nested insets)
That was the idea.
but now i get the asserts below
Can you post this version of the patch and I'll try to have a look? I'm
wondering where these comparisons are being made and whether we need to
do something to the cursor. But I'm really just guessing here.
I guess the other idea would be to recurse through all the insets.
Something like (pseudocode):
InsetText::setMacroContextRecursive()
{
text_->setMacrocontextPosition(...); // what goes here?
foreach inset in text_ {
if (inset.asInsetText())
inset.asInsetText()->setMacroContextRecursive();
}
}
and then call this from InsetTabular.
Frankly, I'm guessing at this point, and I never seem to know where to
get the DocIterator I need. Hence the "..." above.
I'm running in and out of the house at the moment but can play with this
some later.
Richard
Re: [DIAGNOSIS] Re: crash in trunk
On 04/23/2011 11:45 AM, Edwin Leuven wrote:
I wonder if we could try:
ParagraphList::const_iterator pit =
tail.getText(0)->paragraphs().begin();
buffer().updateBuffer(pit, OutputUpdate);
i am not sure i completely follow you, but when i use
buffer().updateBuffer(par_iterator_begin(tail), OutputUpdate);
instead of the code above, the crashes are gone (also with nested insets)
That was the idea.
but now i get the asserts below
Can you post this version of the patch and I'll try to have a look? I'm
wondering where these comparisons are being made and whether we need to
do something to the cursor. But I'm really just guessing here.
I guess the other idea would be to recurse through all the insets.
Something like (pseudocode):
InsetText::setMacroContextRecursive()
{
text_->setMacrocontextPosition(...); // what goes here?
foreach inset in text_ {
if (inset.asInsetText())
inset.asInsetText()->setMacroContextRecursive();
}
}
and then call this from InsetTabular.
Frankly, I'm guessing at this point, and I never seem to know where to
get the DocIterator I need. Hence the "..." above.
Richard
Re: [DIAGNOSIS] Re: crash in trunk
Richard Heck wrote: > My main comment would be that I suspect, but of course > couldn't verify, due to the crashes, that the same > problem exists in the TexRow() output routine, i checked that previewing the document and having the source view pane open worked my guess was that things are ok as long as there is no metrics call behind texrow()'s back but i agree that it is better to fix it in every instance > Is that sufficient? i thought that insetList() provided a list of all nested insets... > What about a Note nested within the Footnote? ...but you're right, it doesn't which means that we will need to traverse all insets :/ > I wonder if we could try: > ParagraphList::const_iterator pit = > tail.getText(0)->paragraphs().begin(); > buffer().updateBuffer(pit, OutputUpdate); i am not sure i completely follow you, but when i use buffer().updateBuffer(par_iterator_begin(tail), OutputUpdate); instead of the code above, the crashes are gone (also with nested insets) but now i get the asserts below ed. 17:40:02.216: (file-open C:/Users/Edwin Leuven/Dropbox/newfile1.lyx)..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor and anchor in different insets p: ..\..\..\lyx-devel\src\support\lassert.cpp(21): ASSERTION false VIOLATED IN ..\..\lyx-devel\src\CursorSlice.cpp:188 ..\..\lyx-devel\src\CursorSlice.cpp(187): can't compare cursor
Re: [DIAGNOSIS] Re: crash in trunk
On 04/23/2011 06:56 AM, Edwin Leuven wrote:
Richard Heck wrote:
Answer: No, probably not, because it's hard to see
how to do this without making copies, which would
just give us the same problem back again.
the attached avoids the crash for me by setting the macrocontext for
nested insets (and only doing the metrics call on the tail; we don't
need the one on head since we did a metrics call before on the whole
cell)
comments welcome (needed)
i thinks this needs to be fixed before 2.0
Definitely.
My main comment would be that I suspect, but of course couldn't verify,
due to the crashes, that the same problem exists in the TexRow() output
routine, since I found last night that the same code is there. I do not
know if this sort of idea will be sufficient to fix that problem, too,
though it might be. My worry is that there is something that happens
during the updateBuffer() routine that will need doing.
Other comments
+ InsetTableCell tail =
InsetTableCell(*tabular.cellInset(cell).get());
+ tail.setBuffer(tabular.buffer());
+
+ // we need to set macrocontext position everywhere
+ // otherwise we crash with nested insets (e.g. footnotes)
+ // after decimal point
+ DocIterator mpos =
+ tabular.cellInset(cell)->getText(0)->macrocontextPosition();
const?
+ tail.getText(0)->setMacrocontextPosition(mpos);
+
+ ParagraphList::const_iterator pit =
tail.getText(0)->paragraphs().begin();
+ ParagraphList::const_iterator pend =
tail.getText(0)->paragraphs().end();
+ for (; pit != pend; ++pit) {
+ InsetList::const_iterator iit = pit->insetList().begin();
+ InsetList::const_iterator end = pit->insetList().end();
+ for (; iit != end; ++iit) {
+ if (iit->inset->asInsetText()) {
+ Text * itext = iit->inset->asInsetText()->getText(0);
+ itext->setMacrocontextPosition(mpos);
+ }
+ }
+ }
+
Is that sufficient? What about a Note nested within the Footnote?
I wonder if we could try:
ParagraphList::const_iterator pit =
tail.getText(0)->paragraphs().begin();
buffer().updateBuffer(pit, OutputUpdate);
in place of the above. It looks dangerous, because the insets are not
really part of the Buffer, but I think it will be OK in the context of
LaTeX output. (I didn't try this earlier, because I wasn't sure how to
get the ParIterator.) If this worked, then it would, I am pretty
confident, also solve the problem in TexRow(), as well.
rh
Re: [DIAGNOSIS] Re: crash in trunk
Richard Heck wrote: > Answer: No, probably not, because it's hard to see > how to do this without making copies, which would > just give us the same problem back again. the attached avoids the crash for me by setting the macrocontext for nested insets (and only doing the metrics call on the tail; we don't need the one on head since we did a metrics call before on the whole cell) comments welcome (needed) i thinks this needs to be fixed before 2.0 ed. x.patch Description: Binary data
Re: [DIAGNOSIS] Re: crash in trunk
On 04/22/2011 10:42 PM, Richard Heck wrote: On 04/22/2011 05:24 PM, Edwin Leuven wrote: Richard Heck wrote: I think I know what's happening here. sounds reasonable indeed We might somehow need to call updateBuffer() on these new insets. or find a way of calculating the width of the insettext before and after the decimal separator without making a copy, chopping it into two and doing a metrics call on each part... don't know how drawing of selection works, but maybe we can set a selection from the start of the inset to the decimal separator, do a metrics call on the inset and back out the length of the selection with the total width of the inset we should have the info to draw the cells in the column properly aligned Would it be possible temporarily to delete everything after the decimal, do the metrics on that, and then restore everything to its previous state? Answer: No, probably not, because it's hard to see how to do this without making copies, which would just give us the same problem back again. rh
Re: [DIAGNOSIS] Re: crash in trunk
On 04/22/2011 05:24 PM, Edwin Leuven wrote: Richard Heck wrote: I think I know what's happening here. sounds reasonable indeed We might somehow need to call updateBuffer() on these new insets. or find a way of calculating the width of the insettext before and after the decimal separator without making a copy, chopping it into two and doing a metrics call on each part... don't know how drawing of selection works, but maybe we can set a selection from the start of the inset to the decimal separator, do a metrics call on the inset and back out the length of the selection with the total width of the inset we should have the info to draw the cells in the column properly aligned Would it be possible temporarily to delete everything after the decimal, do the metrics on that, and then restore everything to its previous state? I really don't know much about the metrics stuff. Richard ed.
Re: [DIAGNOSIS] Re: crash in trunk
Richard Heck wrote: > I think I know what's happening here. sounds reasonable indeed > We might somehow need to call updateBuffer() > on these new insets. or find a way of calculating the width of the insettext before and after the decimal separator without making a copy, chopping it into two and doing a metrics call on each part... don't know how drawing of selection works, but maybe we can set a selection from the start of the inset to the decimal separator, do a metrics call on the inset and back out the length of the selection with the total width of the inset we should have the info to draw the cells in the column properly aligned ed.
