Am Freitag, 27. Januar 2006 03:20 schrieb Beck, Andrew Thomas - BECAT001:
> OK, here is a better formatted patch, preserving encoding etc. Thanks
> for the feedback. The update is still being done in metrics() in the
> same way that the macro expansion was being done previously. I have
> changed it to cast away the const-ness rather than make cells_ mutable.
> Assuming that the interface & function of the patch is acceptable I
> will now implement a doDispatch(). In addition to changing the name
> & number of arguments, I will also remove the expansion from metrics().
> This will also negate the need for the mutable on MathMacro::tmpl_ and
> MathMacro::expanded_.
Good.
> If I rename a macro template, does it seem reasonable to automatically
> update the name in all instantiations of that template?
I am not sure. What if a user wants to copy an existing macro template and
give it a new name?
Here follow some comments:
> --- text3.C 31 Dec 2005 11:40:32 -0000 1.323
> +++ text3.C 27 Jan 2006 02:06:30 -0000
> @@ -1254,10 +1254,10 @@
> else {
> string s = cmd.argument;
> string const s1 = token(s, ' ', 1);
> - int const nargs = s1.empty() ? 0 :
convert<int>(s1);
> +// int const nargs = s1.empty() ? 0 :
convert<int>(s1);
> string const s2 = token(s, ' ', 2);
> string const type = s2.empty() ? "newcommand" :
s2;
> - cur.insert(new MathMacroTemplate(token(s, ' ',
0), nargs, type));
> + cur.insert(new MathMacroTemplate(token(s, ' ',
0), s1, type));
I think it is better not to make the internal storage of the number of
arguments visible here. Simply provide an additional constructor with an
int argument that does the conversion.
> - MacroTable::globalMacros().get(name()).expand(cells_,
expanded_);
> - expanded_.metrics(mi, dim);
> + const idx_type defArgs =
static_cast<idx_type>(MacroTable::globalMacros().get(name()).numargs());
That should work without the cast.
> + if (nargs() < defArgs)
> + ((MathMacro*)this)->cells_.resize(defArgs);
> +
> + if (editing(mi.base.bv)) {
>
+ asArray(MacroTable::globalMacros().get(name()).def(),
tmpl_);
> + LyXFont font = mi.base.font;
> + augmentFont(font, "lyxtex");
> + tmpl_.metrics(mi, dim);
> + dim.wid += mathed_string_width(font, name()) +
10;
> + int ww = mathed_string_width(font, "#1: ");
Indentation is wrong. Please use tabs for logical indenting, and spaces
for alignment (e.g. for an if-clause over several lines)
> --- mathed/math_macro.h 5 Oct 2005 21:19:32 -0000 1.103
> +++ mathed/math_macro.h 27 Jan 2006 02:06:39 -0000
> @@ -55,10 +55,14 @@
> ///
> void infoize2(std::ostream &) const;
>
> -private:
> - virtual std::auto_ptr<InsetBase> doClone() const;
> + virtual MathMacro *asMacro() { return this; }
> + virtual MathMacro const *asMacro() const { return this; }
Here you have one of the broken points that I mentioned in my earlier
mail: asMacro() is already defined in math_inset.h and used elsewere, but
always returns 0!
The overall approach looks sensible. Provided you get rid of the
const_casts and find a way to update existing macros not in the metrics
function this would be a nice feature for 1.5.
Georg
