Bo Peng wrote:
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.
I am still not convinced that, even after I can read and restore
params correctly, the need to separate params and EmbeddedFileList.
IMHO, having a single representation is easier to maintain.
I agree, but you don't have a single representation. Some parameters are
represented one way, and some are represented another way. See line 110
below.
As far as
I can tell, params are changed only during inset creation, copying,
and modification, and they are already handled correctly now. (And
params are updated after EmbeddedFileList changes.)
Could you please explain this? When does this happen, and why do the
inset's *parameters* need to be updated as a result of external changes?
I suspect it's not the parameters that need updating but rather
information that the EmbeddedFile object needs. If so, then let's do
that. Indeed, it seems to me that the inset's parameters ought NOT to be
updated from outside like this, except possibly for the "embed"
parameter that holds the inzipName. If so, then let's just do that.
The only problem
is the restoration of original user input for latex output purposes,
and my short patch fixes that easily.
If I understand you correctly, you would like to update params in the
traditional way, and output EmbeddedFileList when necessary. However,
because file movement needs to happen *during* inset creation,
copying and modification, you have to create an embedded file in those
events, enable them to move files around, and disable embedding if
file movement fails.
Why does it need to happen then? (I understand that this is a high-level
design question.) This seems like something that needs to happen only
when there are certain buffer-level events: Export, saving, etc. As long
as it gets done before it needs doing, isn't that good enough? My idea
was to do it when the EmbeddedFileList is accessed, "just in time".
Surely it can't need doing before that list is read. Of course, as you
say below, if the embedding fails for some reason, then we need to
change the parameters. But that can be done.
If, in some case as you mentioned, some developer
change params without checking EmbeddedFile, lyx may crash due to
inconsistent embedded file. THAT IS TO SAY, you can NOT change params
independently, so what is the point of separating them?
If the problem is that bad, then the solution you're suggesting would
require us to reimplement the whole parameter-setting mechanism, it
seems to me. Otherwise, you are asking for problems down the road. If we
have to go that way, we can, of course, but it seems very messy to me.
And I don't see why the InsetCommand structure should change just to
accommodate the EmbeddedFile feature.
In programming terms, you first need to determine if you want to use
enable(...., true) in your getEmbeddedFileList function. The answer is
obviously no (use enable(.... false)), because you do not want to
update an embedded file from external file each time it is used. Then,
if you use enable(...., false), you will have to make sure your params
are CORRECT, meaning that if a file is embedded, the embedded file
indeed exists.
OK, that would need sorting out. But let's come back to it if necessary.
Part of why I'm puzzled is that I don't see where this is happening in
the existing code. This is the old code:
87 void InsetBibtex::doDispatch(Cursor & cur, FuncRequest & cmd)
91 case LFUN_INSET_MODIFY: {
92 InsetCommandParams p(BIBTEX_CODE);
93 try {
94 if (!InsetCommand::string2params("bibtex",
95 to_utf8(cmd.argument()),
p)) {
96 cur.noUpdate();
97 break;
98 }
99 } catch (ExceptionMessage const & message) {
...
108 createBibFiles(p["bibfiles"], p["embed"]);
109 updateParam();
110 setParam("options", p["options"]);
111 buffer().updateBibfilesCache();
112 break;
113 }
And here's createBibFiles:
749 void InsetBibtex::createBibFiles(docstring const & bibParam,
750 docstring const & embedParam) const
751 {
752 bibfiles_.clear();
...
763 bibfiles = split(bibfiles, tmp, ',');
764 embedStatus = split(embedStatus, emb, ',');
765
766 while (!tmp.empty()) {
767 EmbeddedFile file(changeExtension(tmp, "bib"),
buffer().filePath());
768
769 file.setInzipName(emb);
770 file.setEmbed(!emb.empty());
771 file.enable(buffer().embedded(), &buffer(), false);
772 bibfiles_.push_back(file);
773 // Get next file name
774 bibfiles = split(bibfiles, tmp, ',');
775 embedStatus = split(embedStatus, emb, ',');
776 }
777 }
This destroys the old EFList and creates a new one from scratch every
time the inset is modified. Now if I use the ICP representation of the
parameters, why can't I just do this instead:
108 setParams(p);
109 createBibFiles();
110 // changing the signature to use our params
Surely this is equivalent to your code, right? But then, to ask the
question from before again, why do I have to call createBibFiles() then?
Why not wait until we access the bibfiles_ data to do it?
And as far as the true/false issue goes, why don't I have that problem
at line 771? Answer: Because you do the true call in GuiBibtex. But
that's insufficient, because this doesn't have to be called from there.
LFUNs can be called in lots of ways, including from the mini-buffer. So
you really need true here. Or else, what seems a better solution, don't
destroy bibfiles_ and recreate it. Just make the new ones and do the
true call for those. But I can do that as well, right?
Richard
