Le 30/12/2016 à 17:11, Richard Heck a écrit :
On 12/29/2016 01:21 AM, Guillaume Munch wrote:
Le 27/12/2016 à 23:11, Scott Kostyshak a écrit :
On Tue, May 31, 2016 at 01:15:38AM +0200, Guillaume Munch wrote:
commit 1cc14a31ca8320d881b674f93c34a09cf1666cca
Author: Guillaume Munch <g...@lyx.org>
Date: Mon May 30 21:42:08 2016 +0100
TocWidget: fix an erroneous collapse and optimise updates based
on profiling
TocModels::reset() in GuiView::structureChanged() collapses the
TocWidget, and
therefore requires an update right after, which was missing.
In fact, profiling TocWidget::updateView() shows that delaying
the update is
good only for fast keypresses (essentially movement). It costs
5% of a
char-forward operation in a document with approx. 100 table of
contents
items. The update optimisation has been rewritten to take this
data into
account.
A git bisect suggests this causes a SIGSEGV. To reproduce:
1. open the attached file
2. open the outline pane
3. put the cursor just to the left of "Hello" (but inside the caption
inset)
4. "Save As" to a new filename.
Can you reproduce?
Backtrace is attached.
Hi,
The root of the issue is in Buffer::reload() called during "Save As".
After loadLyXFile() there, all the insets have been deleted, and
therefore the Inset pointer
GuiView::documentBufferView()->cursor().inset() is dangling. Immediately
after loadLyXFile(), reload() calls updateBuffer() from your backtrace
which then crashes.
While debugging I got other segfaults caused by the same dangling Inset
pointer in Cursor, notably: 1) a trace identical to the second one from
<http://www.lyx.org/trac/ticket/10520>, and 2) a repeated segfault in
the critical path (a call to inMathed()) which closes LyX instantly
after the first segfault is caught (luckily after the emergency save is
complete).
I am trying to find a proper fix for this issue, but the strategy for
managing these Inset pointers in CursorSlice escapes me. Does anyone
know which assumptions are made on when these pointers must be valid?
If there are such assumptions, how are they supposed to be enforced?
The attached patch fixes this crash, but I find very unsatisfactory to
have to update the cursors by hand like this.
I don't know the answer to those questions, but I think that something
like this
solution is all right. The problem is that we would ordinarily suppose
that, after
reading the document, we have a brand new cursor in the open view. But
that's
not true in this case, because the document has been reloaded.
It's probably fine to put the updateCursors() call in readDocument, as
you have
it. But it can also be put into Buffer::reload, right before the call to
updateBuffer.
That's where we actually need it, it seems to me.
Thanks for the patch. It is the same as calling fixIfBroken in
GuiView::structureChanged (as attached) which I agree becomes even
simpler. But, provided one accepts the idea that DocIterators hold
perishable pointers that need to be fixed (I do not see any way out of
it), I find it better to fix them the moment we know that they are
broken. However, I still miss how and when all the cursors not connected
to the GuiView are being fixed, though. Any idea?
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 90b43eb..76911f8 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -1652,6 +1652,8 @@ void GuiView::updateTocItem(string const & type, DocIterator const & dit)
void GuiView::structureChanged()
{
+ if (documentBufferView())
+ documentBufferView()->cursor().fixIfBroken();
// FIXME: This is slightly expensive, though less than the tocBackend update
// (#9880). This also resets the view in the Toc Widget (#6675).
d.toc_models_.reset(documentBufferView());
