I'm no expert on this part of the code, but this doesn't look too
dangerous. Since 2.2.3 is still a little ways away, is it worth
committing to stable?

Richard



On 12/01/2016 12:03 PM, Enrico Forestieri wrote:
> commit e8f480e7c22ae29804ff0c386c54e86c9b72d3ce
> Author: Enrico Forestieri <for...@lyx.org>
> Date:   Thu Dec 1 18:02:47 2016 +0100
>
>     Fix display and output of math macros with optional arguments
>     
>     This is a long standing issue, present since the new math macros
>     inception in version 1.6. It manifests as a display issue when a
>     macro with optional arguments appears in the optional argument of
>     another macro. In this case the display is messed up and it is
>     difficult, if not impossible, changing the arguments as they do not
>     appear on screen as related to a specific macro instance. It also
>     manifests as latex errors when compiling, even if the latex output
>     is formally correct, due to limitations of the xargs package used
>     to output the macros. Most probably, both aspects have the same
>     root cause, as simply enclosing in braces the macro and its
>     parameters solves both issues. However, when reloading a document,
>     lyx strips the outer braces enclosing a macro argument, thus
>     frustrating this possible workaround.
>     
>     This commit solves the display issue by correctly accounting for
>     macros with optional arguments nested in the argument of another
>     macro, and circumvents the xargs package limitations causing errors
>     by enclosing in braces the macros with optional arguments appearing
>     in the argument of an outer macro when they are output. This means
>     that when loading an old document with such macros and saving it
>     again, the macro representation is updated and will have these
>     additional braces. However, as such braces are stripped by lyx on
>     loading, there is no risk that they accumulate.
>     
>     See also this thread:
>     http://www.mail-archive.com/lyx-devel@lists.lyx.org/msg197828.html
> ---
>  src/mathed/MathData.cpp   |    7 +++++--
>  src/mathed/MathMacro.cpp  |   17 +++++++++++++++--
>  src/mathed/MathStream.cpp |    8 ++++----
>  src/mathed/MathStream.h   |    6 ++++++
>  4 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/src/mathed/MathData.cpp b/src/mathed/MathData.cpp
> index f2100af..2a36e55 100644
> --- a/src/mathed/MathData.cpp
> +++ b/src/mathed/MathData.cpp
> @@ -659,12 +659,15 @@ void MathData::collectOptionalParameters(Cursor * cur,
>               if (operator[](pos)->getChar() != '[')
>                       break;
>  
> -             // found possible optional argument, look for "]"
> +             // found possible optional argument, look for pairing "]"
> +             int count = 1;
>               size_t right = pos + 1;
>               for (; right < size(); ++right) {
>                       MathAtom & cell = operator[](right);
>  
> -                     if (cell->getChar() == ']')
> +                     if (cell->getChar() == '[')
> +                             ++count;
> +                     else if (cell->getChar() == ']' && --count == 0)
>                               // found right end
>                               break;
>  
> diff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
> index 34b5160..afc40a4 100644
> --- a/src/mathed/MathMacro.cpp
> +++ b/src/mathed/MathMacro.cpp
> @@ -1068,6 +1068,15 @@ void MathMacro::write(WriteStream & os) const
>       // we should be ok to continue even if this fails.
>       LATTEST(d->macro_);
>  
> +     // We may already be in the argument of a macro
> +     bool const inside_macro = os.insideMacro();
> +     os.insideMacro(true);
> +
> +     // Enclose in braces to avoid latex errors with xargs if we have
> +     // optional arguments and are in the optional argument of a macro
> +     if (d->optionals_ && inside_macro)
> +             os << '{';
> +
>       // Always protect macros in a fragile environment
>       if (os.fragile())
>               os << "\\protect";
> @@ -1106,9 +1115,13 @@ void MathMacro::write(WriteStream & os) const
>               first = false;
>       }
>  
> -     // add space if there was no argument
> -     if (first)
> +     // Close the opened brace or add space if there was no argument
> +     if (d->optionals_ && inside_macro)
> +             os << '}';
> +     else if (first)
>               os.pendingSpace(true);
> +
> +     os.insideMacro(inside_macro);
>  }
>  
>  
> diff --git a/src/mathed/MathStream.cpp b/src/mathed/MathStream.cpp
> index ee65835..cf3fb51 100644
> --- a/src/mathed/MathStream.cpp
> +++ b/src/mathed/MathStream.cpp
> @@ -128,10 +128,10 @@ WriteStream & operator<<(WriteStream & ws, docstring 
> const & s)
>  WriteStream::WriteStream(otexrowstream & os, bool fragile, bool latex,
>                                                OutputType output, Encoding 
> const * encoding)
>       : os_(os), fragile_(fragile), firstitem_(false), latex_(latex),
> -       output_(output), pendingspace_(false), pendingbrace_(false),
> -       textmode_(false), locked_(0), ascii_(0), canbreakline_(true),
> -       mathsout_(false), ulemcmd_(NONE), line_(0), encoding_(encoding),
> -       row_entry_(TexRow::row_none)
> +       output_(output), insidemacro_(false), pendingspace_(false),
> +       pendingbrace_(false), textmode_(false), locked_(0), ascii_(0),
> +       canbreakline_(true), mathsout_(false), ulemcmd_(NONE), line_(0),
> +       encoding_(encoding), row_entry_(TexRow::row_none)
>  {}
>  
>  
> diff --git a/src/mathed/MathStream.h b/src/mathed/MathStream.h
> index 2a4d4d7..271330e 100644
> --- a/src/mathed/MathStream.h
> +++ b/src/mathed/MathStream.h
> @@ -81,6 +81,10 @@ public:
>       void ulemCmd(UlemCmdType ulemcmd) { ulemcmd_ = ulemcmd; }
>       /// tell which ulem command type we are inside
>       UlemCmdType ulemCmd() const { return ulemcmd_; }
> +     /// record whether we are in the argument of a math macro
> +     void insideMacro(bool insidemacro) { insidemacro_ = insidemacro; }
> +     /// tell whether we are in the argument of a math macro
> +     bool insideMacro() const { return insidemacro_; }
>       /// writes space if next thing is isalpha()
>       void pendingSpace(bool how);
>       /// writes space if next thing is isalpha()
> @@ -120,6 +124,8 @@ private:
>       int latex_;
>       /// output type (default, source preview, instant preview)?
>       OutputType output_;
> +     /// are we in the argument of a math macro?
> +     bool insidemacro_;
>       /// do we have a space pending?
>       bool pendingspace_;
>       /// do we have a brace pending?


Reply via email to