Abdelrazak Younes wrote:
>> + //It's not obvious this is the right place for this enum. Note
>> that it
>> + //serves a different purpose from the one in insetbase.C and so
>> can't be
>> + //replaced by it.
> Couldn't they be merged instead? See above.
The InsetBase::Code can be different even if we're dealing with the same
insets. For example, url and htmlurl are both commands for an InsetUrl,
but they map to differents Codes. So no, unfortunately.
> I am not well versed in this InsetCommand stuff but your patch looks
> good and the comments look sound. But, and this is a big BUT, please
> put this new conversion method command2ParamType() *outside* of the
> InsetCommandParams class. InsetCommandParams is a base class and
> should not know _anything_ about which type of class is using it.
This is the root of the problem. An instance of InsetCommandParams does
need to know what kind of inset is using it, because that's the only way
it can make sure that new commands passed to it via read() or
setCmdName() are legitimate. In any event, this information was already
stored in the name_ variable. I'm just storing it in a different form
and, in the process, dissocating the inset type from the command name,
which would allow a bit more flexibility if it were wanted. The key to
the patch is really the isCompatible() method, and that could have been
written without a converter or the InsetParamType enum thus:
if (newname == "bibtex")
if (oldname == "bibtext") return true;
else return false;
...
if (newname == "url" || newname == "htmlurl")
if (oldname == "url" || oldname == "htmlurl") return true;
else return false;
But that gets very ugly when you get more names involved. Hence the idea
of using the converter as a kind of helper method.
That said, I agree with you that maybe InsetCommandParams should be pure
virtual and that each kind of inset should subclass it for its own
purposes. And after looking very quickly at the code, I don't think this
would be very hard, though it would mean a lot of changes elsewhere,
e.g., in factory.C, where all the calls like:
InsetCommandParams p( "include")
would need to change to:
InsetIncludeCommandParams(p)
or perhaps the InsetCommandParams constructor could become a kind of
factory method. And there are some other comments in this code that
suggest it needs cleanup in other ways, too. This obviously isn't the
time for such re-organization. But I can add an additional comment to
this effect, if you think it's worth it, and plan to do it once things
calm down a bit.
Richard
--
==================================================================
Richard G Heck, Jr
Professor of Philosophy
Brown University
http://frege.brown.edu/heck/
==================================================================
Get my public key from http://sks.keyserver.penguin.de
Hash: 0x1DE91F1E66FFBDEC
Learn how to sign your email using Thunderbird and GnuPG at:
http://dudu.dyn.2-h.org/nist/gpg-enigmail-howto
