Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Andre Poenitz
On Tue, Apr 01, 2008 at 06:49:25PM -0400, rgheck wrote:
 Andre Poenitz wrote:
  We won't know this until we look at fixing that. I'm inclined, actually, 
 to  do something more general, namely, to try to bring InsetGraphic into 
 the  InsetCommand hierarchy. I think this was intended all along as part 
 of a  more general transition that was never finished.
   
 Could you please elaborate on why having the InsetCommand hierarchy is
 useful at all?

 As far as I can tell the main benefit was to have a uniform means to
 pass Parameters through the Controller layer in the Old Days. This
 layer is gone now, and it feels a bit strange to pass around unrelated
 pieces of data from otherwise unrelated insets in a common structure.
 Wouldn't per-inset specific data with direct access from the GUI simplify
 overall structure and reduce conversion costs?
   

 Frankly, I don't know the overall structure here well enough to be sure 
 exactly what the advantages or disadvantages to keeping the 
 InsetCommandParams business the way it is. Mostly, I try to go on how Georg 
 explained this to me some time ago, and the understanding I've developed 
 since then of why he (they?) did things the way they did.

You refered to this conversation lately and I had a look then. It
contained sound arguments for the time, with the Controller layer being
still around - which is gone now.

 In particular, 
 Georg insisted that we are not to subclass InsetCommandParams, not unless 
 we have a really, really good reason.

That's still true.

 There are times it seems it would be 
 very natural to me to do so, but Georg made a strong case (I don't remember 
 all the details) for not doing so, and if that's how he designed the code, 
 then, well, I'm guessing there are reasons, and I'd probably find them out 
 if I violated his orders.

 I agree that there's something unhappy about the stringification, 
 unstringification, and so on and so forth. But, as JMarc often says, we 
 know we have to do that anyway---remember that the strings are in exactly 
 the form they are in the .lyx file---so it's not that unreasonable to use 
 the same infrastructure to pass things around.

Well, the existence of a road from Berlin to Paris does not necessarily
mean we have to take it when coming from London...

 That said, I also agree with 
 Abdel that we shouldn't bind ourselves to doing that if it makes things 
 overly complex, and I don't really care that much how things are passed 
 back and forth from the GUI. But that's really a separate issue from how 
 ICP works. You can build an ICP without all the stringification. You just 
 do it directly:
 params.setParam(bibfiles) = whatever;

I understand that. But all such parameters are bound to a specific Inset
instance, so why is this not

  InsetBibFoo::setBibFiles(whatever);  

?

 and then you can pass params itself to the inset if you like. Doing so has 
 a downside, though, one that JMarc (again) is always pointing out, namely, 
 that if you go via an LFUN, you get all kinds of updates, etc, for free; in 
 particular, going via the dispatch mechanism gets you this. But then you do 
 need a common representation of the parameters being passed, and a string 
 is a pretty natural choice. (I don't even want to think about all the 
 casting that would otherwise be required.) But the main point, really, is 
 that this is a different issue. The conversion cost question is really 
 separate from the question whether ICP is A Good Thing. By itself, getting 
 rid of ICP wouldn't help you there.

The conversion costs are not outrageous. I would be just a nice bonus.
I question the extra horizontal layer...

 Anyway, even if there were reasons to do a large scale re-write of this 
 stuff, I don't see anyone doing that any time real soon. So, unless someone 
 is going to do that, then, as things stand, I strongly suspect that there's 
 substantial duplication of code across InsetGraphicsParams, 
 InsetExternalParams, and InsetCommandParams. (There has to be.) Barring the 
 massive re-write, then, the strategy I suggested seems sensible. And for 
 the same reason, barring some large-scale change of direction, I'm 
 reluctant to introduce InsetBibtexParams and take InsetBibtex out of the 
 existing framework.

Right now we have (rough estimation) around 70-80 lines worth of code
handling the Parameter stuff per 'participating inset' (sum of per-inset
glue code and 1/14th or so share of the common infrastructure).

I am fairly sure that per-inset code using a few common helper function
would end up rather in the 20-30 lines range, and be more flexible in
the end as we do not have to stick to yet another framework.

Andre'


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Jean-Marc Lasgouttes
Bo Peng [EMAIL PROTECTED] writes:

 Could you elaborate? Is that a pure-text tar-like format? I think you
 are suggesting a tar-like format that is not compressed so that it can
 be svned. I do not know how binary files such as jpg would be
 represented in such a file.

No, a mac osx bundle is a directory with files inside it. It is seen
by the OS as a file, but it is not one. A pretty powerful concept,
actually (for example, LyX.app is a bundle containing all of our
files, but it seen as a simple application that is installed by
copying it somewhere).

JMarc


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Bo Peng
  Could you elaborate? Is that a pure-text tar-like format? I think you
   are suggesting a tar-like format that is not compressed so that it can
   be svned. I do not know how binary files such as jpg would be
   represented in such a file.

  No, a mac osx bundle is a directory with files inside it. It is seen
  by the OS as a file, but it is not one. A pretty powerful concept,
  actually (for example, LyX.app is a bundle containing all of our
  files, but it seen as a simple application that is installed by
  copying it somewhere).

How is this related to svn-ability? Basically, you want to be able to
svn a bunbled .lyx file so it would better be in pure-text format.
Then, it can be compressed when needed...

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Bo Peng
  I am fairly sure that per-inset code using a few common helper function
  would end up rather in the 20-30 lines range, and be more flexible in
  the end as we do not have to stick to yet another framework.

I would happy to see this as well. There will be no conversion between
params and EmbeddedFileList if we get rid of params altogether. Even
if removing InsetCommand is too costly now, free InsetBibtex from
InsetCommand would be less work.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Richard Heck

Andre Poenitz wrote:
That said, I also agree with 
Abdel that we shouldn't bind ourselves to doing that if it makes things 
overly complex, and I don't really care that much how things are passed 
back and forth from the GUI. But that's really a separate issue from how 
ICP works. You can build an ICP without all the stringification. You just 
do it directly:

params.setParam(bibfiles) = whatever;



I understand that. But all such parameters are bound to a specific Inset
instance, so why is this not

  InsetBibFoo::setBibFiles(whatever);  


?

  
Actually, the parameters need not be bound to a specific inset instance 
when they are created, and very often they are not. This is one of the 
things Georg most strongly insisted upon, and I expect it's really the 
core reason that ICP is separated out from the insets the way it is. In 
the GUI, for example, instances of InsetCommandParams are used to 
represent the state of the parameters *according to the dialog*, not 
according to the inset. So those are not bound to any inset (though the 
dialog may be). In short, an instance of ICP is the *kind* of thing that 
might be an inset's parameters, but it doesn't actually have to be an 
inset's parameters. Of course, there are sure to be way to work around 
this. You could create insets in the dialogs and use them to represent 
the parameters. But I don't expect you want to do quite that.


So if the idea was to roll ICP into the insets, well, I'm sure you could 
do that, but it would involve a lot of changes elsewhere.


Richard



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Andre Poenitz
On Wed, Apr 02, 2008 at 12:25:35PM -0400, Richard Heck wrote:
 Andre Poenitz wrote:
 I understand that. But all such parameters are bound to a specific Inset
 instance, so why is this not

   InsetBibFoo::setBibFiles(whatever);  
 ?
   
 Actually, the parameters need not be bound to a specific inset instance 
 when they are created, and very often they are not. This is one of the 
 things Georg most strongly insisted upon, and I expect it's really the core 
 reason that ICP is separated out from the insets the way it is. In the GUI, 
 for example, instances of InsetCommandParams are used to represent the 
 state of the parameters *according to the dialog*, not according to the 
 inset.

Note that the state of the dialog is _also_ duplicated in the individual
GUI elements, so there is no real strong reason to have yet another
copy hanging around.

In any case, this is not an argument for uniform inset parameters but
rather for an inset specific parameter structure...

Andre'


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Richard Heck

Andre Poenitz wrote:

On Wed, Apr 02, 2008 at 12:25:35PM -0400, Richard Heck wrote:
  

Andre Poenitz wrote:


I understand that. But all such parameters are bound to a specific Inset
instance, so why is this not

  InsetBibFoo::setBibFiles(whatever);  
?
  
  
Actually, the parameters need not be bound to a specific inset instance 
when they are created, and very often they are not. This is one of the 
things Georg most strongly insisted upon, and I expect it's really the core 
reason that ICP is separated out from the insets the way it is. In the GUI, 
for example, instances of InsetCommandParams are used to represent the 
state of the parameters *according to the dialog*, not according to the 
inset.



Note that the state of the dialog is _also_ duplicated in the individual
GUI elements, so there is no real strong reason to have yet another
copy hanging around.

  

It simplifies dispatch. And some other stuff.


In any case, this is not an argument for uniform inset parameters but
rather for an inset specific parameter structure...

  

Of course.

rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Andre Poenitz
On Tue, Apr 01, 2008 at 06:49:25PM -0400, rgheck wrote:
> Andre Poenitz wrote:
>> > We won't know this until we look at fixing that. I'm inclined, actually, 
>> to > do something more general, namely, to try to bring InsetGraphic into 
>> the > InsetCommand hierarchy. I think this was intended all along as part 
>> of a > more general transition that was never finished.
>>   
>> Could you please elaborate on why having the InsetCommand hierarchy is
>> useful at all?
>>
>> As far as I can tell the main benefit was to have a uniform means to
>> pass "Parameters" through the Controller layer in the Old Days. This
>> layer is gone now, and it feels a bit strange to pass around unrelated
>> pieces of data from otherwise unrelated insets in a common structure.
>> Wouldn't per-inset specific data with direct access from the GUI simplify
>> overall structure and reduce conversion costs?
>>   
>
> Frankly, I don't know the overall structure here well enough to be sure 
> exactly what the advantages or disadvantages to keeping the 
> InsetCommandParams business the way it is. Mostly, I try to go on how Georg 
> explained this to me some time ago, and the understanding I've developed 
> since then of why he (they?) did things the way they did.

You refered to this conversation lately and I had a look then. It
contained sound arguments for the time, with the Controller layer being
still around - which is gone now.

> In particular, 
> Georg insisted that we are not to subclass InsetCommandParams, not unless 
> we have a really, really good reason.

That's still true.

> There are times it seems it would be 
> very natural to me to do so, but Georg made a strong case (I don't remember 
> all the details) for not doing so, and if that's how he designed the code, 
> then, well, I'm guessing there are reasons, and I'd probably find them out 
> if I violated his orders.
>
> I agree that there's something unhappy about the stringification, 
> unstringification, and so on and so forth. But, as JMarc often says, we 
> know we have to do that anyway---remember that the strings are in exactly 
> the form they are in the .lyx file---so it's not that unreasonable to use 
> the same infrastructure to pass things around.

Well, the existence of a road from Berlin to Paris does not necessarily
mean we have to take it when coming from London...

> That said, I also agree with 
> Abdel that we shouldn't bind ourselves to doing that if it makes things 
> overly complex, and I don't really care that much how things are passed 
> back and forth from the GUI. But that's really a separate issue from how 
> ICP works. You can build an ICP without all the stringification. You just 
> do it directly:
> params.setParam("bibfiles") = whatever;

I understand that. But all such parameters are bound to a specific Inset
instance, so why is this not

  InsetBibFoo::setBibFiles(whatever);  

?

> and then you can pass params itself to the inset if you like. Doing so has 
> a downside, though, one that JMarc (again) is always pointing out, namely, 
> that if you go via an LFUN, you get all kinds of updates, etc, for free; in 
> particular, going via the dispatch mechanism gets you this. But then you do 
> need a common representation of the parameters being passed, and a string 
> is a pretty natural choice. (I don't even want to think about all the 
> casting that would otherwise be required.) But the main point, really, is 
> that this is a different issue. The conversion cost question is really 
> separate from the question whether ICP is A Good Thing. By itself, getting 
> rid of ICP wouldn't help you there.

The conversion costs are not outrageous. I would be just a nice bonus.
I question the extra horizontal layer...

> Anyway, even if there were reasons to do a large scale re-write of this 
> stuff, I don't see anyone doing that any time real soon. So, unless someone 
> is going to do that, then, as things stand, I strongly suspect that there's 
> substantial duplication of code across InsetGraphicsParams, 
> InsetExternalParams, and InsetCommandParams. (There has to be.) Barring the 
> massive re-write, then, the strategy I suggested seems sensible. And for 
> the same reason, barring some large-scale change of direction, I'm 
> reluctant to introduce InsetBibtexParams and take InsetBibtex out of the 
> existing framework.

Right now we have (rough estimation) around 70-80 lines worth of code
handling the Parameter stuff per 'participating inset' (sum of per-inset
glue code and 1/14th or so share of the common infrastructure).

I am fairly sure that per-inset code using a few common helper function
would end up rather in the 20-30 lines range, and be more flexible in
the end as we do not have to stick to yet another framework.

Andre'


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Jean-Marc Lasgouttes
"Bo Peng" <[EMAIL PROTECTED]> writes:

> Could you elaborate? Is that a pure-text tar-like format? I think you
> are suggesting a tar-like format that is not compressed so that it can
> be svned. I do not know how binary files such as jpg would be
> represented in such a file.

No, a mac osx bundle is a directory with files inside it. It is seen
by the OS as a file, but it is not one. A pretty powerful concept,
actually (for example, LyX.app is a bundle containing all of our
files, but it seen as a simple application that is installed by
copying it somewhere).

JMarc


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Bo Peng
> > Could you elaborate? Is that a pure-text tar-like format? I think you
>  > are suggesting a tar-like format that is not compressed so that it can
>  > be svned. I do not know how binary files such as jpg would be
>  > represented in such a file.
>
>  No, a mac osx bundle is a directory with files inside it. It is seen
>  by the OS as a file, but it is not one. A pretty powerful concept,
>  actually (for example, LyX.app is a bundle containing all of our
>  files, but it seen as a simple application that is installed by
>  copying it somewhere).

How is this related to svn-ability? Basically, you want to be able to
svn a bunbled .lyx file so it would better be in pure-text format.
Then, it can be compressed when needed...

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Bo Peng
>  I am fairly sure that per-inset code using a few common helper function
>  would end up rather in the 20-30 lines range, and be more flexible in
>  the end as we do not have to stick to yet another framework.

I would happy to see this as well. There will be no conversion between
params and EmbeddedFileList if we get rid of params altogether. Even
if removing InsetCommand is too costly now, free InsetBibtex from
InsetCommand would be less work.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Richard Heck

Andre Poenitz wrote:
That said, I also agree with 
Abdel that we shouldn't bind ourselves to doing that if it makes things 
overly complex, and I don't really care that much how things are passed 
back and forth from the GUI. But that's really a separate issue from how 
ICP works. You can build an ICP without all the stringification. You just 
do it directly:

params.setParam("bibfiles") = whatever;



I understand that. But all such parameters are bound to a specific Inset
instance, so why is this not

  InsetBibFoo::setBibFiles(whatever);  


?

  
Actually, the parameters need not be bound to a specific inset instance 
when they are created, and very often they are not. This is one of the 
things Georg most strongly insisted upon, and I expect it's really the 
core reason that ICP is separated out from the insets the way it is. In 
the GUI, for example, instances of InsetCommandParams are used to 
represent the state of the parameters *according to the dialog*, not 
according to the inset. So those are not bound to any inset (though the 
dialog may be). In short, an instance of ICP is the *kind* of thing that 
might be an inset's parameters, but it doesn't actually have to be an 
inset's parameters. Of course, there are sure to be way to work around 
this. You could create insets in the dialogs and use them to represent 
the parameters. But I don't expect you want to do quite that.


So if the idea was to roll ICP into the insets, well, I'm sure you could 
do that, but it would involve a lot of changes elsewhere.


Richard



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Andre Poenitz
On Wed, Apr 02, 2008 at 12:25:35PM -0400, Richard Heck wrote:
> Andre Poenitz wrote:
>> I understand that. But all such parameters are bound to a specific Inset
>> instance, so why is this not
>>
>>   InsetBibFoo::setBibFiles(whatever);  
>> ?
>   
> Actually, the parameters need not be bound to a specific inset instance 
> when they are created, and very often they are not. This is one of the 
> things Georg most strongly insisted upon, and I expect it's really the core 
> reason that ICP is separated out from the insets the way it is. In the GUI, 
> for example, instances of InsetCommandParams are used to represent the 
> state of the parameters *according to the dialog*, not according to the 
> inset.

Note that the state of the dialog is _also_ duplicated in the individual
GUI elements, so there is no real strong reason to have yet another
copy hanging around.

In any case, this is not an argument for uniform inset parameters but
rather for an inset specific parameter structure...

Andre'


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-02 Thread Richard Heck

Andre Poenitz wrote:

On Wed, Apr 02, 2008 at 12:25:35PM -0400, Richard Heck wrote:
  

Andre Poenitz wrote:


I understand that. But all such parameters are bound to a specific Inset
instance, so why is this not

  InsetBibFoo::setBibFiles(whatever);  
?
  
  
Actually, the parameters need not be bound to a specific inset instance 
when they are created, and very often they are not. This is one of the 
things Georg most strongly insisted upon, and I expect it's really the core 
reason that ICP is separated out from the insets the way it is. In the GUI, 
for example, instances of InsetCommandParams are used to represent the 
state of the parameters *according to the dialog*, not according to the 
inset.



Note that the state of the dialog is _also_ duplicated in the individual
GUI elements, so there is no real strong reason to have yet another
copy hanging around.

  

It simplifies dispatch. And some other stuff.


In any case, this is not an argument for uniform inset parameters but
rather for an inset specific parameter structure...

  

Of course.

rh



Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:

 Yes, I understand that we need to look up that information. But what I'm
 saying is that I want to use the EmbeddedFileList, which we're keeping
 in sync with the params, ONLY to look that information up if and when
 it's needed, and not use it in other ways. Yes, this is a minor dispute
 now. But it matters, it seems to me, for the other reasons I've
 mentioned, viz: If the embedding stuff changes, I want the change only
 to affect things that have to do with embedding.



snip Your example /snip

I can not believe that we spent so much time on this small issue. Of
course when we use another reprentation to params, we would better be
able to reproduce params. The meta_ patch will do exactly this, and if
someone would like to use a EmbeddedFileMap, the same issue should be
considered. 

  
I wasn't trying to argue. I was trying to collaborate. But you are so 
protective of your code that you won't even let me fix the bugs YOU created.


And you're missing the point of the example. If you use an 
EmbeddedFileMap, you can't recover the order of the parameters. That's 
why the LaTeX output would change. Of course, you could add some 
auxiliary data structure that recorded this information, but that would 
be ugly. And why should you need to do that? There's nothing about what 
the EmbeddedFileList represents that has anything to do with order.


The example wasn't idle. Using a map in InsetBibtex is totally natural: 
a map from parameter representations, like biblio, to the EmbeddedFile 
objects that represent them; then we get easy lookup of one from the 
other. There's no reason to use an EmbeddedFileList. We don't use any of 
the methods of EmbeddedFileList except the ones that we get from 
std::vector (and the findFile() method, which ought just to be a wrapper 
around find_if). So we're not using the EmbeddedFileList at all, and we 
might as well just use vectorEmbeddedFile. But if so, why can't we use 
a mapdocstring, EmbeddedFile? (This also has non-trivial advantages 
back in Buffer.cpp.) But we can't do that if we're iterating over the 
vector to access the parameters. So we shouldn't iterate over the 
vector. In short, and as I've said before: The basic operations of the 
inset should not depend upon this kind of stuff.


I don't see why any of that should be remotely controversial. It's 
Programming 101 stuff.


Can someone step in here please and resolve this? If I'm wrong, please 
feel free to tell me.


Richard



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
  I wasn't trying to argue. I was trying to collaborate. But you are so
  protective of your code that you won't even let me fix the bugs YOU created.

protective?? right. Note that your patch came before you understood
how embedding works.

  The example wasn't idle. Using a map in InsetBibtex is totally natural:

Come on, if you use a map, you open a file with embedded files and
save it. The embedded files may be saved in a different order and
result in a totally different .lyx file. It is lucky that you are not
supposed to svn an embedded .lyx file.

  Can someone step in here please and resolve this? If I'm wrong, please
  feel free to tell me.

Please.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Jean-Marc Lasgouttes
Bo Peng [EMAIL PROTECTED] writes:

 It is lucky that you are not supposed to svn an embedded .lyx file.

Actually, it is rather a shortcoming of the implementation. If we
allowed for a osx bundle-like representation (which would just have to
be zipped to be a proper embedded file), this could go seemlessly into
an svn archive. I understand this may not be doable immediately, but
it is something that would make much sense.

JMarc


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
  Actually, it is rather a shortcoming of the implementation. If we
  allowed for a osx bundle-like representation (which would just have to
  be zipped to be a proper embedded file), this could go seemlessly into
  an svn archive. I understand this may not be doable immediately, but
  it is something that would make much sense.

Could you elaborate? Is that a pure-text tar-like format? I think you
are suggesting a tar-like format that is not compressed so that it can
be svned. I do not know how binary files such as jpg would be
represented in such a file.

We are not even at alpha stage so this can be changed.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Jean-Marc Lasgouttes wrote:

Bo Peng [EMAIL PROTECTED] writes:

  

It is lucky that you are not supposed to svn an embedded .lyx file.



Actually, it is rather a shortcoming of the implementation. 

  
As I say elsewhere, the current implementation can guarantee that the 
order of the embedded files remains the same after minor changes.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:

 I wasn't trying to argue. I was trying to collaborate. But you are so
 protective of your code that you won't even let me fix the bugs YOU created.



protective?? right. Note that your patch came before you understood
how embedding works.

  
That was several patches ago. God forbid I might have had trouble 
understanding your code.



 The example wasn't idle. Using a map in InsetBibtex is totally natural:



Come on, if you use a map, you open a file with embedded files and
save it. The embedded files may be saved in a different order and
result in a totally different .lyx file. It is lucky that you are not
supposed to svn an embedded .lyx file.

  
You STILL don't understand the point. This isn't about 
Buffer::embeddedFile(), which is what is used to save the zip file. It's 
about the bibfilesCache_, which is a totally separate thing.


Look, I want to make bibfilesCache_ a map. I have very good reasons to 
want to do this. I therefore want InsetBibtex::getBibFilesCache() to 
return such a map. (I suppose it could return something else, and then I 
could do some conversion, but why?) Back in InsetBibtex, I can produce 
the map from an EmbeddedFileList, but the much easier thing to do is to 
have InsetBibtex::bibfiles_ itself be such a map, and I'll just return a 
const reference to it---just like we do now. If your implementation of 
the embedding feature prohibits me from doing this, then that, it seems 
to me, is a big problem. Do you think it does? If not, is it OK with you 
if I make this change?


And second: If the order of the embedded files in the LyX file matters, 
then you'd better check the code, because it simply does not guarantee 
that the order will not change after minor edits. It is also false that 
it might change after an open/save sequence, unless you've moved 
systems. But in that case, it may change anyway.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
  You STILL don't understand the point. This isn't about
  Buffer::embeddedFile(), which is what is used to save the zip file. It's
  about the bibfilesCache_, which is a totally separate thing.

I though that you are changing EmbeddedFileList from
vectorEmbeddedFile to mapdocstring, EmbeddedFile. This will of
course change buffer level embedded file list.

  Look, I want to make bibfilesCache_ a map. I have very good reasons to
  want to do this. I therefore want InsetBibtex::getBibFilesCache() to
  return such a map. (I suppose it could return something else, and then I
  could do some conversion, but why?) Back in InsetBibtex, I can produce
  the map from an EmbeddedFileList, but the much easier thing to do is to
  have InsetBibtex::bibfiles_ itself be such a map, and I'll just return a
  const reference to it---just like we do now. If your implementation of
  the embedding feature prohibits me from doing this, then that, it seems
  to me, is a big problem. Do you think it does? If not, is it OK with you
  if I make this change?

What is the key to this map? If it is biblio, not /path/to/biblio.bib,
then you can make bibfiles_ a map, and the meta_ patch is no longer
needed. A complication here is that registerEmbeddedFile has to ask
params for the order of the files this is in the end not so good.

  And second: If the order of the embedded files in the LyX file matters,
  then you'd better check the code, because it simply does not guarantee
  that the order will not change after minor edits. It is also false that
  it might change after an open/save sequence, unless you've moved
  systems. But in that case, it may change anyway.

The current code will not change the order of embedded files, unless
you use a map to return bib files without order from InsetBibtex.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:



 Look, I want to make bibfilesCache_ a map. I have very good reasons to
 want to do this. I therefore want InsetBibtex::getBibFilesCache() to
 return such a map. (I suppose it could return something else, and then I
 could do some conversion, but why?) Back in InsetBibtex, I can produce
 the map from an EmbeddedFileList, but the much easier thing to do is to
 have InsetBibtex::bibfiles_ itself be such a map, and I'll just return a
 const reference to it---just like we do now. If your implementation of
 the embedding feature prohibits me from doing this, then that, it seems
 to me, is a big problem. Do you think it does? If not, is it OK with you
 if I make this change?



What is the key to this map? If it is biblio, not /path/to/biblio.bib,
then you can make bibfiles_ a map, and the meta_ patch is no longer
needed. 

  
Yes. This is what I've been trying to say, more or less. But note that, 
when we output latex, we iterate not over bibfiles_ but over params, and 
we (quickly) look up the embedded file info in the map. We do it this 
way because we should keep the order. This requires that bibfiles_ and 
the params be in sync, but they have to be kept in sync anyway. We 
resolved that issue a long time ago.



A complication here is that registerEmbeddedFile has to ask
params for the order of the files this is in the end not so good.

  
Why do they need to be saved in the same order as the parameters? If the 
concern is that the order should stay the same from save to save, then 
this is a non-issue: It will, because the order of the map will not 
change unless the keys do. This will be so even across systems and 
platforms.

 And second: If the order of the embedded files in the LyX file matters,
 then you'd better check the code, because it simply does not guarantee
 that the order will not change after minor edits. It is also false that
 it might change after an open/save sequence, unless you've moved
 systems. But in that case, it may change anyway.



The current code will not change the order of embedded files, unless
you use a map to return bib files without order from InsetBibtex.

  
Yes, it will. Not just from open/save---I didn't say that---but it will 
change the order, say, if you change the order of the files in the 
bibliography inset, or if you move one graphic in front of another. This 
is because of how the files are registered. If it matters that the order 
stay the same, then you need to sort the list in 
EmbeddedFileList::writeFile(), or probably better in update(). But if 
you do that, then it doesn't matter what the reported order is, and it 
doesn't matter if we use a map.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
   What is the key to this map? If it is biblio, not /path/to/biblio.bib,
   then you can make bibfiles_ a map, and the meta_ patch is no longer
   needed.
  
  Yes. This is what I've been trying to say, more or less. But note that,
  when we output latex, we iterate not over bibfiles_ but over params, and
  we (quickly) look up the embedded file info in the map. We do it this
  way because we should keep the order. This requires that bibfiles_ and
  the params be in sync, but they have to be kept in sync anyway. We
  resolved that issue a long time ago.

OK. We have agree upon at least a few things,

1. EmbeddedFileList (or map) has to be in sync with params, has to be
valid, so it can not be separated from params. In this sense, who
should be the core information (so that another one does not have to
be updated) is a false problem.

2. EmbeddedFile needs this meta_, and your solution of mapmeta_,
EmbeddedFile is acceptable to me. However, meta_ is a solution to a
general problem, that may be needed anyway in, for example,
InsetGraphics. I am not against the idea that you fix InsetBibtex now,
and fix InsetGraphic later.

There are then some minor problems left, such as 1. In functions such
as addDatabase, should we update params first and update
EmbeddedFileList/Map; or another way around? 2. When we are in need
some information, from whom should we ask for it? I say they are minor
problems because if params and EmbeddedFiles are in sync, it does not
really matter, so let use just use the easier method. I tend to use
EmbeddedFileList because it is structured so it is easier to handle.

   The current code will not change the order of embedded files, unless
   you use a map to return bib files without order from InsetBibtex.
  
  
  Yes, it will. Not just from open/save---I didn't say that---but it will
  change the order, say, if you change the order of the files in the
  bibliography inset, or if you move one graphic in front of another. This
  is because of how the files are registered. If it matters that the order
  stay the same, then you need to sort the list in
  EmbeddedFileList::writeFile(), or probably better in update(). But if
  you do that, then it doesn't matter what the reported order is, and it
  doesn't matter if we use a map.

I think sorting the embedded files before writing is a great idea. At
least it no longer matters if you use map..., embeddedfile in
InsetBibtex::registerFile.

Cheers,
Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:

  What is the key to this map? If it is biblio, not /path/to/biblio.bib,
  then you can make bibfiles_ a map, and the meta_ patch is no longer
  needed.
 
 Yes. This is what I've been trying to say, more or less. But note that,
 when we output latex, we iterate not over bibfiles_ but over params, and
 we (quickly) look up the embedded file info in the map. We do it this
 way because we should keep the order. This requires that bibfiles_ and
 the params be in sync, but they have to be kept in sync anyway. We
 resolved that issue a long time ago.



OK. We have agree upon at least a few things,

1. EmbeddedFileList (or map) has to be in sync with params, has to be
valid, so it can not be separated from params. In this sense, who
should be the core information (so that another one does not have to
be updated) is a false problem.

2. EmbeddedFile needs this meta_, and your solution of mapmeta_,
EmbeddedFile is acceptable to me. However, meta_ is a solution to a
general problem, that may be needed anyway in, for example,
InsetGraphics. I am not against the idea that you fix InsetBibtex now,
and fix InsetGraphic later.

  
We won't know this until we look at fixing that. I'm inclined, actually, 
to do something more general, namely, to try to bring InsetGraphic into 
the InsetCommand hierarchy. I think this was intended all along as part 
of a more general transition that was never finished.



There are then some minor problems left, such as 1. In functions such
as addDatabase, should we update params first and update
EmbeddedFileList/Map; or another way around? 
  
or encapsulate it in overridden setParam methods, so you can't forget to 
do it.



2. When we are in need
some information, from whom should we ask for it? I say they are minor
problems because if params and EmbeddedFiles are in sync, it does not
really matter, so let use just use the easier method. 

  
Iterating over params is more sensible, I think, if only because, if 
embedding is not enabled, you needn't consult anything else. Exactly 
what the code will look like, I don't yet know, but we'll have things like:

   string const database =
   buffer().enabled() ? consult(paramIterator-first) : 
paramIterator-first;

We'd anyway have (more or less)
   string const database =
   buffer().enabled() ? bibfilesIterator-availableFilename() : 
bibfilesIterator-metaInfo();
So it doesn't end up mattering that much, and the former makes it 
clearer exactly where the information from EmbeddedFiles is used and 
where it is not.


Anyway, it sounds as if we're well enough agreed that I can move ahead 
with this.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
   There are then some minor problems left, such as 1. In functions such
   as addDatabase, should we update params first and update
   EmbeddedFileList/Map; or another way around?
  
  or encapsulate it in overridden setParam methods, so you can't forget to
  do it.

I disliked your implementation of setParam because if you set params
first, and then update EmbeddedFileList, you may be bounced back to
set params again (to disable embedding). On the other hand, if you
update EmbeddedFile first, you do not have to do this. Anyway, all
these can happen in setParam and the differences are trivial.

   2. When we are in need
   some information, from whom should we ask for it? I say they are minor
   problems because if params and EmbeddedFiles are in sync, it does not
   really matter, so let use just use the easier method.
  
  
  Iterating over params is more sensible, I think, if only because, if
  embedding is not enabled, you needn't consult anything else. Exactly
  what the code will look like, I don't yet know, but we'll have things like:
 string const database =
 buffer().enabled() ? consult(paramIterator-first) :
  paramIterator-first;
  We'd anyway have (more or less)
 string const database =
 buffer().enabled() ? bibfilesIterator-availableFilename() :
  bibfilesIterator-metaInfo();
  So it doesn't end up mattering that much, and the former makes it
  clearer exactly where the information from EmbeddedFiles is used and
  where it is not.

I will have to see your patch (or commits) to determine if I like it,
but you know I dislike parsing params again and again. :-)

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Andre Poenitz
On Tue, Apr 01, 2008 at 05:36:20PM -0400, rgheck wrote:
 Bo Peng wrote:
   What is the key to this map? If it is biblio, not /path/to/biblio.bib,
   then you can make bibfiles_ a map, and the meta_ patch is no longer
   needed.
  
  Yes. This is what I've been trying to say, more or less. But note that,
  when we output latex, we iterate not over bibfiles_ but over params, and
  we (quickly) look up the embedded file info in the map. We do it this
  way because we should keep the order. This requires that bibfiles_ and
  the params be in sync, but they have to be kept in sync anyway. We
  resolved that issue a long time ago.
 

 OK. We have agree upon at least a few things,

 1. EmbeddedFileList (or map) has to be in sync with params, has to be
 valid, so it can not be separated from params. In this sense, who
 should be the core information (so that another one does not have to
 be updated) is a false problem.

 2. EmbeddedFile needs this meta_, and your solution of mapmeta_,
 EmbeddedFile is acceptable to me. However, meta_ is a solution to a
 general problem, that may be needed anyway in, for example,
 InsetGraphics. I am not against the idea that you fix InsetBibtex now,
 and fix InsetGraphic later.

   
 We won't know this until we look at fixing that. I'm inclined, actually, to 
 do something more general, namely, to try to bring InsetGraphic into the 
 InsetCommand hierarchy. I think this was intended all along as part of a 
 more general transition that was never finished.

Could you please elaborate on why having the InsetCommand hierarchy is
useful at all?

As far as I can tell the main benefit was to have a uniform means to
pass Parameters through the Controller layer in the Old Days. This
layer is gone now, and it feels a bit strange to pass around unrelated
pieces of data from otherwise unrelated insets in a common structure.
Wouldn't per-inset specific data with direct access from the GUI simplify
overall structure and reduce conversion costs?

[Note that I did not follow GUI development too closely then, so I might
easily miss something very important here.]

Andre'


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Andre Poenitz wrote:
 We won't know this until we look at fixing that. I'm inclined, actually, to 
 do something more general, namely, to try to bring InsetGraphic into the 
 InsetCommand hierarchy. I think this was intended all along as part of a 
 more general transition that was never finished.
  


Could you please elaborate on why having the InsetCommand hierarchy is
useful at all?

As far as I can tell the main benefit was to have a uniform means to
pass Parameters through the Controller layer in the Old Days. This
layer is gone now, and it feels a bit strange to pass around unrelated
pieces of data from otherwise unrelated insets in a common structure.
Wouldn't per-inset specific data with direct access from the GUI simplify
overall structure and reduce conversion costs?
  


Frankly, I don't know the overall structure here well enough to be sure 
exactly what the advantages or disadvantages to keeping the 
InsetCommandParams business the way it is. Mostly, I try to go on how 
Georg explained this to me some time ago, and the understanding I've 
developed since then of why he (they?) did things the way they did. In 
particular, Georg insisted that we are not to subclass 
InsetCommandParams, not unless we have a really, really good reason. 
There are times it seems it would be very natural to me to do so, but 
Georg made a strong case (I don't remember all the details) for not 
doing so, and if that's how he designed the code, then, well, I'm 
guessing there are reasons, and I'd probably find them out if I violated 
his orders.


I agree that there's something unhappy about the stringification, 
unstringification, and so on and so forth. But, as JMarc often says, we 
know we have to do that anyway---remember that the strings are in 
exactly the form they are in the .lyx file---so it's not that 
unreasonable to use the same infrastructure to pass things around. That 
said, I also agree with Abdel that we shouldn't bind ourselves to doing 
that if it makes things overly complex, and I don't really care that 
much how things are passed back and forth from the GUI. But that's 
really a separate issue from how ICP works. You can build an ICP without 
all the stringification. You just do it directly:

params.setParam(bibfiles) = whatever;
and then you can pass params itself to the inset if you like. Doing so 
has a downside, though, one that JMarc (again) is always pointing out, 
namely, that if you go via an LFUN, you get all kinds of updates, etc, 
for free; in particular, going via the dispatch mechanism gets you this. 
But then you do need a common representation of the parameters being 
passed, and a string is a pretty natural choice. (I don't even want to 
think about all the casting that would otherwise be required.) But the 
main point, really, is that this is a different issue. The conversion 
cost question is really separate from the question whether ICP is A Good 
Thing. By itself, getting rid of ICP wouldn't help you there.


Anyway, even if there were reasons to do a large scale re-write of this 
stuff, I don't see anyone doing that any time real soon. So, unless 
someone is going to do that, then, as things stand, I strongly suspect 
that there's substantial duplication of code across InsetGraphicsParams, 
InsetExternalParams, and InsetCommandParams. (There has to be.) Barring 
the massive re-write, then, the strategy I suggested seems sensible. And 
for the same reason, barring some large-scale change of direction, I'm 
reluctant to introduce InsetBibtexParams and take InsetBibtex out of the 
existing framework.


Hope that helps.

Richard



Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:

 Yes, I understand that we need to look up that information. But what I'm
 saying is that I want to use the EmbeddedFileList, which we're keeping
 in sync with the params, ONLY to look that information up if and when
 it's needed, and not use it in other ways. Yes, this is a minor dispute
 now. But it matters, it seems to me, for the other reasons I've
 mentioned, viz: If the embedding stuff changes, I want the change only
 to affect things that have to do with embedding.



 Your example 

I can not believe that we spent so much time on this small issue. Of
course when we use another reprentation to params, we would better be
able to reproduce params. The meta_ patch will do exactly this, and if
someone would like to use a EmbeddedFileMap, the same issue should be
considered. 

  
I wasn't trying to argue. I was trying to collaborate. But you are so 
protective of your code that you won't even let me fix the bugs YOU created.


And you're missing the point of the example. If you use an 
EmbeddedFileMap, you can't recover the order of the parameters. That's 
why the LaTeX output would change. Of course, you could add some 
auxiliary data structure that recorded this information, but that would 
be ugly. And why should you need to do that? There's nothing about what 
the EmbeddedFileList represents that has anything to do with order.


The example wasn't idle. Using a map in InsetBibtex is totally natural: 
a map from parameter representations, like "biblio", to the EmbeddedFile 
objects that represent them; then we get easy lookup of one from the 
other. There's no reason to use an EmbeddedFileList. We don't use any of 
the methods of EmbeddedFileList except the ones that we get from 
std::vector (and the findFile() method, which ought just to be a wrapper 
around find_if). So we're not using the EmbeddedFileList at all, and we 
might as well just use vector. But if so, why can't we use 
a map? (This also has non-trivial advantages 
back in Buffer.cpp.) But we can't do that if we're iterating over the 
vector to access the parameters. So we shouldn't iterate over the 
vector. In short, and as I've said before: The basic operations of the 
inset should not depend upon this kind of stuff.


I don't see why any of that should be remotely controversial. It's 
Programming 101 stuff.


Can someone step in here please and resolve this? If I'm wrong, please 
feel free to tell me.


Richard



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
>  I wasn't trying to argue. I was trying to collaborate. But you are so
>  protective of your code that you won't even let me fix the bugs YOU created.

protective?? right. Note that your patch came before you understood
how embedding works.

>  The example wasn't idle. Using a map in InsetBibtex is totally natural:

Come on, if you use a map, you open a file with embedded files and
save it. The embedded files may be saved in a different order and
result in a totally different .lyx file. It is lucky that you are not
supposed to svn an embedded .lyx file.

>  Can someone step in here please and resolve this? If I'm wrong, please
>  feel free to tell me.

Please.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Jean-Marc Lasgouttes
"Bo Peng" <[EMAIL PROTECTED]> writes:

> It is lucky that you are not supposed to svn an embedded .lyx file.

Actually, it is rather a shortcoming of the implementation. If we
allowed for a osx bundle-like representation (which would just have to
be zipped to be a proper embedded file), this could go seemlessly into
an svn archive. I understand this may not be doable immediately, but
it is something that would make much sense.

JMarc


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
>  Actually, it is rather a shortcoming of the implementation. If we
>  allowed for a osx bundle-like representation (which would just have to
>  be zipped to be a proper embedded file), this could go seemlessly into
>  an svn archive. I understand this may not be doable immediately, but
>  it is something that would make much sense.

Could you elaborate? Is that a pure-text tar-like format? I think you
are suggesting a tar-like format that is not compressed so that it can
be svned. I do not know how binary files such as jpg would be
represented in such a file.

We are not even at alpha stage so this can be changed.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Jean-Marc Lasgouttes wrote:

"Bo Peng" <[EMAIL PROTECTED]> writes:

  

It is lucky that you are not supposed to svn an embedded .lyx file.



Actually, it is rather a shortcoming of the implementation. 

  
As I say elsewhere, the current implementation can guarantee that the 
order of the embedded files remains the same after minor changes.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:

 I wasn't trying to argue. I was trying to collaborate. But you are so
 protective of your code that you won't even let me fix the bugs YOU created.



protective?? right. Note that your patch came before you understood
how embedding works.

  
That was several patches ago. God forbid I might have had trouble 
understanding your code.



 The example wasn't idle. Using a map in InsetBibtex is totally natural:



Come on, if you use a map, you open a file with embedded files and
save it. The embedded files may be saved in a different order and
result in a totally different .lyx file. It is lucky that you are not
supposed to svn an embedded .lyx file.

  
You STILL don't understand the point. This isn't about 
Buffer::embeddedFile(), which is what is used to save the zip file. It's 
about the bibfilesCache_, which is a totally separate thing.


Look, I want to make bibfilesCache_ a map. I have very good reasons to 
want to do this. I therefore want InsetBibtex::getBibFilesCache() to 
return such a map. (I suppose it could return something else, and then I 
could do some conversion, but why?) Back in InsetBibtex, I can produce 
the map from an EmbeddedFileList, but the much easier thing to do is to 
have InsetBibtex::bibfiles_ itself be such a map, and I'll just return a 
const reference to it---just like we do now. If your implementation of 
the embedding feature prohibits me from doing this, then that, it seems 
to me, is a big problem. Do you think it does? If not, is it OK with you 
if I make this change?


And second: If the order of the embedded files in the LyX file matters, 
then you'd better check the code, because it simply does not guarantee 
that the order will not change after minor edits. It is also false that 
it might change after an open/save sequence, unless you've moved 
systems. But in that case, it may change anyway.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
>  You STILL don't understand the point. This isn't about
>  Buffer::embeddedFile(), which is what is used to save the zip file. It's
>  about the bibfilesCache_, which is a totally separate thing.

I though that you are changing EmbeddedFileList from
vector to map. This will of
course change buffer level embedded file list.

>  Look, I want to make bibfilesCache_ a map. I have very good reasons to
>  want to do this. I therefore want InsetBibtex::getBibFilesCache() to
>  return such a map. (I suppose it could return something else, and then I
>  could do some conversion, but why?) Back in InsetBibtex, I can produce
>  the map from an EmbeddedFileList, but the much easier thing to do is to
>  have InsetBibtex::bibfiles_ itself be such a map, and I'll just return a
>  const reference to it---just like we do now. If your implementation of
>  the embedding feature prohibits me from doing this, then that, it seems
>  to me, is a big problem. Do you think it does? If not, is it OK with you
>  if I make this change?

What is the key to this map? If it is biblio, not /path/to/biblio.bib,
then you can make bibfiles_ a map, and the meta_ patch is no longer
needed. A complication here is that registerEmbeddedFile has to ask
params for the order of the files this is in the end not so good.

>  And second: If the order of the embedded files in the LyX file matters,
>  then you'd better check the code, because it simply does not guarantee
>  that the order will not change after minor edits. It is also false that
>  it might change after an open/save sequence, unless you've moved
>  systems. But in that case, it may change anyway.

The current code will not change the order of embedded files, unless
you use a map to return bib files without order from InsetBibtex.

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:



 Look, I want to make bibfilesCache_ a map. I have very good reasons to
 want to do this. I therefore want InsetBibtex::getBibFilesCache() to
 return such a map. (I suppose it could return something else, and then I
 could do some conversion, but why?) Back in InsetBibtex, I can produce
 the map from an EmbeddedFileList, but the much easier thing to do is to
 have InsetBibtex::bibfiles_ itself be such a map, and I'll just return a
 const reference to it---just like we do now. If your implementation of
 the embedding feature prohibits me from doing this, then that, it seems
 to me, is a big problem. Do you think it does? If not, is it OK with you
 if I make this change?



What is the key to this map? If it is biblio, not /path/to/biblio.bib,
then you can make bibfiles_ a map, and the meta_ patch is no longer
needed. 

  
Yes. This is what I've been trying to say, more or less. But note that, 
when we output latex, we iterate not over bibfiles_ but over params, and 
we (quickly) look up the embedded file info in the map. We do it this 
way because we should keep the order. This requires that bibfiles_ and 
the params be in sync, but they have to be kept in sync anyway. We 
resolved that issue a long time ago.



A complication here is that registerEmbeddedFile has to ask
params for the order of the files this is in the end not so good.

  
Why do they need to be saved in the same order as the parameters? If the 
concern is that the order should stay the same from save to save, then 
this is a non-issue: It will, because the order of the map will not 
change unless the keys do. This will be so even across systems and 
platforms.

 And second: If the order of the embedded files in the LyX file matters,
 then you'd better check the code, because it simply does not guarantee
 that the order will not change after minor edits. It is also false that
 it might change after an open/save sequence, unless you've moved
 systems. But in that case, it may change anyway.



The current code will not change the order of embedded files, unless
you use a map to return bib files without order from InsetBibtex.

  
Yes, it will. Not just from open/save---I didn't say that---but it will 
change the order, say, if you change the order of the files in the 
bibliography inset, or if you move one graphic in front of another. This 
is because of how the files are registered. If it matters that the order 
stay the same, then you need to sort the list in 
EmbeddedFileList::writeFile(), or probably better in update(). But if 
you do that, then it doesn't matter what the reported order is, and it 
doesn't matter if we use a map.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
>  > What is the key to this map? If it is biblio, not /path/to/biblio.bib,
>  > then you can make bibfiles_ a map, and the meta_ patch is no longer
>  > needed.
>  >
>  Yes. This is what I've been trying to say, more or less. But note that,
>  when we output latex, we iterate not over bibfiles_ but over params, and
>  we (quickly) look up the embedded file info in the map. We do it this
>  way because we should keep the order. This requires that bibfiles_ and
>  the params be in sync, but they have to be kept in sync anyway. We
>  resolved that issue a long time ago.

OK. We have agree upon at least a few things,

1. EmbeddedFileList (or map) has to be in sync with params, has to be
valid, so it can not be separated from params. In this sense, who
should be the core information (so that another one does not have to
be updated) is a false problem.

2. EmbeddedFile needs this meta_, and your solution of map is acceptable to me. However, meta_ is a solution to a
general problem, that may be needed anyway in, for example,
InsetGraphics. I am not against the idea that you fix InsetBibtex now,
and fix InsetGraphic later.

There are then some minor problems left, such as 1. In functions such
as addDatabase, should we update params first and update
EmbeddedFileList/Map; or another way around? 2. When we are in need
some information, from whom should we ask for it? I say they are minor
problems because if params and EmbeddedFiles are in sync, it does not
really matter, so let use just use the easier method. I tend to use
EmbeddedFileList because it is structured so it is easier to handle.

>  > The current code will not change the order of embedded files, unless
>  > you use a map to return bib files without order from InsetBibtex.
>  >
>  >
>  Yes, it will. Not just from open/save---I didn't say that---but it will
>  change the order, say, if you change the order of the files in the
>  bibliography inset, or if you move one graphic in front of another. This
>  is because of how the files are registered. If it matters that the order
>  stay the same, then you need to sort the list in
>  EmbeddedFileList::writeFile(), or probably better in update(). But if
>  you do that, then it doesn't matter what the reported order is, and it
>  doesn't matter if we use a map.

I think sorting the embedded files before writing is a great idea. At
least it no longer matters if you use map<..., embeddedfile> in
InsetBibtex::registerFile.

Cheers,
Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Bo Peng wrote:

 > What is the key to this map? If it is biblio, not /path/to/biblio.bib,
 > then you can make bibfiles_ a map, and the meta_ patch is no longer
 > needed.
 >
 Yes. This is what I've been trying to say, more or less. But note that,
 when we output latex, we iterate not over bibfiles_ but over params, and
 we (quickly) look up the embedded file info in the map. We do it this
 way because we should keep the order. This requires that bibfiles_ and
 the params be in sync, but they have to be kept in sync anyway. We
 resolved that issue a long time ago.



OK. We have agree upon at least a few things,

1. EmbeddedFileList (or map) has to be in sync with params, has to be
valid, so it can not be separated from params. In this sense, who
should be the core information (so that another one does not have to
be updated) is a false problem.

2. EmbeddedFile needs this meta_, and your solution of map is acceptable to me. However, meta_ is a solution to a
general problem, that may be needed anyway in, for example,
InsetGraphics. I am not against the idea that you fix InsetBibtex now,
and fix InsetGraphic later.

  
We won't know this until we look at fixing that. I'm inclined, actually, 
to do something more general, namely, to try to bring InsetGraphic into 
the InsetCommand hierarchy. I think this was intended all along as part 
of a more general transition that was never finished.



There are then some minor problems left, such as 1. In functions such
as addDatabase, should we update params first and update
EmbeddedFileList/Map; or another way around? 
  
or encapsulate it in overridden setParam methods, so you can't forget to 
do it.



2. When we are in need
some information, from whom should we ask for it? I say they are minor
problems because if params and EmbeddedFiles are in sync, it does not
really matter, so let use just use the easier method. 

  
Iterating over params is more sensible, I think, if only because, if 
embedding is not enabled, you needn't consult anything else. Exactly 
what the code will look like, I don't yet know, but we'll have things like:

   string const database =
   buffer().enabled() ? consult(paramIterator->first) : 
paramIterator->first;

We'd anyway have (more or less)
   string const database =
   buffer().enabled() ? bibfilesIterator->availableFilename() : 
bibfilesIterator->metaInfo();
So it doesn't end up mattering that much, and the former makes it 
clearer exactly where the information from EmbeddedFiles is used and 
where it is not.


Anyway, it sounds as if we're well enough agreed that I can move ahead 
with this.


rh



Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Bo Peng
>  > There are then some minor problems left, such as 1. In functions such
>  > as addDatabase, should we update params first and update
>  > EmbeddedFileList/Map; or another way around?
>  >
>  or encapsulate it in overridden setParam methods, so you can't forget to
>  do it.

I disliked your implementation of setParam because if you set params
first, and then update EmbeddedFileList, you may be bounced back to
set params again (to disable embedding). On the other hand, if you
update EmbeddedFile first, you do not have to do this. Anyway, all
these can happen in setParam and the differences are trivial.

>  > 2. When we are in need
>  > some information, from whom should we ask for it? I say they are minor
>  > problems because if params and EmbeddedFiles are in sync, it does not
>  > really matter, so let use just use the easier method.
>  >
>  >
>  Iterating over params is more sensible, I think, if only because, if
>  embedding is not enabled, you needn't consult anything else. Exactly
>  what the code will look like, I don't yet know, but we'll have things like:
> string const database =
> buffer().enabled() ? consult(paramIterator->first) :
>  paramIterator->first;
>  We'd anyway have (more or less)
> string const database =
> buffer().enabled() ? bibfilesIterator->availableFilename() :
>  bibfilesIterator->metaInfo();
>  So it doesn't end up mattering that much, and the former makes it
>  clearer exactly where the information from EmbeddedFiles is used and
>  where it is not.

I will have to see your patch (or commits) to determine if I like it,
but you know I dislike parsing params again and again. :-)

Bo


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread Andre Poenitz
On Tue, Apr 01, 2008 at 05:36:20PM -0400, rgheck wrote:
> Bo Peng wrote:
>>>  > What is the key to this map? If it is biblio, not /path/to/biblio.bib,
>>>  > then you can make bibfiles_ a map, and the meta_ patch is no longer
>>>  > needed.
>>>  >
>>>  Yes. This is what I've been trying to say, more or less. But note that,
>>>  when we output latex, we iterate not over bibfiles_ but over params, and
>>>  we (quickly) look up the embedded file info in the map. We do it this
>>>  way because we should keep the order. This requires that bibfiles_ and
>>>  the params be in sync, but they have to be kept in sync anyway. We
>>>  resolved that issue a long time ago.
>>> 
>>
>> OK. We have agree upon at least a few things,
>>
>> 1. EmbeddedFileList (or map) has to be in sync with params, has to be
>> valid, so it can not be separated from params. In this sense, who
>> should be the core information (so that another one does not have to
>> be updated) is a false problem.
>>
>> 2. EmbeddedFile needs this meta_, and your solution of map> EmbeddedFile> is acceptable to me. However, meta_ is a solution to a
>> general problem, that may be needed anyway in, for example,
>> InsetGraphics. I am not against the idea that you fix InsetBibtex now,
>> and fix InsetGraphic later.
>>
>>   
> We won't know this until we look at fixing that. I'm inclined, actually, to 
> do something more general, namely, to try to bring InsetGraphic into the 
> InsetCommand hierarchy. I think this was intended all along as part of a 
> more general transition that was never finished.

Could you please elaborate on why having the InsetCommand hierarchy is
useful at all?

As far as I can tell the main benefit was to have a uniform means to
pass "Parameters" through the Controller layer in the Old Days. This
layer is gone now, and it feels a bit strange to pass around unrelated
pieces of data from otherwise unrelated insets in a common structure.
Wouldn't per-inset specific data with direct access from the GUI simplify
overall structure and reduce conversion costs?

[Note that I did not follow GUI development too closely then, so I might
easily miss something very important here.]

Andre'


Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]

2008-04-01 Thread rgheck

Andre Poenitz wrote:
> We won't know this until we look at fixing that. I'm inclined, actually, to 
> do something more general, namely, to try to bring InsetGraphic into the 
> InsetCommand hierarchy. I think this was intended all along as part of a 
> more general transition that was never finished.
  


Could you please elaborate on why having the InsetCommand hierarchy is
useful at all?

As far as I can tell the main benefit was to have a uniform means to
pass "Parameters" through the Controller layer in the Old Days. This
layer is gone now, and it feels a bit strange to pass around unrelated
pieces of data from otherwise unrelated insets in a common structure.
Wouldn't per-inset specific data with direct access from the GUI simplify
overall structure and reduce conversion costs?
  


Frankly, I don't know the overall structure here well enough to be sure 
exactly what the advantages or disadvantages to keeping the 
InsetCommandParams business the way it is. Mostly, I try to go on how 
Georg explained this to me some time ago, and the understanding I've 
developed since then of why he (they?) did things the way they did. In 
particular, Georg insisted that we are not to subclass 
InsetCommandParams, not unless we have a really, really good reason. 
There are times it seems it would be very natural to me to do so, but 
Georg made a strong case (I don't remember all the details) for not 
doing so, and if that's how he designed the code, then, well, I'm 
guessing there are reasons, and I'd probably find them out if I violated 
his orders.


I agree that there's something unhappy about the stringification, 
unstringification, and so on and so forth. But, as JMarc often says, we 
know we have to do that anyway---remember that the strings are in 
exactly the form they are in the .lyx file---so it's not that 
unreasonable to use the same infrastructure to pass things around. That 
said, I also agree with Abdel that we shouldn't bind ourselves to doing 
that if it makes things overly complex, and I don't really care that 
much how things are passed back and forth from the GUI. But that's 
really a separate issue from how ICP works. You can build an ICP without 
all the stringification. You just do it directly:

params.setParam("bibfiles") = whatever;
and then you can pass params itself to the inset if you like. Doing so 
has a downside, though, one that JMarc (again) is always pointing out, 
namely, that if you go via an LFUN, you get all kinds of updates, etc, 
for free; in particular, going via the dispatch mechanism gets you this. 
But then you do need a common representation of the parameters being 
passed, and a string is a pretty natural choice. (I don't even want to 
think about all the casting that would otherwise be required.) But the 
main point, really, is that this is a different issue. The conversion 
cost question is really separate from the question whether ICP is A Good 
Thing. By itself, getting rid of ICP wouldn't help you there.


Anyway, even if there were reasons to do a large scale re-write of this 
stuff, I don't see anyone doing that any time real soon. So, unless 
someone is going to do that, then, as things stand, I strongly suspect 
that there's substantial duplication of code across InsetGraphicsParams, 
InsetExternalParams, and InsetCommandParams. (There has to be.) Barring 
the massive re-write, then, the strategy I suggested seems sensible. And 
for the same reason, barring some large-scale change of direction, I'm 
reluctant to introduce InsetBibtexParams and take InsetBibtex out of the 
existing framework.


Hope that helps.

Richard



Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
On Sun, Mar 30, 2008 at 9:39 AM, Bo Peng [EMAIL PROTECTED] wrote:
   Well, that's my case. Bo, you want to state yours?

Hi, Richard,

Can we settle down on the meta_ solution, at least for now? This
debate has lasted for a week and I do not see a clear end. Let us
focus on bug fixing, and seek for a better solution if mine is proven
to be insufficient.

Cheers,
Bo


Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

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); 

Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

Abdelrazak Younes wrote:

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' seems to have a point here... Long discussions often means that 
some designs needs to be changed :-)


Right, but as I said in the reply to Andre, my point was supposed to be 
just that we should have a simple, consistent interface. I don't myself 
care whether it is the current one or a new one, though it's an argument 
in favor of the current one that (a) it works just fine and (b) 
re-writing it would be a lot of work. What we should not do, it seems to 
me, is have inconsistencies in the interface. My point was that Bo's 
code gives us precisely that. The interface is so inconsistent that the 
example code I gave can lead to crashes.


rh



Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
  Right, but as I said in the reply to Andre, my point was supposed to be
  just that we should have a simple, consistent interface. I don't myself
  care whether it is the current one or a new one, though it's an argument
  in favor of the current one that (a) it works just fine and (b)
  re-writing it would be a lot of work. What we should not do, it seems to
  me, is have inconsistencies in the interface. My point was that Bo's
  code gives us precisely that. The interface is so inconsistent that the
  example code I gave can lead to crashes.

I do not know which crash you are talking about but as I have said,
params is not enough to make InsetBibtex run smoothly, reconstructing
EmbeddedFileList each time from param is likely to lead to crashes.

Anyway, if we eventually agrees on how embedding *should* work, I can
give up on this discussion and leave InsetBibtex to you, as long as
you do not change EmbeddedFile interface that much. Warning you again,
that, please produce valid EmbeddedFileList each time you update your
params.

Bo


Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

Bo Peng wrote:

 Right, but as I said in the reply to Andre, my point was supposed to be
 just that we should have a simple, consistent interface. I don't myself
 care whether it is the current one or a new one, though it's an argument
 in favor of the current one that (a) it works just fine and (b)
 re-writing it would be a lot of work. What we should not do, it seems to
 me, is have inconsistencies in the interface. My point was that Bo's
 code gives us precisely that. The interface is so inconsistent that the
 example code I gave can lead to crashes.



I do not know which crash you are talking about 
  
This was mentioned in an earlier message. It's not an actual crash but a 
potential crash. See below.



but as I have said,
params is not enough to make InsetBibtex run smoothly, reconstructing
EmbeddedFileList each time from param is likely to lead to crashes.

  
But your original code DID reconstruct EmbeddedFileList: 
LFUN_INSET_MODIFY called createBibFiles(), which deleted the old 
structure and built a new one from scratch, and this happened every time 
the user edited the inset. You and I agreed a while ago that we 
shouldn't reconstruct this each time. Apparently, it worked, but it's a 
bit of a waste. So I fixed that and renamed createBibFiles to 
updateBibFiles, and now it's not reconstructed each time, but we just 
update the ones that need updating. The same code appears in my patch. 
That's where I got it.


I think the issue here is being misunderstood. We agree that (given the 
current implementation of embedding), we need to have both these 
structures, and we have to keep them in sync. The issue is just whether, 
when we need to access the parameters, we should do so by looking at 
InsetCommandParams, which is the way most of the code works, or whether 
we should look at the EmbeddedFileList. Now, if they're in sync, like 
they're supposed to be, it doesn't really matter, except that it does: I 
want all these insets to use a consistent interface to the parameters, 
for maintainability reasons if no other; and I want to separate the guts 
of the inset from the embedding stuff, so that, if the embedding stuff 
changes, we don't have to re-write the guts of the inset. This seems 
like standard encapsulation sort of stuff. And indeed...



Anyway, if we eventually agree on how embedding *should* work, I can
give up on this discussion and leave InsetBibtex to you, as long as
you do not change EmbeddedFile interface that much. 

  
this illustrates the point: We shouldn't have to wait until we decide 
how embedding works to figure out how we iterate over the bibfiles. Of 
course, embedding will interact in SOME ways with the inset. But it 
should be used where it is needed, and not used elsewhere.



Warning you again, that, please produce valid EmbeddedFileList each time you 
update your
params.

  

That I understand now.

rh



Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
   but as I have said,
   params is not enough to make InsetBibtex run smoothly, reconstructing
   EmbeddedFileList each time from param is likely to lead to crashes.
  
  
  But your original code DID reconstruct EmbeddedFileList:
  LFUN_INSET_MODIFY called createBibFiles(), which deleted the old
  structure and built a new one from scratch, and this happened every time
  the user edited the inset. You and I agreed a while ago that we
  shouldn't reconstruct this each time. Apparently, it worked, but it's a
  bit of a waste. So I fixed that and renamed createBibFiles to
  updateBibFiles, and now it's not reconstructed each time, but we just
  update the ones that need updating. The same code appears in my patch.
  That's where I got it.

I was lazy (which may lead to bugs), and the current code is better.
The point is that an EmbeddedFile needs to be created once for each
file. If you call something like setParam(getParam()) which of course
should be discouraged, EmbeddedFileList should not change.

  The issue is just whether,
  when we need to access the parameters, we should do so by looking at
  InsetCommandParams, which is the way most of the code works, or whether
  we should look at the EmbeddedFileList.

As I have repeated several times, when we look at params, we do not
always get what we want because params *does not* hold all the
information. For example, when we output to latex, and the file is
embedded, we need to use the embedded version and may (I have not
figured this part out) need to output /tmp/blah/biblio, instead of
biblio. This is why we should ask EmbeddedFile for the parameters.

Cheers,
Bo


Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

Bo Peng wrote:

The issue is just whether,
 when we need to access the parameters, we should do so by looking at
 InsetCommandParams, which is the way most of the code works, or whether
 we should look at the EmbeddedFileList.



As I have repeated several times, when we look at params, we do not
always get what we want because params *does not* hold all the
information. For example, when we output to latex, and the file is
embedded, we need to use the embedded version and may (I have not
figured this part out) need to output /tmp/blah/biblio, instead of
biblio. This is why we should ask EmbeddedFile for the parameters.

  
Yes, I understand that we need to look up that information. But what I'm 
saying is that I want to use the EmbeddedFileList, which we're keeping 
in sync with the params, ONLY to look that information up if and when 
it's needed, and not use it in other ways. Yes, this is a minor dispute 
now. But it matters, it seems to me, for the other reasons I've 
mentioned, viz: If the embedding stuff changes, I want the change only 
to affect things that have to do with embedding.


Here's a simple example. I can see lots of reasons one might use a map 
in EmbeddedFileList rather than a vector. A common thing to do here is 
to find the EmbeddedFile object representing a certain absolute path, 
and this is trivial with a map. But if you did that, given the current 
implementation, the latex output would (probably) change! Why? Because 
maps are sorted by key. I don't think this would actually matter in this 
particular case---I don't think that the order in which the bibfiles are 
given is significant---but the fact that this could happen points to a 
problem with how the EmbeddedFileList is being used. The latex output 
should not depend upon implementation details of some distant class, and 
the implementation of EmbeddedFileList should not be constrained by the 
way we iterate over the bibfiles in InsetBibtex.


I think the latex output stuff is OK now, at least as far as ViewDVI is 
concerned. At least, I think I committed that part. If not, I will.


rh



Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
  Yes, I understand that we need to look up that information. But what I'm
  saying is that I want to use the EmbeddedFileList, which we're keeping
  in sync with the params, ONLY to look that information up if and when
  it's needed, and not use it in other ways. Yes, this is a minor dispute
  now. But it matters, it seems to me, for the other reasons I've
  mentioned, viz: If the embedding stuff changes, I want the change only
  to affect things that have to do with embedding.

snip Your example /snip

I can not believe that we spent so much time on this small issue. Of
course when we use another reprentation to params, we would better be
able to reproduce params. The meta_ patch will do exactly this, and if
someone would like to use a EmbeddedFileMap, the same issue should be
considered. I do not want to argue any more. I propose that any of us
get an OK first from another developer can go ahead, all right?

  I think the latex output stuff is OK now, at least as far as ViewDVI is
  concerned. At least, I think I committed that part. If not, I will.

It is not. Please commit the fix if you have them.

Bo


Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
On Sun, Mar 30, 2008 at 9:39 AM, Bo Peng <[EMAIL PROTECTED]> wrote:
> >  Well, that's my case. Bo, you want to state yours?

Hi, Richard,

Can we settle down on the meta_ solution, at least for now? This
debate has lasted for a week and I do not see a clear end. Let us
focus on bug fixing, and seek for a better solution if mine is proven
to be insufficient.

Cheers,
Bo


Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

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); 

Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

Abdelrazak Younes wrote:

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' seems to have a point here... Long discussions often means that 
some designs needs to be changed :-)


Right, but as I said in the reply to Andre, my point was supposed to be 
just that we should have a simple, consistent interface. I don't myself 
care whether it is the current one or a new one, though it's an argument 
in favor of the current one that (a) it works just fine and (b) 
re-writing it would be a lot of work. What we should not do, it seems to 
me, is have inconsistencies in the interface. My point was that Bo's 
code gives us precisely that. The interface is so inconsistent that the 
example code I gave can lead to crashes.


rh



Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
>  Right, but as I said in the reply to Andre, my point was supposed to be
>  just that we should have a simple, consistent interface. I don't myself
>  care whether it is the current one or a new one, though it's an argument
>  in favor of the current one that (a) it works just fine and (b)
>  re-writing it would be a lot of work. What we should not do, it seems to
>  me, is have inconsistencies in the interface. My point was that Bo's
>  code gives us precisely that. The interface is so inconsistent that the
>  example code I gave can lead to crashes.

I do not know which crash you are talking about but as I have said,
params is not enough to make InsetBibtex run smoothly, reconstructing
EmbeddedFileList each time from param is likely to lead to crashes.

Anyway, if we eventually agrees on how embedding *should* work, I can
give up on this discussion and leave InsetBibtex to you, as long as
you do not change EmbeddedFile interface that much. Warning you again,
that, please produce valid EmbeddedFileList each time you update your
params.

Bo


Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

Bo Peng wrote:

 Right, but as I said in the reply to Andre, my point was supposed to be
 just that we should have a simple, consistent interface. I don't myself
 care whether it is the current one or a new one, though it's an argument
 in favor of the current one that (a) it works just fine and (b)
 re-writing it would be a lot of work. What we should not do, it seems to
 me, is have inconsistencies in the interface. My point was that Bo's
 code gives us precisely that. The interface is so inconsistent that the
 example code I gave can lead to crashes.



I do not know which crash you are talking about 
  
This was mentioned in an earlier message. It's not an actual crash but a 
potential crash. See below.



but as I have said,
params is not enough to make InsetBibtex run smoothly, reconstructing
EmbeddedFileList each time from param is likely to lead to crashes.

  
But your original code DID reconstruct EmbeddedFileList: 
LFUN_INSET_MODIFY called createBibFiles(), which deleted the old 
structure and built a new one from scratch, and this happened every time 
the user edited the inset. You and I agreed a while ago that we 
shouldn't reconstruct this each time. Apparently, it worked, but it's a 
bit of a waste. So I fixed that and renamed createBibFiles to 
updateBibFiles, and now it's not reconstructed each time, but we just 
update the ones that need updating. The same code appears in my patch. 
That's where I got it.


I think the issue here is being misunderstood. We agree that (given the 
current implementation of embedding), we need to have both these 
structures, and we have to keep them in sync. The issue is just whether, 
when we need to access the parameters, we should do so by looking at 
InsetCommandParams, which is the way most of the code works, or whether 
we should look at the EmbeddedFileList. Now, if they're in sync, like 
they're supposed to be, it doesn't really matter, except that it does: I 
want all these insets to use a consistent interface to the parameters, 
for maintainability reasons if no other; and I want to separate the guts 
of the inset from the embedding stuff, so that, if the embedding stuff 
changes, we don't have to re-write the guts of the inset. This seems 
like standard encapsulation sort of stuff. And indeed...



Anyway, if we eventually agree on how embedding *should* work, I can
give up on this discussion and leave InsetBibtex to you, as long as
you do not change EmbeddedFile interface that much. 

  
this illustrates the point: We shouldn't have to wait until we decide 
how embedding works to figure out how we iterate over the bibfiles. Of 
course, embedding will interact in SOME ways with the inset. But it 
should be used where it is needed, and not used elsewhere.



Warning you again, that, please produce valid EmbeddedFileList each time you 
update your
params.

  

That I understand now.

rh



Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
>  > but as I have said,
>  > params is not enough to make InsetBibtex run smoothly, reconstructing
>  > EmbeddedFileList each time from param is likely to lead to crashes.
>  >
>  >
>  But your original code DID reconstruct EmbeddedFileList:
>  LFUN_INSET_MODIFY called createBibFiles(), which deleted the old
>  structure and built a new one from scratch, and this happened every time
>  the user edited the inset. You and I agreed a while ago that we
>  shouldn't reconstruct this each time. Apparently, it worked, but it's a
>  bit of a waste. So I fixed that and renamed createBibFiles to
>  updateBibFiles, and now it's not reconstructed each time, but we just
>  update the ones that need updating. The same code appears in my patch.
>  That's where I got it.

I was lazy (which may lead to bugs), and the current code is better.
The point is that an EmbeddedFile needs to be created once for each
file. If you call something like setParam(getParam()) which of course
should be discouraged, EmbeddedFileList should not change.

>  The issue is just whether,
>  when we need to access the parameters, we should do so by looking at
>  InsetCommandParams, which is the way most of the code works, or whether
>  we should look at the EmbeddedFileList.

As I have repeated several times, when we look at params, we do not
always get what we want because params *does not* hold all the
information. For example, when we output to latex, and the file is
embedded, we need to use the embedded version and may (I have not
figured this part out) need to output /tmp/blah/biblio, instead of
biblio. This is why we should ask EmbeddedFile for the parameters.

Cheers,
Bo


Re: InsetBibtex Design Issue

2008-03-31 Thread Richard Heck

Bo Peng wrote:

The issue is just whether,
 when we need to access the parameters, we should do so by looking at
 InsetCommandParams, which is the way most of the code works, or whether
 we should look at the EmbeddedFileList.



As I have repeated several times, when we look at params, we do not
always get what we want because params *does not* hold all the
information. For example, when we output to latex, and the file is
embedded, we need to use the embedded version and may (I have not
figured this part out) need to output /tmp/blah/biblio, instead of
biblio. This is why we should ask EmbeddedFile for the parameters.

  
Yes, I understand that we need to look up that information. But what I'm 
saying is that I want to use the EmbeddedFileList, which we're keeping 
in sync with the params, ONLY to look that information up if and when 
it's needed, and not use it in other ways. Yes, this is a minor dispute 
now. But it matters, it seems to me, for the other reasons I've 
mentioned, viz: If the embedding stuff changes, I want the change only 
to affect things that have to do with embedding.


Here's a simple example. I can see lots of reasons one might use a map 
in EmbeddedFileList rather than a vector. A common thing to do here is 
to find the EmbeddedFile object representing a certain absolute path, 
and this is trivial with a map. But if you did that, given the current 
implementation, the latex output would (probably) change! Why? Because 
maps are sorted by key. I don't think this would actually matter in this 
particular case---I don't think that the order in which the bibfiles are 
given is significant---but the fact that this could happen points to a 
problem with how the EmbeddedFileList is being used. The latex output 
should not depend upon implementation details of some distant class, and 
the implementation of EmbeddedFileList should not be constrained by the 
way we iterate over the bibfiles in InsetBibtex.


I think the latex output stuff is OK now, at least as far as View>DVI is 
concerned. At least, I think I committed that part. If not, I will.


rh



Re: InsetBibtex Design Issue

2008-03-31 Thread Bo Peng
>  Yes, I understand that we need to look up that information. But what I'm
>  saying is that I want to use the EmbeddedFileList, which we're keeping
>  in sync with the params, ONLY to look that information up if and when
>  it's needed, and not use it in other ways. Yes, this is a minor dispute
>  now. But it matters, it seems to me, for the other reasons I've
>  mentioned, viz: If the embedding stuff changes, I want the change only
>  to affect things that have to do with embedding.

 Your example 

I can not believe that we spent so much time on this small issue. Of
course when we use another reprentation to params, we would better be
able to reproduce params. The meta_ patch will do exactly this, and if
someone would like to use a EmbeddedFileMap, the same issue should be
considered. I do not want to argue any more. I propose that any of us
get an OK first from another developer can go ahead, all right?

>  I think the latex output stuff is OK now, at least as far as View>DVI is
>  concerned. At least, I think I committed that part. If not, I will.

It is not. Please commit the fix if you have them.

Bo


InsetBibtex Design Issue

2008-03-30 Thread rgheck


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 

Re: InsetBibtex Design Issue

2008-03-30 Thread Andre Poenitz
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 

Re: InsetBibtex Design Issue

2008-03-30 Thread Abdelrazak Younes

Andre Poenitz wrote:

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.


Maybe I'd be even worth to generalize those at Inset level (using 
docstring):


virtual docstring Inset::toString() const;
virtual void Inset::toString(docstring const );



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.




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' seems to have a point here... Long discussions often means that 
some designs needs to be changed :-)


Abdel.



Re: InsetBibtex Design Issue

2008-03-30 Thread Bo Peng
  Well, that's my case. Bo, you want to state yours?

You have stated the problems clear enough. It seems to me that your
solution will not work because params, by its nature, does not hold
enough information to reconstruct EmbeddedFileList, so a lot of
complexity will have to be introduced to make it work. I use
EmbeddedFIleList to represent the list, and contruct params when
needed. The only problem, as you have said, is the lack of original
user input, which may be represented in other cases as a lack of
relative path information. The natural solution is to add such
information to EmbeddedFile as meta_, or as a main piece of
information if we are going to stop deriving EmbeddedFile from
DocFileName.

I do not actually see any problem with this approach, but if params is
indeed troublesome to work with, it is params that needs to be
changed.

Cheers,
Bo


InsetBibtex Design Issue

2008-03-30 Thread rgheck


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 

Re: InsetBibtex Design Issue

2008-03-30 Thread Andre Poenitz
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 

Re: InsetBibtex Design Issue

2008-03-30 Thread Abdelrazak Younes

Andre Poenitz wrote:

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.


Maybe I'd be even worth to generalize those at Inset level (using 
docstring):


virtual docstring Inset::toString() const;
virtual void Inset::toString(docstring const &);



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.




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' seems to have a point here... Long discussions often means that 
some designs needs to be changed :-)


Abdel.



Re: InsetBibtex Design Issue

2008-03-30 Thread Bo Peng
>  Well, that's my case. Bo, you want to state yours?

You have stated the problems clear enough. It seems to me that your
solution will not work because params, by its nature, does not hold
enough information to reconstruct EmbeddedFileList, so a lot of
complexity will have to be introduced to make it work. I use
EmbeddedFIleList to represent the list, and contruct params when
needed. The only problem, as you have said, is the lack of original
user input, which may be represented in other cases as a lack of
relative path information. The natural solution is to add such
information to EmbeddedFile as meta_, or as a main piece of
information if we are going to stop deriving EmbeddedFile from
DocFileName.

I do not actually see any problem with this approach, but if params is
indeed troublesome to work with, it is params that needs to be
changed.

Cheers,
Bo