Re: test of math previews
On Tue, Jun 23, 2015 at 11:14:29AM -0400, Richard Heck wrote: On 06/22/2015 06:59 PM, Enrico Forestieri wrote: On Sat, Jun 20, 2015 at 02:11:53AM +0200, Enrico Forestieri wrote: On Fri, Jun 19, 2015 at 04:50:43PM +0100, Guillaume M-M wrote: Also, lyx-preview-macros-2-lassert.lyx no longer lasserts but on opening the preview is not generated because \Coloneqq is not defined. This looks like the bug that you are referring to in your message where packages for symbols are not properly loaded. But, notice that the preview is correctly generated on a second time, when entering and leaving the math inset. So maybe it's easier to fix than it seemed. But indeed it's not a regression. Yes, this has always been the case, seemingly. However, now one can obtain a preview after entering and exiting a math inset, while previously the preview was simply never generated. So, this is a step forward. As regards the reason it occurs, I have still to investigate the matter. I had a closer look and this is a quite strange issue. It seems that this occurs only for a lonely math inset. Enable instant preview for math and then try loading the attached examples. Only the first one will fail. I think that this is due to the fact that the MacroData of the global macros defined in the lib/symbols file is set only after the first call to updateMacros() and when there is only a math inset, that method has no chance of being called and thus we miss the requirements. This seems to be confirmed by the attached patch for master which allows to preview all of the attached documents on loading. The patch is quite simple but it seems to cure a very particular case, so I am not sure it is worth applying. What do others think? I always imagine there are other ways to cause this sort of problem, e.g., on reload of a document, or maybe someone has a small child document with only one macro in it. (They use that complicated macro in a lot of different papers, say, but not all the time.) Ok, I will commit it, then. -- Enrico
Re: test of math previews
On Tue, Jun 23, 2015 at 10:20:21PM +0200, Georg Baum wrote:
Enrico Forestieri wrote:
I don't know, maybe this is a case of a race condition. Most probably,
with preview on, updateMacros() is not called (or is called later) and
the MacroData:sym_ pointer is not updated at the time MacroData::xlmname()
is called so that it contains bogus values. This seems to be confirmed by
the attached patch that explicitly sets to null the MacroData::sym_
pointer whenever a macro is copied. With this patch the crash is gone for
me.
I think that this is to be considered as a different instance of #9418,
as also witnessed by the exact same backtrace.
This is interesting. In theory, sym_ should only be set once, and only for
global macros from initSymbols(). For user defined macros it should always
be 0. According to my valgrind investigations of #9490 (which is exactly the
same problem as reported in this thread IMHO), the crash happens because the
MathMacro memory is already freed when MathMacro::mathmlize() is called.
Therefore, accessing d-macro_ invokes undefined behaviour. If this is true
then setting sym_ to 0 would only cure the symptom, but since I was unable
up to now to pinpoint the real cause of the crash I rather do not trust my
own findings very much anymore.
I think I understand what happens here and it seems to be a different
problem than #9490. All macros are stored using a map and when
Buffer::updateMacros() is called, it first clears the map and then
reconstructs it. The d-macro_ pointer points to a MacroData in this map
but is only updated when MathData::metrics() is called. Now, when instant
preview for math is on, MathData::metrics() is never called when
selecting a math inset, so that d-macro_ is not updated and now
d-macro_-sym_ is a bogus pointer and hence the crash when hitting Ctrl+C.
Luckily, when d-macros_ is updated by MathMacro::updateMacro(), a copy
of the pointed to data is also made. This is the data the now obsolete
d-macro_ was pointing to before Buffer::updateMacros() reorganized
things. This data contains the necessary info for trying to update
d-macro_ in the copy constructor and, if it still fails, we can simply
let d-macro_ point to that data directly.
The attached patch fixes the crash for me and I am going to commit it.
--
Enrico
diff --git a/src/mathed/MacroTable.h b/src/mathed/MacroTable.h
index 3bd04ea..030e6f7 100644
--- a/src/mathed/MacroTable.h
+++ b/src/mathed/MacroTable.h
@@ -69,6 +69,8 @@ public:
char const * MathMLtype() const;
///
void setSymbol(latexkeys const * sym) { sym_ = sym; }
+ ///
+ DocIterator const pos() { return pos_; }
/// lock while being drawn to avoid recursions
int lock() const { return ++lockCount_; }
diff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
index 24ebe77..8f2d15f 100644
--- a/src/mathed/MathMacro.cpp
+++ b/src/mathed/MathMacro.cpp
@@ -203,6 +203,14 @@ MathMacro::MathMacro(MathMacro const that)
: InsetMathNest(that), d(new Private(*that.d))
{
d-updateChildren(this);
+ if (d-macro_ lyxrc.preview == LyXRC::PREVIEW_ON) {
+ // We need to update d-macro_ by ourselves because in this case
+ // MathData::metrics() is not called when selecting a math inset
+ DocIterator const pos = d-macroBackup_.pos();
+ d-macro_ = pos.buffer()-getMacro(name(), pos);
+ if (!d-macro_)
+ d-macro_ = d-macroBackup_;
+ }
}
@@ -213,6 +221,14 @@ MathMacro MathMacro::operator=(MathMacro const that)
InsetMathNest::operator=(that);
*d = *that.d;
d-updateChildren(this);
+ if (d-macro_ lyxrc.preview == LyXRC::PREVIEW_ON) {
+ // We need to update d-macro_ by ourselves because in this case
+ // MathData::metrics() is not called when selecting a math inset
+ DocIterator const pos = d-macroBackup_.pos();
+ d-macro_ = pos.buffer()-getMacro(name(), pos);
+ if (!d-macro_)
+ d-macro_ = d-macroBackup_;
+ }
return *this;
}
Re: test of math previews
On Mon, Jun 22, 2015 at 11:32:17PM +0100, Guillaume M-M wrote: 3) Previews have the incorrect size with layouts that offer font sizes other than 1[0-2]pt (lyx-preview-fontsize.lyx). The legacy method offers a function that retrieves the actual font size and calculates the actual dpi. Attached is a patch (extract_resolution.diff) that imports this function in the new script. Thank you, this is much better than trying to catch all possible cases. I have applied the patch to master at 0751f96d. -- Enrico
Re: test of math previews
On 06/24/2015 08:49 PM, Guillaume M-M wrote: Le 25/06/2015 00:44, Enrico Forestieri a écrit : On Mon, Jun 22, 2015 at 11:32:17PM +0100, Guillaume M-M wrote: 3) Previews have the incorrect size with layouts that offer font sizes other than 1[0-2]pt (lyx-preview-fontsize.lyx). The legacy method offers a function that retrieves the actual font size and calculates the actual dpi. Attached is a patch (extract_resolution.diff) that imports this function in the new script. Thank you, this is much better than trying to catch all possible cases. I have applied the patch to master at 0751f96d. Thank you for applying it. Here's a trivial patch for stable, if you are not ready to backport 0751f96d. Both were tested in stable. This is fine with me, as is the original patch, as Enrico sees fit. Richard
Re: test of math previews
Le 25/06/2015 00:44, Enrico Forestieri a écrit : On Mon, Jun 22, 2015 at 11:32:17PM +0100, Guillaume M-M wrote: 3) Previews have the incorrect size with layouts that offer font sizes other than 1[0-2]pt (lyx-preview-fontsize.lyx). The legacy method offers a function that retrieves the actual font size and calculates the actual dpi. Attached is a patch (extract_resolution.diff) that imports this function in the new script. Thank you, this is much better than trying to catch all possible cases. I have applied the patch to master at 0751f96d. Thank you for applying it. Here's a trivial patch for stable, if you are not ready to backport 0751f96d. Both were tested in stable. Guillaume diff --git a/lib/scripts/lyxpreview2bitmap.py b/lib/scripts/lyxpreview2bitmap.py index 9775b0e..8cd3c09 100755 --- a/lib/scripts/lyxpreview2bitmap.py +++ b/lib/scripts/lyxpreview2bitmap.py @@ -159,7 +159,7 @@ def extract_metrics_info(dvipng_stdout): def fix_latex_file(latex_file, pdf_output): -documentclass_re = re.compile((documentclass\[)(1[012]pt,?)(.+)) +documentclass_re = re.compile((documentclass\[)([0-9]+pt,?)(.+)) def_re = re.compile(r(\\newcommandx|\\global\\long\\def)(\\[a-zA-Z]+)) tmp = mkstemp()
LyX-Function textstyle-update
Hi, could we please have also a more user friendly lyx-function? For instance we have to use textstyle-update ... color 16 ... to set the color to red. Any key-binding using numbers has a risk of being invalid if the corresponding number changes. (As recently happend) The function may be named e.g. textstyle-update-user, and accept also symbolic values. Kornel signature.asc Description: This is a digitally signed message part.
LyX-Function textstyle-update
Hi, could we please have also a more user friendly lyx-function? For instance we have to use textstyle-update ... color 16 ... to set the color to "red". Any key-binding using numbers has a risk of being invalid if the corresponding number changes. (As recently happend) The function may be named e.g. "textstyle-update-user", and accept also symbolic values. Kornel signature.asc Description: This is a digitally signed message part.
Re: test of math previews
On Tue, Jun 23, 2015 at 10:20:21PM +0200, Georg Baum wrote:
> Enrico Forestieri wrote:
>
> > I don't know, maybe this is a case of a race condition. Most probably,
> > with preview on, updateMacros() is not called (or is called later) and
> > the MacroData:sym_ pointer is not updated at the time MacroData::xlmname()
> > is called so that it contains bogus values. This seems to be confirmed by
> > the attached patch that explicitly sets to null the MacroData::sym_
> > pointer whenever a macro is copied. With this patch the crash is gone for
> > me.
> >
> > I think that this is to be considered as a different instance of #9418,
> > as also witnessed by the exact same backtrace.
>
> This is interesting. In theory, sym_ should only be set once, and only for
> global macros from initSymbols(). For user defined macros it should always
> be 0. According to my valgrind investigations of #9490 (which is exactly the
> same problem as reported in this thread IMHO), the crash happens because the
> MathMacro memory is already freed when MathMacro::mathmlize() is called.
> Therefore, accessing d->macro_ invokes undefined behaviour. If this is true
> then setting sym_ to 0 would only cure the symptom, but since I was unable
> up to now to pinpoint the real cause of the crash I rather do not trust my
> own findings very much anymore.
I think I understand what happens here and it seems to be a different
problem than #9490. All macros are stored using a map and when
Buffer::updateMacros() is called, it first clears the map and then
reconstructs it. The d->macro_ pointer points to a MacroData in this map
but is only updated when MathData::metrics() is called. Now, when instant
preview for math is on, MathData::metrics() is never called when
selecting a math inset, so that d->macro_ is not updated and now
d->macro_->sym_ is a bogus pointer and hence the crash when hitting Ctrl+C.
Luckily, when d->macros_ is updated by MathMacro::updateMacro(), a copy
of the pointed to data is also made. This is the data the now obsolete
d->macro_ was pointing to before Buffer::updateMacros() reorganized
things. This data contains the necessary info for trying to update
d->macro_ in the copy constructor and, if it still fails, we can simply
let d->macro_ point to that data directly.
The attached patch fixes the crash for me and I am going to commit it.
--
Enrico
diff --git a/src/mathed/MacroTable.h b/src/mathed/MacroTable.h
index 3bd04ea..030e6f7 100644
--- a/src/mathed/MacroTable.h
+++ b/src/mathed/MacroTable.h
@@ -69,6 +69,8 @@ public:
char const * MathMLtype() const;
///
void setSymbol(latexkeys const * sym) { sym_ = sym; }
+ ///
+ DocIterator const & pos() { return pos_; }
/// lock while being drawn to avoid recursions
int lock() const { return ++lockCount_; }
diff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
index 24ebe77..8f2d15f 100644
--- a/src/mathed/MathMacro.cpp
+++ b/src/mathed/MathMacro.cpp
@@ -203,6 +203,14 @@ MathMacro::MathMacro(MathMacro const & that)
: InsetMathNest(that), d(new Private(*that.d))
{
d->updateChildren(this);
+ if (d->macro_ && lyxrc.preview == LyXRC::PREVIEW_ON) {
+ // We need to update d->macro_ by ourselves because in this case
+ // MathData::metrics() is not called when selecting a math inset
+ DocIterator const & pos = d->macroBackup_.pos();
+ d->macro_ = pos.buffer()->getMacro(name(), pos);
+ if (!d->macro_)
+ d->macro_ = >macroBackup_;
+ }
}
@@ -213,6 +221,14 @@ MathMacro & MathMacro::operator=(MathMacro const & that)
InsetMathNest::operator=(that);
*d = *that.d;
d->updateChildren(this);
+ if (d->macro_ && lyxrc.preview == LyXRC::PREVIEW_ON) {
+ // We need to update d->macro_ by ourselves because in this case
+ // MathData::metrics() is not called when selecting a math inset
+ DocIterator const & pos = d->macroBackup_.pos();
+ d->macro_ = pos.buffer()->getMacro(name(), pos);
+ if (!d->macro_)
+ d->macro_ = >macroBackup_;
+ }
return *this;
}
Re: test of math previews
On Tue, Jun 23, 2015 at 11:14:29AM -0400, Richard Heck wrote: > On 06/22/2015 06:59 PM, Enrico Forestieri wrote: > >On Sat, Jun 20, 2015 at 02:11:53AM +0200, Enrico Forestieri wrote: > >>On Fri, Jun 19, 2015 at 04:50:43PM +0100, Guillaume M-M wrote: > >> > >>>Also, lyx-preview-macros-2-lassert.lyx no longer lasserts but on opening > >>>the > >>>preview is not generated because \Coloneqq is not defined. This looks like > >>>the bug that you are referring to in your message where packages for > >>>symbols > >>>are not properly loaded. But, notice that the preview is correctly > >>>generated > >>>on a second time, when entering and leaving the math inset. So maybe it's > >>>easier to fix than it seemed. But indeed it's not a regression. > >>Yes, this has always been the case, seemingly. However, now one can obtain > >>a preview after entering and exiting a math inset, while previously the > >>preview was simply never generated. So, this is a step forward. As regards > >>the reason it occurs, I have still to investigate the matter. > >I had a closer look and this is a quite strange issue. It seems that this > >occurs only for a lonely math inset. Enable instant preview for math and > >then try loading the attached examples. Only the first one will fail. > > > >I think that this is due to the fact that the MacroData of the global > >macros defined in the lib/symbols file is set only after the first > >call to updateMacros() and when there is only a math inset, that method > >has no chance of being called and thus we miss the requirements. > >This seems to be confirmed by the attached patch for master which allows > >to preview all of the attached documents on loading. > > > >The patch is quite simple but it seems to cure a very particular case, > >so I am not sure it is worth applying. What do others think? > > I always imagine there are other ways to cause this sort of problem, e.g., > on reload of a document, or maybe someone has a small child document with > only one macro in it. (They use that complicated macro in a lot of different > papers, say, but not all the time.) Ok, I will commit it, then. -- Enrico
Re: test of math previews
On Mon, Jun 22, 2015 at 11:32:17PM +0100, Guillaume M-M wrote: > > 3) Previews have the incorrect size with layouts that offer font sizes other > than 1[0-2]pt (lyx-preview-fontsize.lyx). The legacy method offers a > function that retrieves the actual font size and calculates the actual dpi. > Attached is a patch (extract_resolution.diff) that imports this function in > the new script. Thank you, this is much better than trying to catch all possible cases. I have applied the patch to master at 0751f96d. -- Enrico
Re: test of math previews
Le 25/06/2015 00:44, Enrico Forestieri a écrit :
On Mon, Jun 22, 2015 at 11:32:17PM +0100, Guillaume M-M wrote:
3) Previews have the incorrect size with layouts that offer font sizes other
than 1[0-2]pt (lyx-preview-fontsize.lyx). The legacy method offers a
function that retrieves the actual font size and calculates the actual dpi.
Attached is a patch (extract_resolution.diff) that imports this function in
the new script.
Thank you, this is much better than trying to catch all possible cases.
I have applied the patch to master at 0751f96d.
Thank you for applying it.
Here's a trivial patch for stable, if you are not ready to backport
0751f96d. Both were tested in stable.
Guillaume
diff --git a/lib/scripts/lyxpreview2bitmap.py b/lib/scripts/lyxpreview2bitmap.py
index 9775b0e..8cd3c09 100755
--- a/lib/scripts/lyxpreview2bitmap.py
+++ b/lib/scripts/lyxpreview2bitmap.py
@@ -159,7 +159,7 @@ def extract_metrics_info(dvipng_stdout):
def fix_latex_file(latex_file, pdf_output):
-documentclass_re = re.compile("(documentclass\[)(1[012]pt,?)(.+)")
+documentclass_re = re.compile("(documentclass\[)([0-9]+pt,?)(.+)")
def_re = re.compile(r"(\\newcommandx|\\global\\long\\def)(\\[a-zA-Z]+)")
tmp = mkstemp()
Re: test of math previews
On 06/24/2015 08:49 PM, Guillaume M-M wrote: Le 25/06/2015 00:44, Enrico Forestieri a écrit : On Mon, Jun 22, 2015 at 11:32:17PM +0100, Guillaume M-M wrote: 3) Previews have the incorrect size with layouts that offer font sizes other than 1[0-2]pt (lyx-preview-fontsize.lyx). The legacy method offers a function that retrieves the actual font size and calculates the actual dpi. Attached is a patch (extract_resolution.diff) that imports this function in the new script. Thank you, this is much better than trying to catch all possible cases. I have applied the patch to master at 0751f96d. Thank you for applying it. Here's a trivial patch for stable, if you are not ready to backport 0751f96d. Both were tested in stable. This is fine with me, as is the original patch, as Enrico sees fit. Richard
