On Sun, Mar 30, 2008 at 05:51:24AM -0400, rgheck wrote: > > 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.
So far it looks like we'd need a class 'EmbeddedObject', that's not derived from FileName, but, possibibly, has a 'fileName()' method to get a 'real' FileName if that is needed. > 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. And right now I start to get the impression that we a close to one of our good old attempts to abstract where abstraction isn't called for. To play the devil's advocate: What would be wrong with virtual string InsetCommand::parametersToString() const; and virtual void InsetCommand::parametersFromString(string const &); with suitable implementations in all derived classes? Common code could be put as helper method into InsetCommand. What exactly is InsetCommandParameter used for? Inset construction from a string? -> We can construct an 'empty' isnet and call parametersFromString() on it. Storing temporary copies in the dialogs? -> Store a stringified version there. We convert it to a string anyway when dispatching. > 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. We can change interfaces, we can keep them. That's no strong argument. > 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.) Typically maintainability increases as LOC decrease. There are exceptions, of course... > Well, that's my case. Bo, you want to state yours? ;-) No matter what he says I wonder whether the dispute is on the right level. I question I see is whether InsetCommandParams is the right thing to use (a) in this case, and (b) in general. Note that it original structure was just a convenient way to pass two or three data items typically associated with a simple LaTeX commands around. Nowadays it looks like a ton of bureaucracy sparking three-week discussions... Andre'
