Andre Poenitz wrote:
On Tue, Apr 01, 2008 at 07:05:41PM -0400, rgheck wrote:
There's are 400 lines of "general" read/write infrastructure plus three
extra static methods per inset that in the end achieve something pretty
similar to
void InsetMathSqrt::write(WriteStream & os) const
{
os << "\\sqrt{" << cell(0) << '}';
}
void Parser::parse1(InsetMathGrid & grid, unsigned flags,
const mode_type mode, const bool numbered)
{
[...]
else if (t.cs() == "sqrt") { [...]
cell->push_back(MathAtom(new
InsetMathSqrt));
parse(cell->back().nucleus()->cell(0),
FLAG_ITEM, mode);
}
[...]
}
i.e. in each class derived from InsetCommand there is already more code
needed just to _interface_ the "nice general" code in InsetCommandParams
than an inset specific read/write implemention would take. This is
cancer that should not be allowed to grow any further.
Where is all this extra code in, say, InsetHyperlink? I don't see it.
///
static ParamInfo const & findInfo(std::string const &);
///
static std::string defaultCommand() { return "href"; };
///
static bool isCompatibleCommand(std::string const & s)
{ return s == "href"; }
ParamInfo const & InsetHyperlink::findInfo(string const & /* cmdName */)
{
static ParamInfo param_info_;
if (param_info_.empty()) {
param_info_.add("name", ParamInfo::LATEX_OPTIONAL);
param_info_.add("target", ParamInfo::LATEX_REQUIRED);
param_info_.add("type", ParamInfo::LATEX_REQUIRED);
}
return param_info_;
}
But (a) none of this code would vanish if you rolled ICP into
InsetCommand. All of that information would still be needed, and I'd
still want at least the second and third method to be static. And (b),
if you're not going to roll ICP into the insets (which, see my other
message, is not as trivial as it might seem), then you're going to need
some kind of factory, and that's precisely what that code is. This was
the design choice Georg et al made. They could have subclassed ICP and
implemented the factory in a different way. But then (said Georg), you'd
need glue code and casts in other places, and it seemed simpler (to
them) to do it this way. The bigger question has to do with having ICP
separate from the insets, and I've explained elsewhere why it's like
that. This is equally true for InsetGraphics as for InsetBibtex and has
nothing to do with ICP as such.
Anyway, I'm not insisting upon converting InsetGraphicsParam. If the
patch makes things worse, we shouldn't do it, obviously. I won't know
that until I look more closely. But if it loses 200 lines of code, I
suspect that will make you happy. ;-)
You mean I'd end up prefering
docstring bibfiles = getParam("bibfiles");
docstring embfiles = getParam("embed");
// do something with bibfiles and embfiles
over
// do something with params_.filename (InsetGraphicsParam)
[snip]
?
That's unlikely.
You can perfectly well do:
params_["bibfiles"]
if you like. Both methods are used, for historical reasons, I'd guess.
I've been trying to clean this up a bit.
rh
