Andre Poenitz wrote:
On Wed, Apr 02, 2008 at 12:52:17PM -0400, Richard Heck wrote:
///
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.
Why oh why? What's wrong with mathed that it can live without it?
Why would we ever need to have 'isCompatibleCommand'? Just because you
need some kind of call back from the 'generic' ICP::read/write routines?
This is exactly a sign that the stuff is _not_ generic.
No, we don't need a callback, and I don't know about mathed---that is a
part of the code I dare not even examine, and if you say mathed doesn't
need it I'll believe you. But adding this method solved a crash in the
inset stuff, one I inadvertently triggered by entering an ill-formed
parameter string via the mini-buffer. (I don't recall the bug number. I
can find it if you like.) The short version is that the parameters for a
particular type of inset need to know what commands it accepts, if only
so that it can do error-checking. (An instance of ICP does know for
which kind of inset it is the parameters; this is just an alternative to
subclasses, hence the factory; there's no substantial issue there.) The
reason the method is static is because of the reason discussed in the
other thread: An instance of ICP (or even of InsetGraphicParams) can
float free of any inset; so we can't call isCompatibleCommand() on an
instance.
All this code used to be in ICP itself, but it seemed more sensible to
have each inset know what commands it can accept. And what parameters
those commands require, and of what types. There is a lot more
flexibility here now than there used to be. None of this even needs to
be hard-coded. You could have user-defined command insets that accepted
arbitrary parameters, all described in module files, kind of like how
custom flex insets work. It's just that these would be command insets.
If you're going to do that, you need a very general, and very flexible,
system for storing parameters. This is all on my to-do list, and we're
part of the way there. So we're going to have that kind of system,
either as ICP or InsetFlexCommandParams. Why not just use it?
This was the design choice Georg et al made.
They could have subclassed ICP and implemented the factory in a different
way.
No, I need at most inset specific data container which can store
each parameter in its respective "native" format (say lengths as
Length) and not everything in a std::map<std::string, docstring>.
Sure. If you want to go to inset specific parameter classes, I have no
strong objection; I suggested something like this implementation myself
to Georg in the conversations to which I keep referring. My idea was to
keep ICP as a virtual base class and subclass it for each inset; in many
cases, such as InsetLabel, that will be sufficient. In other cases, you
could do other things. Or maybe a different architecture is better. If
so, then so.
But I think what you suggest will make the code harder to understand,
because each inset will work differently from the rest. I say this from
experience. I understand very well how all the InsetCommand insets work;
understanding one is pretty much understanding all. This is not true of
InsetGraphics and InsetExternal. We could debate how much of a cost this
is. But it is a cost. And many of the some advantages of the approach
you are suggesting could be gained within ICP by associating type
information with parameters.
Restricting yourselves to this approach leeds to the kind of trouble
you are currently discussing with Bo: An embedded object seems to
be more than a string, and suddenly major infrastructure changes
_outside_ the "guilty" inset are needed just because it does not fit
well into the abstraction.
Yes, I agree, to some extent. I've made the same point in a different
thread. The problem is that the string representation is constraining.
But this is not just an issue for InsetBibtex; it's a general problem,
and it could have a general solution, if we wanted it to have one; there
is no reason that everything has to be a string.
But I disagree that embedded objects are parameters. In particular, I
think using an EmbeddedFile object to represent a pair of strings is
overkill that ultimately makes the code less comprehensible and less
maintainable, because (a) changes to the EmbeddedFile stuff could in
principle affect the inner workings of the insets and (b) you can't work
on the inset without figuring out the (extremely complex) EmbeddedFile
code. (Somewhere in there is the reason kpsewhich support got broken,
and there are still bugs elsewhere that we have for similar reasons.) As
far as I'm concerned, it's no argument that the EmbeddedFile objects are
already lying around. They should do the work they need to do and leave
the rest of the inset alone. And I'd feel exactly the same way about
this if we were talking about InsetBibtexParams. This issue has nothing
to do with ICP.
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"]
And the proper solution would be:
bibfiles_
if you like. Both methods are used, for historical reasons, I'd guess. I've
been trying to clean this up a bit.
I don't see a reason to access something that's conceptionally a simple
C structure as a map with string keys. This is pretty much the same kind
of overengineering we had with the old controllers.
I find it hard to get worked up about this.
rh
