So, to get everyone up to speed, Bo and I disagree about a question
concerning the design of InsetBibtex and, in particular, about how the
parameters of the inset should be handled. The original code, written (I
believe) by Georg back when the new InsetCommandParams structure was
conceived, handles the parameters via the InsetCommandParams structure.
The existing code, which Bo introduced a while back in connection with
the embedding feature, changes this. Where the old code would have used
InsetCommandParams, Bo uses an EmbeddedFileList, which is in the inset
for other reasons and which becomes the primary representation of the
parameters---primary in the sense that it's what gets used when the
parameters need to be known. The InsetCommandParams are reconstructed
from the EmbeddedFileList for use by e.g. the write() routine.
This has led to problems, because *.bib files aren't to be associated
with locations in the filesystem, and EmbeddedFile subclasses FileName
and so represents them exactly that way. (As JMarc has pointed out,
there are likely similar problems elsewhere. This will affect any case
where the file's actual location might be indifferent to TeX.) In short,
the problem is that, in the code, the "bibfiles" parameter might have
looked like this:
biblio,chomsky
whereas in the new code it looks like this:
/path/to/lyx/file/biblio.bib,/path/to/lyx/file/chomsky.bib
The question is how to solve this problem.
Bo has a patch that adds "metadata" to the EmbeddedFile object and then
uses this metadata to reconstruct the parameters. The patch could be
made to work, but I feel quite strongly that this is wrong. I would
prefer to go back more to the older representation, using
InsetCommandParams as the primary representation---in the sense that
it's what you consult when you want to know about the parameters---and
then constructing the EmbeddedFileList from it when and as necessary.
Note that, either way, we have to go back and forth between these
somewhere. This is unfortunate, but it seems built into the way
embedding works right now. (I've got other questions about whether it
should work that way, but that's a separate issue.)
It's seems pretty clear at this point that Bo and I don't agree, so the
advice of others would be welcome.
My reason to prefer my solution is simple. About a year ago, I tried to
solve some crashes related to InsetCommandParams. I posted a patch,
which got me a long and informative lecture from Georg and Abdel about
why InsetCommandParams and InsetCommand need to be kept separate. The
thread begins here:
http://www.mail-archive.com/lyx-devel@lists.lyx.org/msg114235.html
and Georg's sternest lecture is here:
http://www.mail-archive.com/lyx-devel@lists.lyx.org/msg114325.html
After studying the code, I came to agree with Georg, and after 1.5 came
out, I re-worked InsetCommandParams to fix the crashes in question and,
as a result, came even more to understand why the InsetCommand stuff is
organized the way it is. So I think I have some understanding of this
part of the code.
The issue is not how the parameters are represented. The issue is *how
an InsetCommand interacts with its parameters*. The design is that the
parameters are stored in InsetCommandParams, and the inset interacts
with them via the methods of InsetCommandParams. Whether there are good
reasons for this---even though there are---is almost irrelevant, because
that is how the code is organized. One could do something like this:
InsetCommand * inset = getCommandInset(); // could be anything
InsetCommandParams icp;
InsetCommand::string2params(insetName(inset.lyxCode(), paramStr, icp);
inset.setParams(icp);
(You could re-organize part of the inset factory this way if you wanted
to.) If one did that sort of thing now, it could lead to a crash,
because the EmbeddedFileList would be out of sync with the parameters.
Now there are ways around this sort of thing, to be sure. But my point
is that, however we approach this, we should use the existing interface.
The parameters should be the parameters, and the code should interact
with them in the way it would if there were no EmbeddedFileList in
InsetBibtex. That's my main point, so let me say it again: The
parameters should be the parameters, and the code should interact with
them in the way it would if there were no EmbeddedFileList in
InsetBibtex. For one thing, maybe at some point there is no longer an
EmbeddedFileList there, because the implementation of the embedding
feature changes again; we should limit the re-writing required in that
case and make sure that changes to that code don't affect this code. But
even if it doesn't ever change, it just doesn't seem sensible to me that
the introduction of this feature should have required such wholesale
changes to the way InsetBibtex works that, having spent as much time
with it as I have---I also reworked the way BibTeX data is
represented---it took me several hours to figure it out again and even
more to figure out why kpsewhich support was broken. A newcomer to the
code who had figured out how most InsetCommand derivatives work would be
lost here.
Even if we do things my way, we still have to keep the EmbeddedFileList
sync'd with the parameters, but the conversion should be automatic and
encapsulated it in overridden methods, e.g.:
void setParams(InsetCommandParams const & icp)
{
InsetCommand::setParams(icp);
updateEmbeddedFiles(); // this could modify the params if embedding
fails
}
We'll have to handle other methods, too, and that will take work and
mean more code. But the issue isn't LOC but maintainability, and a lot
of the required code has to do with the facts (a) that ICP stores and
reports all parameters as strings, whereas our parameters represent
lists, and (b) that we have two lists, bibfiles and embed, that we need
to iterate over together. It's kind of messy. (The really dramatic way
to do this would be to subclass InsetCommandParams, but I don't think we
need to go that far.)
Well, that's my case. Bo, you want to state yours?
Richard
