Re: [ugly patch] fix bug 9418
Le 01/04/2015 21:49, Georg Baum a écrit : Jean-Marc Lasgouttes wrote: Le 31/03/2015 21:38, Georg Baum a écrit : BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. I know that this needs work, but I don't see an obvious miracle:-( Currently we also have the strange mixture of lazy and non-lazy macros. What do you mean by lazy/non-lazy? See src/mathed/MacroTable.h. There is one constructor of MacroData documented as lazy, and one documented as non-lazy. I see. My plan was just to skip Buffer::updateMacros() in general and just execute it as needed whhen getMacro is invoked. But now I see the mess in MacroData::expand and I do not understand anymore why there is a need for another badly written loop that goes all over the buffer. JMarc
Re: [ugly patch] fix bug 9418
Le 01/04/2015 21:49, Georg Baum a écrit : Jean-Marc Lasgouttes wrote: Le 31/03/2015 21:38, Georg Baum a écrit : BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. I know that this needs work, but I don't see an obvious miracle:-( Currently we also have the strange mixture of lazy and non-lazy macros. What do you mean by lazy/non-lazy? See src/mathed/MacroTable.h. There is one constructor of MacroData documented as lazy, and one documented as non-lazy. I see. My plan was just to skip Buffer::updateMacros() in general and just execute it as needed whhen getMacro is invoked. But now I see the mess in MacroData::expand and I do not understand anymore why there is a need for another badly written loop that goes all over the buffer. JMarc
Re: [ugly patch] fix bug 9418
On 04/05/2015 11:01 AM, Georg Baum wrote: Georg Baum wrote: All ArgumentProxy instances are contained in MathMacro::expanded_, which means that there is always a unique parent-child like relationship between MathMacro and ArgumentProxy. Therefore, a single place exists where ArgumentProxy::mathMacro_ can become invalid, and where it could be updated: Whenever a MathMacro object is copied. Therefore we can simply create a user defined copy constructor and assignment operator for MathMacro, and update all ArgumentProxy::mathMacro_ references after copying the macro. That's it! With this simple fix we do not require anymore the additional property of RandomAccessList that resizing never copies the elemnts. Furthermore, the fix also helps in cases where the complete MathMacro::expanded_ vector is copied, which would not be fixed by converting it to a RandomAccessList only. The drawback of that approach is that it is easy to forget to update the copy constructor and assignment operator when adding new members. Therefore I'd like to use a pimpl, so that the pimpl copies can be compiler generated, and the manual operators do not need to be touched in the future. This looks like the attached lengthy patch. If I don't get complaints I'd like to put this into master, and for 2.1 I'd like to do the same, but in order to minimize the changes without the pimpl, i.e. with a manual copy constructor and assignment operator. This is now in master. Richard, for 2.1 I propose the attached solution without pimpl. I do not want to change too much code, and I don't expect any new memeber variables in 2.1 anymore. OK? Yes, that is fine. rh
Re: [ugly patch] fix bug 9418
Georg Baum wrote:
All ArgumentProxy instances are contained in MathMacro::expanded_, which
means that there is always a unique parent-child like relationship between
MathMacro and ArgumentProxy. Therefore, a single place exists where
ArgumentProxy::mathMacro_ can become invalid, and where it could be
updated: Whenever a MathMacro object is copied. Therefore we can simply
create a user defined copy constructor and assignment operator for
MathMacro, and update all ArgumentProxy::mathMacro_ references after
copying the macro. That's it! With this simple fix we do not require
anymore the additional property of RandomAccessList that resizing never
copies the elemnts. Furthermore, the fix also helps in cases where the
complete MathMacro::expanded_ vector is copied, which would not be fixed
by converting it to a RandomAccessList only.
The drawback of that approach is that it is easy to forget to update the
copy constructor and assignment operator when adding new members.
Therefore I'd like to use a pimpl, so that the pimpl copies can be
compiler generated, and the manual operators do not need to be touched in
the future. This looks like the attached lengthy patch. If I don't get
complaints I'd like to put this into master, and for 2.1 I'd like to do
the same, but in order to minimize the changes without the pimpl, i.e.
with a manual copy constructor and assignment operator.
This is now in master.
Richard, for 2.1 I propose the attached solution without pimpl. I do not
want to change too much code, and I don't expect any new memeber variables
in 2.1 anymore. OK?
Georgdiff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
index bb855b2..06cfd5c 100644
--- a/src/mathed/MathMacro.cpp
+++ b/src/mathed/MathMacro.cpp
@@ -52,35 +52,37 @@ namespace lyx {
class ArgumentProxy : public InsetMath {
public:
///
- ArgumentProxy(MathMacro mathMacro, size_t idx)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx)
: mathMacro_(mathMacro), idx_(idx) {}
///
- ArgumentProxy(MathMacro mathMacro, size_t idx, docstring const def)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx, docstring const def)
: mathMacro_(mathMacro), idx_(idx)
{
asArray(def, def_);
}
///
+ void setOwner(MathMacro * mathMacro) { mathMacro_ = mathMacro; }
+ ///
InsetCode lyxCode() const { return ARGUMENT_PROXY_CODE; }
///
void metrics(MetricsInfo mi, Dimension dim) const {
- mathMacro_.macro()-unlock();
- mathMacro_.cell(idx_).metrics(mi, dim);
+ mathMacro_-macro()-unlock();
+ mathMacro_-cell(idx_).metrics(mi, dim);
- if (!mathMacro_.editMetrics(mi.base.bv)
- mathMacro_.cell(idx_).empty())
+ if (!mathMacro_-editMetrics(mi.base.bv)
+ mathMacro_-cell(idx_).empty())
def_.metrics(mi, dim);
- mathMacro_.macro()-lock();
+ mathMacro_-macro()-lock();
}
// FIXME Other external things need similar treatment.
///
- void mathmlize(MathStream ms) const { ms mathMacro_.cell(idx_); }
+ void mathmlize(MathStream ms) const { ms mathMacro_-cell(idx_); }
///
- void htmlize(HtmlStream ms) const { ms mathMacro_.cell(idx_); }
+ void htmlize(HtmlStream ms) const { ms mathMacro_-cell(idx_); }
///
void draw(PainterInfo pi, int x, int y) const {
- if (mathMacro_.editMetrics(pi.base.bv)) {
+ if (mathMacro_-editMetrics(pi.base.bv)) {
// The only way a ArgumentProxy can appear is in a cell of the
// MathMacro. Moreover the cells are only drawn in the DISPLAY_FOLDED
// mode and then, if the macro is edited the monochrome
@@ -90,22 +92,22 @@ public:
// here (and the assert triggers in pain.leaveMonochromeMode())
// it's a bug.
pi.pain.leaveMonochromeMode();
- mathMacro_.cell(idx_).draw(pi, x, y);
+ mathMacro_-cell(idx_).draw(pi, x, y);
pi.pain.enterMonochromeMode(Color_mathbg, Color_mathmacroblend);
- } else if (mathMacro_.cell(idx_).empty()) {
- mathMacro_.cell(idx_).setXY(*pi.base.bv, x, y);
+ } else if (mathMacro_-cell(idx_).empty()) {
+ mathMacro_-cell(idx_).setXY(*pi.base.bv, x, y);
def_.draw(pi, x, y);
} else
- mathMacro_.cell(idx_).draw(pi, x, y);
+ mathMacro_-cell(idx_).draw(pi, x, y);
}
///
size_t idx() const { return idx_; }
///
int kerning(BufferView const * bv) const
{
- if (mathMacro_.editMetrics(bv)
- || !mathMacro_.cell(idx_).empty())
- return mathMacro_.cell(idx_).kerning(bv);
+ if (mathMacro_-editMetrics(bv)
+ || !mathMacro_-cell(idx_).empty())
+ return mathMacro_-cell(idx_).kerning(bv);
else
return def_.kerning(bv);
}
@@ -117,7 +119,7 @@ private:
return new ArgumentProxy(*this);
}
///
- MathMacro mathMacro_;
+ MathMacro * mathMacro_;
///
size_t idx_;
///
@@ -133,6 +135,50 @@ MathMacro::MathMacro(Buffer * buf, docstring const name)
{}
+MathMacro::MathMacro(MathMacro const that)
+ : InsetMathNest(that), expanded_(that.buffer_)
+{
+ assign(that);
+}
+
+
+MathMacro MathMacro::operator=(MathMacro const that)
+{
+ if (that == this)
+ return *this;
+
Re: [ugly patch] fix bug 9418
Georg Baum wrote:
> All ArgumentProxy instances are contained in MathMacro::expanded_, which
> means that there is always a unique parent-child like relationship between
> MathMacro and ArgumentProxy. Therefore, a single place exists where
> ArgumentProxy::mathMacro_ can become invalid, and where it could be
> updated: Whenever a MathMacro object is copied. Therefore we can simply
> create a user defined copy constructor and assignment operator for
> MathMacro, and update all ArgumentProxy::mathMacro_ references after
> copying the macro. That's it! With this simple fix we do not require
> anymore the additional property of RandomAccessList that resizing never
> copies the elemnts. Furthermore, the fix also helps in cases where the
> complete MathMacro::expanded_ vector is copied, which would not be fixed
> by converting it to a RandomAccessList only.
>
> The drawback of that approach is that it is easy to forget to update the
> copy constructor and assignment operator when adding new members.
> Therefore I'd like to use a pimpl, so that the pimpl copies can be
> compiler generated, and the manual operators do not need to be touched in
> the future. This looks like the attached lengthy patch. If I don't get
> complaints I'd like to put this into master, and for 2.1 I'd like to do
> the same, but in order to minimize the changes without the pimpl, i.e.
> with a manual copy constructor and assignment operator.
This is now in master.
Richard, for 2.1 I propose the attached solution without pimpl. I do not
want to change too much code, and I don't expect any new memeber variables
in 2.1 anymore. OK?
Georgdiff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
index bb855b2..06cfd5c 100644
--- a/src/mathed/MathMacro.cpp
+++ b/src/mathed/MathMacro.cpp
@@ -52,35 +52,37 @@ namespace lyx {
class ArgumentProxy : public InsetMath {
public:
///
- ArgumentProxy(MathMacro & mathMacro, size_t idx)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx)
: mathMacro_(mathMacro), idx_(idx) {}
///
- ArgumentProxy(MathMacro & mathMacro, size_t idx, docstring const & def)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx, docstring const & def)
: mathMacro_(mathMacro), idx_(idx)
{
asArray(def, def_);
}
///
+ void setOwner(MathMacro * mathMacro) { mathMacro_ = mathMacro; }
+ ///
InsetCode lyxCode() const { return ARGUMENT_PROXY_CODE; }
///
void metrics(MetricsInfo & mi, Dimension & dim) const {
- mathMacro_.macro()->unlock();
- mathMacro_.cell(idx_).metrics(mi, dim);
+ mathMacro_->macro()->unlock();
+ mathMacro_->cell(idx_).metrics(mi, dim);
- if (!mathMacro_.editMetrics(mi.base.bv)
- && mathMacro_.cell(idx_).empty())
+ if (!mathMacro_->editMetrics(mi.base.bv)
+ && mathMacro_->cell(idx_).empty())
def_.metrics(mi, dim);
- mathMacro_.macro()->lock();
+ mathMacro_->macro()->lock();
}
// FIXME Other external things need similar treatment.
///
- void mathmlize(MathStream & ms) const { ms << mathMacro_.cell(idx_); }
+ void mathmlize(MathStream & ms) const { ms << mathMacro_->cell(idx_); }
///
- void htmlize(HtmlStream & ms) const { ms << mathMacro_.cell(idx_); }
+ void htmlize(HtmlStream & ms) const { ms << mathMacro_->cell(idx_); }
///
void draw(PainterInfo & pi, int x, int y) const {
- if (mathMacro_.editMetrics(pi.base.bv)) {
+ if (mathMacro_->editMetrics(pi.base.bv)) {
// The only way a ArgumentProxy can appear is in a cell of the
// MathMacro. Moreover the cells are only drawn in the DISPLAY_FOLDED
// mode and then, if the macro is edited the monochrome
@@ -90,22 +92,22 @@ public:
// here (and the assert triggers in pain.leaveMonochromeMode())
// it's a bug.
pi.pain.leaveMonochromeMode();
- mathMacro_.cell(idx_).draw(pi, x, y);
+ mathMacro_->cell(idx_).draw(pi, x, y);
pi.pain.enterMonochromeMode(Color_mathbg, Color_mathmacroblend);
- } else if (mathMacro_.cell(idx_).empty()) {
- mathMacro_.cell(idx_).setXY(*pi.base.bv, x, y);
+ } else if (mathMacro_->cell(idx_).empty()) {
+ mathMacro_->cell(idx_).setXY(*pi.base.bv, x, y);
def_.draw(pi, x, y);
} else
- mathMacro_.cell(idx_).draw(pi, x, y);
+ mathMacro_->cell(idx_).draw(pi, x, y);
}
///
size_t idx() const { return idx_; }
///
int kerning(BufferView const * bv) const
{
- if (mathMacro_.editMetrics(bv)
- || !mathMacro_.cell(idx_).empty())
- return mathMacro_.cell(idx_).kerning(bv);
+ if (mathMacro_->editMetrics(bv)
+ || !mathMacro_->cell(idx_).empty())
+ return mathMacro_->cell(idx_).kerning(bv);
else
return def_.kerning(bv);
}
@@ -117,7 +119,7 @@ private:
return new ArgumentProxy(*this);
}
///
- MathMacro & mathMacro_;
+ MathMacro * mathMacro_;
///
size_t idx_;
///
@@ -133,6 +135,50 @@ MathMacro::MathMacro(Buffer * buf, docstring const & name)
{}
+MathMacro::MathMacro(MathMacro const & that)
+ : InsetMathNest(that), expanded_(that.buffer_)
+{
+ assign(that);
+}
+
+
+MathMacro &
Re: [ugly patch] fix bug 9418
On 04/05/2015 11:01 AM, Georg Baum wrote: Georg Baum wrote: All ArgumentProxy instances are contained in MathMacro::expanded_, which means that there is always a unique parent-child like relationship between MathMacro and ArgumentProxy. Therefore, a single place exists where ArgumentProxy::mathMacro_ can become invalid, and where it could be updated: Whenever a MathMacro object is copied. Therefore we can simply create a user defined copy constructor and assignment operator for MathMacro, and update all ArgumentProxy::mathMacro_ references after copying the macro. That's it! With this simple fix we do not require anymore the additional property of RandomAccessList that resizing never copies the elemnts. Furthermore, the fix also helps in cases where the complete MathMacro::expanded_ vector is copied, which would not be fixed by converting it to a RandomAccessList only. The drawback of that approach is that it is easy to forget to update the copy constructor and assignment operator when adding new members. Therefore I'd like to use a pimpl, so that the pimpl copies can be compiler generated, and the manual operators do not need to be touched in the future. This looks like the attached lengthy patch. If I don't get complaints I'd like to put this into master, and for 2.1 I'd like to do the same, but in order to minimize the changes without the pimpl, i.e. with a manual copy constructor and assignment operator. This is now in master. Richard, for 2.1 I propose the attached solution without pimpl. I do not want to change too much code, and I don't expect any new memeber variables in 2.1 anymore. OK? Yes, that is fine. rh
Re: [ugly patch] fix bug 9418
Le 31/03/2015 21:38, Georg Baum a écrit : BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. I know that this needs work, but I don't see an obvious miracle:-( Currently we also have the strange mixture of lazy and non-lazy macros. What do you mean by lazy/non-lazy? JMarc
Re: [ugly patch] fix bug 9418
Jean-Marc Lasgouttes wrote: Le 31/03/2015 21:38, Georg Baum a écrit : BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. I know that this needs work, but I don't see an obvious miracle:-( Currently we also have the strange mixture of lazy and non-lazy macros. What do you mean by lazy/non-lazy? See src/mathed/MacroTable.h. There is one constructor of MacroData documented as lazy, and one documented as non-lazy. Georg
Re: [ugly patch] fix bug 9418
Le 31/03/2015 21:38, Georg Baum a écrit : BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. I know that this needs work, but I don't see an obvious miracle:-( Currently we also have the strange mixture of lazy and non-lazy macros. What do you mean by lazy/non-lazy? JMarc
Re: [ugly patch] fix bug 9418
Jean-Marc Lasgouttes wrote: > Le 31/03/2015 21:38, Georg Baum a écrit : >>> BTW updateMacros is quite heavy on big documents (#5973) and I have >>> medium-term plans for making the update lazy. This may become >>> unnecessary if you can do another kind of miracle. My plan is to use >>> recordUndo to increment a version_id for the buffer. Therefore we can >>> know when macros really need to be updated. >> >> I know that this needs work, but I don't see an obvious miracle:-( >> Currently we also have the strange mixture of lazy and non-lazy macros. > > What do you mean by lazy/non-lazy? See src/mathed/MacroTable.h. There is one constructor of MacroData documented as lazy, and one documented as non-lazy. Georg
Re: [ugly patch] fix bug 9418
Jean-Marc Lasgouttes wrote: Could you tell us a little bit about ArgumentProxy and what is is good for? It is used for macros with arguments. Each argument of a math macro is one cell of the macro. An ArgumentProxy is the expanded representation of such a cell. It has basically no own data (except for a possible default argument), but forwards everything interesting to the macro itself. The only reason why it is needed is reusage of some MathData member functions: draw(), metrics, dimension() etc. BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. I know that this needs work, but I don't see an obvious miracle:-( Currently we also have the strange mixture of lazy and non-lazy macros. Georg
Re: [ugly patch] fix bug 9418
Jean-Marc Lasgouttes wrote: > Could you tell us a little bit about ArgumentProxy and what is is good > for? It is used for macros with arguments. Each argument of a math macro is one cell of the macro. An ArgumentProxy is the expanded representation of such a cell. It has basically no own data (except for a possible default argument), but forwards everything interesting to the macro itself. The only reason why it is needed is reusage of some MathData member functions: draw(), metrics, dimension() etc. > BTW updateMacros is quite heavy on big documents (#5973) and I have > medium-term plans for making the update lazy. This may become > unnecessary if you can do another kind of miracle. My plan is to use > recordUndo to increment a version_id for the buffer. Therefore we can > know when macros really need to be updated. I know that this needs work, but I don't see an obvious miracle:-( Currently we also have the strange mixture of lazy and non-lazy macros. Georg
Re: [ugly patch] fix bug 9418
Richard Heck wrote:
On 03/22/2015 09:14 AM, Georg Baum wrote:
Jean-Marc Lasgouttes wrote:
Le 20/03/15 21:53, Georg Baum a écrit :
The real cause for the bug is that ArgumentProxy::mathMacro_ is a
reference to an object which is stored in a MathData, which is a
std::vector storing MathAtoms by value (not pointers). Therefore, each
time when the MathData object which contains a math macro instance is
resized, the ArgumentProxy::mathMacro_ members of its arguments may
become invalid. In case of bug 9418 the resizing happens because only
macro \a is copied to the clipboard, and macro \b is not, therefore \b
is converted to an unknown inset and its argument is put as a separate
inset after \b.
What about using a std::list or our own RandomAccessList instead of
std::vector?
Good idea. The random access is needed, so std::list does not work. Using
RandomAccessList seems to work, but MathData is a very central part of
mathed, so this would need very intensive testing. Also, some of the
loops should be rewritten using iterators to avoid recomputation of the
current iterator. General performance testing (memory and speed) would be
needed as well.
In total I am not sure whether we should go that way. I would rather try
to rework ArgumnentProxy (or get rid of it) to better match the general
mathed structure: Each inset is self contained and does not need to know
anything about other insets.
I'd join this conversation, but this is beyond my competence to judge.
If there's a worry about testing, I think we have some time before 2.2.
We can always back it out if need be and do something simpler.
Actually my main concern is not testing anymore, but whether
RandomAccessList is really the best solution.
Meanwhile I do have a better understanding of the math macros and found
another solution which I think is much better. First, it is really very
difficult to get rid of ArgumentProxy::mathMacro_. So, I stopped trying to
get rid of it but rather tried to find out how to keep it up to date in an
idiot proof way (because everything else will break in the future, BTDT),
and I believe I found one:
All ArgumentProxy instances are contained in MathMacro::expanded_, which
means that there is always a unique parent-child like relationship between
MathMacro and ArgumentProxy. Therefore, a single place exists where
ArgumentProxy::mathMacro_ can become invalid, and where it could be updated:
Whenever a MathMacro object is copied. Therefore we can simply create a user
defined copy constructor and assignment operator for MathMacro, and update
all ArgumentProxy::mathMacro_ references after copying the macro. That's it!
With this simple fix we do not require anymore the additional property of
RandomAccessList that resizing never copies the elemnts. Furthermore, the
fix also helps in cases where the complete MathMacro::expanded_ vector is
copied, which would not be fixed by converting it to a RandomAccessList
only.
The drawback of that approach is that it is easy to forget to update the
copy constructor and assignment operator when adding new members. Therefore
I'd like to use a pimpl, so that the pimpl copies can be compiler generated,
and the manual operators do not need to be touched in the future. This looks
like the attached lengthy patch. If I don't get complaints I'd like to put
this into master, and for 2.1 I'd like to do the same, but in order to
minimize the changes without the pimpl, i.e. with a manual copy constructor
and assignment operator.
Georgdiff --git a/src/mathed/MathData.cpp b/src/mathed/MathData.cpp
index 2d5bcd0..6daf096 100644
--- a/src/mathed/MathData.cpp
+++ b/src/mathed/MathData.cpp
@@ -405,9 +405,9 @@ void MathData::updateMacros(Cursor * cur, MacroContext const mc,
// go over the array and look for macros
for (size_t i = 0; i size(); ++i) {
MathMacro * macroInset = operator[](i).nucleus()-asMacro();
- if (!macroInset || macroInset-name_.empty()
-|| macroInset-name_[0] == '^'
-|| macroInset-name_[0] == '_'
+ if (!macroInset || macroInset-macroName().empty()
+|| macroInset-macroName()[0] == '^'
+|| macroInset-macroName()[0] == '_'
|| (macroInset-name() == edited_name
macroInset-displayMode() ==
MathMacro::DISPLAY_UNFOLDED))
diff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
index 4cdecbd..22f5d80 100644
--- a/src/mathed/MathMacro.cpp
+++ b/src/mathed/MathMacro.cpp
@@ -52,44 +52,46 @@ namespace lyx {
class ArgumentProxy : public InsetMath {
public:
///
- ArgumentProxy(MathMacro mathMacro, size_t idx)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx)
: mathMacro_(mathMacro), idx_(idx) {}
///
- ArgumentProxy(MathMacro mathMacro, size_t idx, docstring const def)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx, docstring const def)
: mathMacro_(mathMacro), idx_(idx)
{
asArray(def, def_);
}
///
+ void setOwner(MathMacro * mathMacro) { mathMacro_ = mathMacro; }
+ ///
Re: [ugly patch] fix bug 9418
Le 30/03/2015 22:42, Georg Baum a écrit : Meanwhile I do have a better understanding of the math macros and found another solution which I think is much better. First, it is really very difficult to get rid of ArgumentProxy::mathMacro_. So, I stopped trying to get rid of it but rather tried to find out how to keep it up to date in an idiot proof way (because everything else will break in the future, BTDT), and I believe I found one: All ArgumentProxy instances are contained in MathMacro::expanded_, which means that there is always a unique parent-child like relationship between MathMacro and ArgumentProxy. [snip very interesting description] Could you tell us a little bit about ArgumentProxy and what is is good for? BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. JMarc
Re: [ugly patch] fix bug 9418
Richard Heck wrote:
> On 03/22/2015 09:14 AM, Georg Baum wrote:
>> Jean-Marc Lasgouttes wrote:
>>
>>> Le 20/03/15 21:53, Georg Baum a écrit :
The real cause for the bug is that ArgumentProxy::mathMacro_ is a
reference to an object which is stored in a MathData, which is a
std::vector storing MathAtoms by value (not pointers). Therefore, each
time when the MathData object which contains a math macro instance is
resized, the ArgumentProxy::mathMacro_ members of its arguments may
become invalid. In case of bug 9418 the resizing happens because only
macro \a is copied to the clipboard, and macro \b is not, therefore \b
is converted to an unknown inset and its argument is put as a separate
inset after \b.
>>> What about using a std::list or our own RandomAccessList instead of
>>> std::vector?
>> Good idea. The random access is needed, so std::list does not work. Using
>> RandomAccessList seems to work, but MathData is a very central part of
>> mathed, so this would need very intensive testing. Also, some of the
>> loops should be rewritten using iterators to avoid recomputation of the
>> current iterator. General performance testing (memory and speed) would be
>> needed as well.
>>
>> In total I am not sure whether we should go that way. I would rather try
>> to rework ArgumnentProxy (or get rid of it) to better match the general
>> mathed structure: Each inset is self contained and does not need to know
>> anything about other insets.
>
> I'd join this conversation, but this is beyond my competence to judge.
>
> If there's a worry about testing, I think we have some time before 2.2.
> We can always back it out if need be and do something simpler.
Actually my main concern is not testing anymore, but whether
RandomAccessList is really the best solution.
Meanwhile I do have a better understanding of the math macros and found
another solution which I think is much better. First, it is really very
difficult to get rid of ArgumentProxy::mathMacro_. So, I stopped trying to
get rid of it but rather tried to find out how to keep it up to date in an
idiot proof way (because everything else will break in the future, BTDT),
and I believe I found one:
All ArgumentProxy instances are contained in MathMacro::expanded_, which
means that there is always a unique parent-child like relationship between
MathMacro and ArgumentProxy. Therefore, a single place exists where
ArgumentProxy::mathMacro_ can become invalid, and where it could be updated:
Whenever a MathMacro object is copied. Therefore we can simply create a user
defined copy constructor and assignment operator for MathMacro, and update
all ArgumentProxy::mathMacro_ references after copying the macro. That's it!
With this simple fix we do not require anymore the additional property of
RandomAccessList that resizing never copies the elemnts. Furthermore, the
fix also helps in cases where the complete MathMacro::expanded_ vector is
copied, which would not be fixed by converting it to a RandomAccessList
only.
The drawback of that approach is that it is easy to forget to update the
copy constructor and assignment operator when adding new members. Therefore
I'd like to use a pimpl, so that the pimpl copies can be compiler generated,
and the manual operators do not need to be touched in the future. This looks
like the attached lengthy patch. If I don't get complaints I'd like to put
this into master, and for 2.1 I'd like to do the same, but in order to
minimize the changes without the pimpl, i.e. with a manual copy constructor
and assignment operator.
Georgdiff --git a/src/mathed/MathData.cpp b/src/mathed/MathData.cpp
index 2d5bcd0..6daf096 100644
--- a/src/mathed/MathData.cpp
+++ b/src/mathed/MathData.cpp
@@ -405,9 +405,9 @@ void MathData::updateMacros(Cursor * cur, MacroContext const & mc,
// go over the array and look for macros
for (size_t i = 0; i < size(); ++i) {
MathMacro * macroInset = operator[](i).nucleus()->asMacro();
- if (!macroInset || macroInset->name_.empty()
-|| macroInset->name_[0] == '^'
-|| macroInset->name_[0] == '_'
+ if (!macroInset || macroInset->macroName().empty()
+|| macroInset->macroName()[0] == '^'
+|| macroInset->macroName()[0] == '_'
|| (macroInset->name() == edited_name
&& macroInset->displayMode() ==
MathMacro::DISPLAY_UNFOLDED))
diff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
index 4cdecbd..22f5d80 100644
--- a/src/mathed/MathMacro.cpp
+++ b/src/mathed/MathMacro.cpp
@@ -52,44 +52,46 @@ namespace lyx {
class ArgumentProxy : public InsetMath {
public:
///
- ArgumentProxy(MathMacro & mathMacro, size_t idx)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx)
: mathMacro_(mathMacro), idx_(idx) {}
///
- ArgumentProxy(MathMacro & mathMacro, size_t idx, docstring const & def)
+ ArgumentProxy(MathMacro * mathMacro, size_t idx, docstring const & def)
: mathMacro_(mathMacro), idx_(idx)
{
Re: [ugly patch] fix bug 9418
Le 30/03/2015 22:42, Georg Baum a écrit : Meanwhile I do have a better understanding of the math macros and found another solution which I think is much better. First, it is really very difficult to get rid of ArgumentProxy::mathMacro_. So, I stopped trying to get rid of it but rather tried to find out how to keep it up to date in an idiot proof way (because everything else will break in the future, BTDT), and I believe I found one: All ArgumentProxy instances are contained in MathMacro::expanded_, which means that there is always a unique parent-child like relationship between MathMacro and ArgumentProxy. [snip very interesting description] Could you tell us a little bit about ArgumentProxy and what is is good for? BTW updateMacros is quite heavy on big documents (#5973) and I have medium-term plans for making the update lazy. This may become unnecessary if you can do another kind of miracle. My plan is to use recordUndo to increment a version_id for the buffer. Therefore we can know when macros really need to be updated. JMarc
Re: [ugly patch] fix bug 9418
Jean-Marc Lasgouttes wrote:
Le 20/03/15 21:53, Georg Baum a écrit :
The real cause for the bug is that ArgumentProxy::mathMacro_ is a
reference to an object which is stored in a MathData, which is a
std::vector storing MathAtoms by value (not pointers). Therefore, each
time when the MathData object which contains a math macro instance is
resized, the ArgumentProxy::mathMacro_ members of its arguments may
become invalid. In case of bug 9418 the resizing happens because only
macro \a is copied to the clipboard, and macro \b is not, therefore \b is
converted to an unknown inset and its argument is put as a separate inset
after \b.
What about using a std::list or our own RandomAccessList instead of
std::vector?
Good idea. The random access is needed, so std::list does not work. Using
RandomAccessList seems to work, but MathData is a very central part of
mathed, so this would need very intensive testing. Also, some of the loops
should be rewritten using iterators to avoid recomputation of the current
iterator. General performance testing (memory and speed) would be needed as
well.
In total I am not sure whether we should go that way. I would rather try to
rework ArgumnentProxy (or get rid of it) to better match the general mathed
structure: Each inset is self contained and does not need to know anything
about other insets.
Georgdiff --git a/src/Cursor.cpp b/src/Cursor.cpp
index 898eab3..9ff7fc2 100644
--- a/src/Cursor.cpp
+++ b/src/Cursor.cpp
@@ -2130,8 +2130,10 @@ void Cursor::handleFont(string const font)
// cursor in between. split cell
MathData::iterator bt = cell().begin();
MathAtom at = createInsetMath(from_utf8(font), buffer());
- at.nucleus()-cell(0) = MathData(buffer(), bt, bt + pos());
- cell().erase(bt, bt + pos());
+ MathData::iterator it = bt;
+ advance(it, pos());
+ at.nucleus()-cell(0) = MathData(buffer(), bt, it);
+ cell().erase(bt, it);
popBackward();
plainInsert(at);
}
diff --git a/src/CutAndPaste.cpp b/src/CutAndPaste.cpp
index 6333892..6536d54 100644
--- a/src/CutAndPaste.cpp
+++ b/src/CutAndPaste.cpp
@@ -1363,7 +1363,11 @@ docstring grabSelection(Cursor const cur)
if (i1.inset().asInsetMath()) {
MathData::const_iterator it = i1.cell().begin();
Buffer * buf = cur.buffer();
- return asString(MathData(buf, it + i1.pos(), it + i2.pos()));
+ MathData::const_iterator it1 = it;
+ advance(it1, i1.pos());
+ MathData::const_iterator it2 = it;
+ advance(it2, i2.pos());
+ return asString(MathData(buf, it1, it2));
} else {
return from_ascii(unknown selection 1);
}
diff --git a/src/DocIterator.cpp b/src/DocIterator.cpp
index 750479f..a821455 100644
--- a/src/DocIterator.cpp
+++ b/src/DocIterator.cpp
@@ -317,9 +317,11 @@ void DocIterator::forwardPos()
if (tip.pos() != tip.lastpos()) {
// move into an inset to the right if possible
Inset * n = 0;
- if (inMathed())
- n = (tip.cell().begin() + tip.pos())-nucleus();
- else
+ if (inMathed()) {
+ MathData::iterator it = tip.cell().begin();
+ advance(it, tip.pos());
+ n = it-nucleus();
+ } else
n = paragraph().getInset(tip.pos());
if (n n-isActive()) {
//lyxerr ... descend endl;
@@ -436,9 +438,11 @@ void DocIterator::backwardPos()
// move into an inset to the left if possible
Inset * n = 0;
- if (inMathed())
- n = (top().cell().begin() + top().pos())-nucleus();
- else
+ if (inMathed()) {
+ MathData::iterator it = top().cell().begin();
+ advance(it, top().pos());
+ n = it-nucleus();
+ } else
n = paragraph().getInset(top().pos());
if (n n-isActive()) {
push_back(CursorSlice(*n));
@@ -535,9 +539,11 @@ bool DocIterator::fixIfBroken()
break;
} else if (i != n - 1 cs.pos() != cs.lastpos()) {
// get inset which is supposed to be in the next slice
- if (cs.inset().inMathed())
-inset = (cs.cell().begin() + cs.pos())-nucleus();
- else if (Inset * csInset = cs.paragraph().getInset(cs.pos()))
+ if (cs.inset().inMathed()) {
+MathData::iterator it = cs.cell().begin();
+advance(it, cs.pos());
+inset = it-nucleus();
+ } else if (Inset * csInset = cs.paragraph().getInset(cs.pos()))
inset = csInset;
else {
// there are slices left, so there must be another inset
diff --git a/src/lyxfind.cpp b/src/lyxfind.cpp
index f3b3eef..5db671e 100644
--- a/src/lyxfind.cpp
+++ b/src/lyxfind.cpp
@@ -1025,12 +1025,13 @@ docstring stringifyFromCursor(DocIterator const cur, int len)
docstring s;
CursorSlice cs = cur.top();
MathData md = cs.cell();
- MathData::const_iterator it_end =
- (( len == -1 || cs.pos() + len int(md.size()))
- ? md.end()
- : md.begin() + cs.pos() + len );
- for (MathData::const_iterator it = md.begin() + cs.pos();
- it != it_end; ++it)
+ MathData::const_iterator it_end = md.end();
+ if (!( len == -1 || cs.pos() + len int(md.size( {
+ it_end = md.begin();
+ advance(it_end, cs.pos() + len);
+ }
+ MathData::const_iterator it = md.begin();
+
Re: [ugly patch] fix bug 9418
On 03/22/2015 09:14 AM, Georg Baum wrote: Jean-Marc Lasgouttes wrote: Le 20/03/15 21:53, Georg Baum a écrit : The real cause for the bug is that ArgumentProxy::mathMacro_ is a reference to an object which is stored in a MathData, which is a std::vector storing MathAtoms by value (not pointers). Therefore, each time when the MathData object which contains a math macro instance is resized, the ArgumentProxy::mathMacro_ members of its arguments may become invalid. In case of bug 9418 the resizing happens because only macro \a is copied to the clipboard, and macro \b is not, therefore \b is converted to an unknown inset and its argument is put as a separate inset after \b. What about using a std::list or our own RandomAccessList instead of std::vector? Good idea. The random access is needed, so std::list does not work. Using RandomAccessList seems to work, but MathData is a very central part of mathed, so this would need very intensive testing. Also, some of the loops should be rewritten using iterators to avoid recomputation of the current iterator. General performance testing (memory and speed) would be needed as well. In total I am not sure whether we should go that way. I would rather try to rework ArgumnentProxy (or get rid of it) to better match the general mathed structure: Each inset is self contained and does not need to know anything about other insets. I'd join this conversation, but this is beyond my competence to judge. If there's a worry about testing, I think we have some time before 2.2. We can always back it out if need be and do something simpler. Richard
Re: [ugly patch] fix bug 9418
Jean-Marc Lasgouttes wrote:
> Le 20/03/15 21:53, Georg Baum a écrit :
>> The real cause for the bug is that ArgumentProxy::mathMacro_ is a
>> reference to an object which is stored in a MathData, which is a
>> std::vector storing MathAtoms by value (not pointers). Therefore, each
>> time when the MathData object which contains a math macro instance is
>> resized, the ArgumentProxy::mathMacro_ members of its arguments may
>> become invalid. In case of bug 9418 the resizing happens because only
>> macro \a is copied to the clipboard, and macro \b is not, therefore \b is
>> converted to an unknown inset and its argument is put as a separate inset
>> after \b.
>
> What about using a std::list or our own RandomAccessList instead of
> std::vector?
Good idea. The random access is needed, so std::list does not work. Using
RandomAccessList seems to work, but MathData is a very central part of
mathed, so this would need very intensive testing. Also, some of the loops
should be rewritten using iterators to avoid recomputation of the current
iterator. General performance testing (memory and speed) would be needed as
well.
In total I am not sure whether we should go that way. I would rather try to
rework ArgumnentProxy (or get rid of it) to better match the general mathed
structure: Each inset is self contained and does not need to know anything
about other insets.
Georgdiff --git a/src/Cursor.cpp b/src/Cursor.cpp
index 898eab3..9ff7fc2 100644
--- a/src/Cursor.cpp
+++ b/src/Cursor.cpp
@@ -2130,8 +2130,10 @@ void Cursor::handleFont(string const & font)
// cursor in between. split cell
MathData::iterator bt = cell().begin();
MathAtom at = createInsetMath(from_utf8(font), buffer());
- at.nucleus()->cell(0) = MathData(buffer(), bt, bt + pos());
- cell().erase(bt, bt + pos());
+ MathData::iterator it = bt;
+ advance(it, pos());
+ at.nucleus()->cell(0) = MathData(buffer(), bt, it);
+ cell().erase(bt, it);
popBackward();
plainInsert(at);
}
diff --git a/src/CutAndPaste.cpp b/src/CutAndPaste.cpp
index 6333892..6536d54 100644
--- a/src/CutAndPaste.cpp
+++ b/src/CutAndPaste.cpp
@@ -1363,7 +1363,11 @@ docstring grabSelection(Cursor const & cur)
if (i1.inset().asInsetMath()) {
MathData::const_iterator it = i1.cell().begin();
Buffer * buf = cur.buffer();
- return asString(MathData(buf, it + i1.pos(), it + i2.pos()));
+ MathData::const_iterator it1 = it;
+ advance(it1, i1.pos());
+ MathData::const_iterator it2 = it;
+ advance(it2, i2.pos());
+ return asString(MathData(buf, it1, it2));
} else {
return from_ascii("unknown selection 1");
}
diff --git a/src/DocIterator.cpp b/src/DocIterator.cpp
index 750479f..a821455 100644
--- a/src/DocIterator.cpp
+++ b/src/DocIterator.cpp
@@ -317,9 +317,11 @@ void DocIterator::forwardPos()
if (tip.pos() != tip.lastpos()) {
// move into an inset to the right if possible
Inset * n = 0;
- if (inMathed())
- n = (tip.cell().begin() + tip.pos())->nucleus();
- else
+ if (inMathed()) {
+ MathData::iterator it = tip.cell().begin();
+ advance(it, tip.pos());
+ n = it->nucleus();
+ } else
n = paragraph().getInset(tip.pos());
if (n && n->isActive()) {
//lyxerr << "... descend" << endl;
@@ -436,9 +438,11 @@ void DocIterator::backwardPos()
// move into an inset to the left if possible
Inset * n = 0;
- if (inMathed())
- n = (top().cell().begin() + top().pos())->nucleus();
- else
+ if (inMathed()) {
+ MathData::iterator it = top().cell().begin();
+ advance(it, top().pos());
+ n = it->nucleus();
+ } else
n = paragraph().getInset(top().pos());
if (n && n->isActive()) {
push_back(CursorSlice(*n));
@@ -535,9 +539,11 @@ bool DocIterator::fixIfBroken()
break;
} else if (i != n - 1 && cs.pos() != cs.lastpos()) {
// get inset which is supposed to be in the next slice
- if (cs.inset().inMathed())
-inset = (cs.cell().begin() + cs.pos())->nucleus();
- else if (Inset * csInset = cs.paragraph().getInset(cs.pos()))
+ if (cs.inset().inMathed()) {
+MathData::iterator it = cs.cell().begin();
+advance(it, cs.pos());
+inset = it->nucleus();
+ } else if (Inset * csInset = cs.paragraph().getInset(cs.pos()))
inset = csInset;
else {
// there are slices left, so there must be another inset
diff --git a/src/lyxfind.cpp b/src/lyxfind.cpp
index f3b3eef..5db671e 100644
--- a/src/lyxfind.cpp
+++ b/src/lyxfind.cpp
@@ -1025,12 +1025,13 @@ docstring stringifyFromCursor(DocIterator const & cur, int len)
docstring s;
CursorSlice cs = cur.top();
MathData md = cs.cell();
- MathData::const_iterator it_end =
- (( len == -1 || cs.pos() + len > int(md.size()))
- ? md.end()
- : md.begin() + cs.pos() + len );
- for (MathData::const_iterator it = md.begin() + cs.pos();
- it != it_end; ++it)
+ MathData::const_iterator it_end = md.end();
+ if (!( len == -1 || cs.pos() + len > int(md.size( {
+ it_end = md.begin();
+ advance(it_end, cs.pos() + len);
+
Re: [ugly patch] fix bug 9418
On 03/22/2015 09:14 AM, Georg Baum wrote: Jean-Marc Lasgouttes wrote: Le 20/03/15 21:53, Georg Baum a écrit : The real cause for the bug is that ArgumentProxy::mathMacro_ is a reference to an object which is stored in a MathData, which is a std::vector storing MathAtoms by value (not pointers). Therefore, each time when the MathData object which contains a math macro instance is resized, the ArgumentProxy::mathMacro_ members of its arguments may become invalid. In case of bug 9418 the resizing happens because only macro \a is copied to the clipboard, and macro \b is not, therefore \b is converted to an unknown inset and its argument is put as a separate inset after \b. What about using a std::list or our own RandomAccessList instead of std::vector? Good idea. The random access is needed, so std::list does not work. Using RandomAccessList seems to work, but MathData is a very central part of mathed, so this would need very intensive testing. Also, some of the loops should be rewritten using iterators to avoid recomputation of the current iterator. General performance testing (memory and speed) would be needed as well. In total I am not sure whether we should go that way. I would rather try to rework ArgumnentProxy (or get rid of it) to better match the general mathed structure: Each inset is self contained and does not need to know anything about other insets. I'd join this conversation, but this is beyond my competence to judge. If there's a worry about testing, I think we have some time before 2.2. We can always back it out if need be and do something simpler. Richard
Re: [ugly patch] fix bug 9418
Le 20/03/15 21:53, Georg Baum a écrit : The real cause for the bug is that ArgumentProxy::mathMacro_ is a reference to an object which is stored in a MathData, which is a std::vector storing MathAtoms by value (not pointers). Therefore, each time when the MathData object which contains a math macro instance is resized, the ArgumentProxy::mathMacro_ members of its arguments may become invalid. In case of bug 9418 the resizing happens because only macro \a is copied to the clipboard, and macro \b is not, therefore \b is converted to an unknown inset and its argument is put as a separate inset after \b. What about using a std::list or our own RandomAccessList instead of std::vector? JMarc
Re: [ugly patch] fix bug 9418
Le 20/03/15 21:53, Georg Baum a écrit : The real cause for the bug is that ArgumentProxy::mathMacro_ is a reference to an object which is stored in a MathData, which is a std::vector storing MathAtoms by value (not pointers). Therefore, each time when the MathData object which contains a math macro instance is resized, the ArgumentProxy::mathMacro_ members of its arguments may become invalid. In case of bug 9418 the resizing happens because only macro \a is copied to the clipboard, and macro \b is not, therefore \b is converted to an unknown inset and its argument is put as a separate inset after \b. What about using a std::list or our own RandomAccessList instead of std::vector? JMarc
[ugly patch] fix bug 9418
Georg Baum wrote:
This problem would be easily avoided if one would iterate backwards
through the macro insets (a macro can never reference another one which
follows later), but unfortunately I failed to implement that, since I do
not understand the DocIterator stuff well enough. I tried several
versions, both using nextInset() and prevInset() in
DocIterator::backwardInset() and Buffer::updateMacroInstances(), but
nothing worked. The attached patcdh shows one failed attempt. Does anybody
have a clue how to implement a backward inset iteration?
After further investigation it turned out that backward iteration did not
fix the problem. A nice side effect of that investigation is that we now
have a working DocIterator::backwardInset().
The real cause for the bug is that ArgumentProxy::mathMacro_ is a reference
to an object which is stored in a MathData, which is a std::vector storing
MathAtoms by value (not pointers). Therefore, each time when the MathData
object which contains a math macro instance is resized, the
ArgumentProxy::mathMacro_ members of its arguments may become invalid. In
case of bug 9418 the resizing happens because only macro \a is copied to the
clipboard, and macro \b is not, therefore \b is converted to an unknown
inset and its argument is put as a separate inset after \b.
ArgumentProxy::mathMacro_ is broken by design IMHO, but fixing this properly
will involve rewriting big parts of the math macros (which would probably be
a good thing, but I don't have the time for it). Therefore I propose the
attached ugly patch, which ensures for the case that a MathData object is
resized by MathData::updateMacros() that the references are valid. This
fixes the crash in bug 9418, it may also fix other crashes, but similar
crashes might still exist if a MathData object is resized without an
immediately following updateMacros().
Does anybody have a better idea?
Georgdiff --git a/src/mathed/MathData.cpp b/src/mathed/MathData.cpp
index 1b79f0c..48e4ffa 100644
--- a/src/mathed/MathData.cpp
+++ b/src/mathed/MathData.cpp
@@ -394,7 +394,7 @@ void MathData::updateBuffer(ParIterator const it, UpdateType utype)
void MathData::updateMacros(Cursor * cur, MacroContext const mc,
- UpdateType utype)
+ UpdateType const utype)
{
// If we are editing a macro, we cannot update it immediately,
// otherwise wrong undo steps will be recorded (bug 6208).
@@ -403,6 +403,8 @@ void MathData::updateMacros(Cursor * cur, MacroContext const mc,
docstring const edited_name = inmacro ? inmacro-name() : docstring();
// go over the array and look for macros
+ size_t const oldsize = size();
+ vectorsize_t macroindices;
for (size_t i = 0; i size(); ++i) {
MathMacro * macroInset = operator[](i).nucleus()-asMacro();
if (!macroInset || macroInset-name_.empty()
@@ -492,6 +494,24 @@ void MathData::updateMacros(Cursor * cur, MacroContext const mc,
inset = inset-asScriptInset()-nuc()[0].nucleus();
LASSERT(inset-asMacro(), continue);
inset-asMacro()-updateRepresentation(cur, mc, utype);
+ macroindices.push_back(i);
+ }
+
+ if (size() == oldsize)
+ return;
+
+ // If our size has changed because macro parameters have been detached
+ // or attached, we need to update the representation of all macros
+ // again, since the references stored in ArgumentProxy may now be
+ // invalid (bug 9418). This will not do any visible change except
+ // creating ArgumentProxy instances with valid references.
+ for (size_t i = 0; i macroindices.size(); ++i) {
+ InsetMath * inset = operator[](macroindices[i]).nucleus();
+ if (inset-asScriptInset())
+ inset = inset-asScriptInset()-nuc()[0].nucleus();
+ MathMacro * macroInset = inset-asMacro();
+ LASSERT(macroInset, continue);
+ macroInset-updateRepresentation(cur, mc, utype);
}
}
[ugly patch] fix bug 9418
Georg Baum wrote:
> This problem would be easily avoided if one would iterate backwards
> through the macro insets (a macro can never reference another one which
> follows later), but unfortunately I failed to implement that, since I do
> not understand the DocIterator stuff well enough. I tried several
> versions, both using nextInset() and prevInset() in
> DocIterator::backwardInset() and Buffer::updateMacroInstances(), but
> nothing worked. The attached patcdh shows one failed attempt. Does anybody
> have a clue how to implement a backward inset iteration?
After further investigation it turned out that backward iteration did not
fix the problem. A nice side effect of that investigation is that we now
have a working DocIterator::backwardInset().
The real cause for the bug is that ArgumentProxy::mathMacro_ is a reference
to an object which is stored in a MathData, which is a std::vector storing
MathAtoms by value (not pointers). Therefore, each time when the MathData
object which contains a math macro instance is resized, the
ArgumentProxy::mathMacro_ members of its arguments may become invalid. In
case of bug 9418 the resizing happens because only macro \a is copied to the
clipboard, and macro \b is not, therefore \b is converted to an unknown
inset and its argument is put as a separate inset after \b.
ArgumentProxy::mathMacro_ is broken by design IMHO, but fixing this properly
will involve rewriting big parts of the math macros (which would probably be
a good thing, but I don't have the time for it). Therefore I propose the
attached ugly patch, which ensures for the case that a MathData object is
resized by MathData::updateMacros() that the references are valid. This
fixes the crash in bug 9418, it may also fix other crashes, but similar
crashes might still exist if a MathData object is resized without an
immediately following updateMacros().
Does anybody have a better idea?
Georgdiff --git a/src/mathed/MathData.cpp b/src/mathed/MathData.cpp
index 1b79f0c..48e4ffa 100644
--- a/src/mathed/MathData.cpp
+++ b/src/mathed/MathData.cpp
@@ -394,7 +394,7 @@ void MathData::updateBuffer(ParIterator const & it, UpdateType utype)
void MathData::updateMacros(Cursor * cur, MacroContext const & mc,
- UpdateType utype)
+ UpdateType const utype)
{
// If we are editing a macro, we cannot update it immediately,
// otherwise wrong undo steps will be recorded (bug 6208).
@@ -403,6 +403,8 @@ void MathData::updateMacros(Cursor * cur, MacroContext const & mc,
docstring const edited_name = inmacro ? inmacro->name() : docstring();
// go over the array and look for macros
+ size_t const oldsize = size();
+ vector macroindices;
for (size_t i = 0; i < size(); ++i) {
MathMacro * macroInset = operator[](i).nucleus()->asMacro();
if (!macroInset || macroInset->name_.empty()
@@ -492,6 +494,24 @@ void MathData::updateMacros(Cursor * cur, MacroContext const & mc,
inset = inset->asScriptInset()->nuc()[0].nucleus();
LASSERT(inset->asMacro(), continue);
inset->asMacro()->updateRepresentation(cur, mc, utype);
+ macroindices.push_back(i);
+ }
+
+ if (size() == oldsize)
+ return;
+
+ // If our size has changed because macro parameters have been detached
+ // or attached, we need to update the representation of all macros
+ // again, since the references stored in ArgumentProxy may now be
+ // invalid (bug 9418). This will not do any visible change except
+ // creating ArgumentProxy instances with valid references.
+ for (size_t i = 0; i < macroindices.size(); ++i) {
+ InsetMath * inset = operator[](macroindices[i]).nucleus();
+ if (inset->asScriptInset())
+ inset = inset->asScriptInset()->nuc()[0].nucleus();
+ MathMacro * macroInset = inset->asMacro();
+ LASSERT(macroInset, continue);
+ macroInset->updateRepresentation(cur, mc, utype);
}
}
