Am Freitag, 20. April 2007 17:13 schrieb Richard Heck:
>
> 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.
We were not aware of that either :-(
> > 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.:
This is not needed. The constructor of InsetCommand could call the virtual
method lyxCode() and compare it with the (to-be-added) code in the info
table of the params.
> 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."));
> }
This looks like a misunderstanding. I did not mean to compare command
names, I meant to compare a to-be-added field of the CommandInfo struct
that would store the InsetBase::Code of a command.
One option that I also investigated was to make the different inset
constructors pass a CommandInfo struct to the InsetCommandParams
constructor. I did not do that, because then the knowledge of the
different params would also need to be outside the insets, e.g. the
factory.
> 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.
The solution with the additional field in CommandInfo would not require any
duplication (but see below for a better one). Currently, the info table of
InsetCommandParams is the only place where information about commands is
stored, and it would remain like that.
> 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?
That would be a bad UI IMHO. We will never have arbitrary command names,
even if we add more flexibility we will at least have a text file where
possible names are declared, so the dialog should present a list to choose
from and no text field. IMHO we can trust the dialogs to do the right
thing. If we don't we would have to add a lot of checks to all insets.
> 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.
A check needs to be done at all places where stuff comes from outside. This
is the minibuffer and the server. If the code exists anyway one could also
check input from .lyx files, but as I wrote we don't do that in many other
places either.
I don't understand why you would want to check in doDispatch(), if the
check in read() is called anyway. To me it looks like read() is the only
place where a check needs to be done. If this check uses exceptions, the
callers could act appropriately.
> 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 has to be seen in historical context. It was like that forever, and
until now I saw no reason to change it.
> 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.
The explicit inset information was simply not needed so far. Therefore it
is only in the comments.
> 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.
This would not help much. You would still need to ensure that the new name
matches the old info, and do some error handling if that is not the case,
e.g. throw some exception. Therefore you can as well do the same error
handling in findInfo().
> 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?
Apart from the still needed error handling there is another problem: The
old parameter values would be destroyed, or they would need to be
explicitly copied.
What about this solution:
- Add an InsetBase::Code const field to InsetCommandParams that denotes the
type of inset. I don't see any reason to allow to change the inset type of
a params instance.
- Change the InsetComnmandParams constructor to take an InsetBase::Code
instead of a name. name_ would simply be set to the first possible name of
that inset.
- Adjust findInfo() and setCmdName() accordingly, with appropriate error
handling.
This looks very simple to me, does not duplicate anything, and I believe
that it makes all needed information for better error handling available.
> 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.
Every call of getContents() is made in old code that needs to be converted
to the new syntax. For example, the idea behind replaceContents() is
wrong: This has nothing to do with some generic "contents", it should
rather be named replaceKeys(), because it simply updates keys after one
got renamed. After these places are fixed it is easy to get rid of all the
obsolete methods. I did not do that so far because some months ago such
cleanup was forbidden.
Georg
