I think we're nearing a solution.
You've got two parallel
representations of the parameters, one in the EmbeddedFileList and one
in the InsetCommandParams, and there is simply no way to guarantee that
they are synchronized and therefore that you don't over-write
information that was committed to one of them from elsewhere. For
example, you can do this:
setParam("bibfiles", bibfilelist);
but then what happens with the metaInfo? Nothing. Or you can do this:
setParams(p); // InsetCommandParams(p)
and again we're out of sync. You can override those methods, I suppose,
but then I can do this:
p_["bibfiles"] = bibfilelist;
What I was proposing was the use of EmbeddedFileList, and convert to
and from params ONLY during construction, and file export. There
should be no more conversion and synchronization between them, so less
confusion.
But the point is that you simply can't enforce this, not without a lot
more code, and maybe not at all. The failure to keep to this policy is
precisely the cause of the problems I have been trying to solve.
The problems I originally encountered were precisely in this area. Of
course, if one were extremely careful not to use the methods of setting
parameters, etc, that the InsetCommand infrastructure provides, then we
won't have these problems. But surely we will have these problems,
because all of that is so natural to do.
Exactly because we need more information, we need to code around the
structure with more information, that is EmbeddedFileList. Setting
params directly should be discouraged, at least in InsetBibtex.
Discouraged? How? My worry is that this is unmaintainable.
This doesn't mean you can't have the EmbeddedFile objects in the insets.
You can if you like. But what you cannot do is use them to represent the
parameter information. It would be trivial to add this sort of thing to
my patch: Give InsetBibtex an EmbeddedFileList, and then, whenever it is
needed, update it with information from the InsetCommandParams. But the
only difference between this approach and mine is that I don't keep this
list around but construct it from scratch when it's needed.
The problem is that EmbeddedFile(s) should be kept consistently, and
should not be created on the fly. Because params does not have all the
information EmbeddedFile needs, you will have to enable a file each
time, and you will have a hard choice of whether or not you need to
move files around. With a consistent EmbeddedFile object, you *know*
the embedded files is there, and whether or not it needs to be
updated.
Yes, I see this issue. So maybe you also want the EmbeddedFile object,
and we can do that.
Take my patch and add an EmbeddedFileList, but do NOT use it to
represent the parameters. Then we can restore a lot of the code you had,
but we just have to make sure that the EmbeddedFileList gets updated
from the parameters whenever it is used. So setBuffer() would look like
this:
void InsetBibtex::setBuffer(Buffer & buffer)
{
+ // FIXME We ought to have a buffer.
if (buffer_) {
+ updateEmbeddedFileList(); // new method
EmbeddedFileList::iterator it = bibfiles_.begin();
EmbeddedFileList::iterator it_end = bibfiles_.end();
for (; it != it_end; ++it) {
try {
*it = it->copyTo(&buffer);
} catch (ExceptionMessage const & message) {
Alert::error(message.title_, message.details_);
// failed to embed
it->setEmbed(false);
}
}
}
This is the best of both worlds, right? And we could even do better,
potentially, by allowing access to bibfiles_ only via a method that
would do the update for us. E.g.:
class BibFileList {
BibFileList(InsetBibtex const * inset) : owner_(inset) {}
EmbeddedFileList const & get() const { update(); return list_; }
EmbeddedFileList & get() { update(); return list_; }
private:
update() { //stuff to do with owner_ that updates the list }
InsetBibtex const * owner_;
EmbeddedFileList list_;
}
And now in InsetBibtex:
BibFileList bibfiles_;
whence setBuffer does:
EmbeddedFileList const & efl = bibfiles_.get();
etc.
If you're agreed, I can change the patch this way. The only task is to
write the update() method, really---we could just take over the old
createBibFiles, but that seems like overkill---and for me to undo some
other bits, like registerEmbeddedFile(). OK?
I knew we'd figure this out eventually.
rh
