Andre Poenitz wrote:
On Sun, Mar 30, 2008 at 05:51:24AM -0400, rgheck wrote:
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.
Let me just say this. I'm not arguing that InsetCommandParams is the
be-all and end-all. If we want to re-work the way InsetCommandParams
works, then we can of course do that. I've already started to think
about that in fact. But that's not, I think, what Bo was proposing. And
I should perhaps add that, when I was discussing this with Georg, he was
absolutely insistent that something along the lines of what you're
suggesting was discussed and dismissed during the design of the present
system. That in itself is not an argument against it. But what I'm
talking about is how best to do this WITHOUT completely re-working the
InsetCommand hierarchy. It all seems to work quite well at the moment,
with the present exception.
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.
Some of us are less keen than others on the constant back and forth
between strings and other representations, and manipulating these will
require more such conversions. The nice thing about InsetCommandParams
is that it lets us do things like: params()["paramname"] = whatever. Of
course, we could do that somewhere else. It's the functionality that
matters. But I'd rather not have to convert back and forth between the
string representation and something else every time I want to assign a
parameter.
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.
Of course. So let me put it a different way: Unless Bo's proposing to
re-write the whole InsetCommand hierarchy, we should work with what we
have. This code is confusing enough without every inset's having its own
way of doing things, and part (again) of what makes ICP nice is that it
provides a consistent interface to the parameters that every
InsetCommand uses. (Weren't you berating me the other day about simple
and consistent interfaces? ;-) )
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...
This is one, because most of this code just goes into helper methods.
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.
This case is the issue. And my claim is that the code is easier to
understand and therefore easier to maintain if the InsetCommand-based
insets work in roughly the same way. And I speak from experience. I
thought I knew the InsetBibtex code real well. If it takes me a whole
day (when, frankly, I'd rather have been in the garden) to figure out
why kpsewhich support is broken because it takes me three or four hours
to figure out how InsetBibtex is now interacting with its parameters,
because I have to decipher the EmbeddedFiles code first, then, well,
that seems to me to be a problem.
rh
