This is long. I'm sorry. But there are a lot of issues here.
Georg Baum wrote:
> The case that you are adressing was not thought of: Stuff that comes from
> a .lyx file is assumed to be correct (we do this elsewhere, too), and we
> were not aware about the minibuffer.
>
The same issue arises with the server pipe, which is what I was actually
investigating. The minibuffer was just an easier way to test commands,
and a typo is what first triggered the crash.
>> 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.
>>
> This does not fit into the current design. Please do not soften it, if you
> wnat to go along the lines of your patch please do it completely,
I also don't know, apparently, what doing it completely would be. I
wasn't trying to do it half way!
> or use something that is compatible with the current design. For example, you
> could add an InsetBase::Code field to the CommandInfo struct. Then you can
> check very easily if the command is compatible to an inset by comparing the
> codes e.g. in the inset constructor.
I thought about that, and in a way that's similar to the pure virtual
idea. But there are problems. First, and least importantly, it seems as
if we would then have to re-write the constructor of every inset that
uses InsetCommandParams. That's easy enough, just copying and modifying
code from findInfo(), e.g.:
InsetRef::InsetRef(InsetCommandParams const & p, Buffer const & buf)
: InsetCommand(p, "ref"), isLatex(buf.isLatex())
{
string cmd = params().getCommandName();
if (cmd != "eqref" && cmd != "pageref" && cmd != "vpageref" &&
cmd != "vref" && cmd != "prettyref" && cmd != "ref")
throw ExceptionMessage(WarningException,
_("Invalid command"),
from_utf8("Command " + cmd + " not recognized by InsetRef."));
}
Maybe that's the right way to do it, but it seems like duplication.
Perhaps it could be done in the InsetCommand constructor, but then
you're putting all the information about which inset accepts which kind
of command there rather than in InsetCommandParams, and I again don't
see the difference.
More importantly, however, if you really want to deal with this kind of
crash, this isn't the only place this check would need to be made.
InsetCommandParams has a setCmdName() method where an incompatible
command name could just as well be set, and then you'd get the same
crash. At present, this is only called directly in the apply() methods
of various dialogs, and perhaps we can trust them to ensure that only
valid commands are ever passed. But maybe not---are we sure no dialog
will ever allow the user to enter a command directly? And maybe it could
be called from somewhere else. But worse still, I haven't actually
tried, but I'm prepared to wager that I can trigger the same crash using
inset-apply rather than inset-insert. If I have an open inset, I'll go
through InsetCommand::doDispatch to string2params and from there to
InsetCommandParams::read(). So now we're doing the check in doDispatch,
too. Is there anywhere else it has to be done? I have no idea.
Surely there needs to be some central place this check is done. The only
central place is in InsetCommandParams itself. And the real issue, for
me, is that it just seems all wrong to me to make the parameter list
depend on the name of the command rather than on the type of the inset.
This issue comes up very sharply when you start to think about
supporting BibLaTeX, since there is no predefined list of citation
commands in BibLaTeX. Yes, you could try making the list of commands in
bilbio.C more flexible and call is_possible_cite_command instead of
hard-coding that same list in findInfo. But it still seems backwards to
me to have the list of parameters be determined by the command rather
than by the type of inset. And what now happens in findInfo is in fact
that the command name is mapped to the inset type. It's just not done
that way, though the comments make it plain that this is in fact what is
happening.
But here's a different question. Is there really any good reason to
allow info_ to be reset? Resetting it is what really causes the crash.
It needs to be reset only if the list of acceptable parameters could
change depending upon what name_ was, which is to say: If the list of
parameters associated with a given inset could change depending upon
what LaTeX command it represented. The extant code allows for this, but
no inset actually works that way. So suppose for a minute that we
decided not to permit this. Then the calls to findInfo() that are
presently made in setCmdName() and read() can be deleted, and the crash
is no more.
Now suppose we did want to permit it---and, again, maybe proper BibLaTeX
support requires it, and maybe it just seems like a good idea to permit
that flexibility. Then the inset can handle this: It can just destroy
its old params_ and make a new one. That's the simplest solution by far.
What do you think?
> Tnis would be an extension of thecurrent design: Simply store one more item
> in the info table, but there is no duplicated list of command names.
>
> PS: Did you read the comments in the header? InsetCommandParams is in a
> transitional state, several methods that look ugly are supposed to go.
>
Yes. The ugly methods don't impinge on what I was doing, but I did
briefly investigate how to get rid of them. Some of them are simple, but
getContents() is called a lot.
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
