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

Reply via email to