Re: Can Someone Please Resolve This? [Re: InsetBibtex Design Issue]
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]
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]
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]
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]
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]
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]
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]
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]
"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]
> > 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]
> 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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
> 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]
"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]
> 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]
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]
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]
> 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]
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]
> > 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 mapis 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]
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 mapis 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]
> > 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]
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]
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
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
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
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
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
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
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
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
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
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
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
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
> 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
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
> > 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
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
> 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
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
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
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
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
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
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
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
> 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