Richard Heck wrote: > Abdelrazak Younes wrote: >>> + //It's not obvious this is the right place for this enum. Note >>> that it >>> + //serves a different purpose from the one in insetbase.C and so >>> can't be >>> + //replaced by it. >> Couldn't they be merged instead? See above. > The InsetBase::Code can be different even if we're dealing with the same > insets. For example, url and htmlurl are both commands for an InsetUrl, > but they map to differents Codes. So no, unfortunately.
This is by design. InsetCommandParams is not supposed to know anything about the inset. It is only a generic container of commands: For each command it knows what parameters are allowed. >> I am not well versed in this InsetCommand stuff but your patch looks >> good and the comments look sound. But, and this is a big BUT, please >> put this new conversion method command2ParamType() *outside* of the >> InsetCommandParams class. InsetCommandParams is a base class and >> should not know _anything_ about which type of class is using it. > This is the root of the problem. An instance of InsetCommandParams does > need to know what kind of inset is using it, because that's the only way > it can make sure that new commands passed to it via read() or > setCmdName() are legitimate. No, the inset is supposed to ensure that only valid parameters are passed. The case that you are adressing was not thought of: Stuff that comes from a .lyx file is assumed to be correct (we do this elsewhere, too), and we were not aware about the minibuffer. > In any event, this information was already > stored in the name_ variable. I'm just storing it in a different form > and, in the process, dissocating the inset type from the command name, > which would allow a bit more flexibility if it were wanted. The key to > the patch is really the isCompatible() method, and that could have been > written without a converter or the InsetParamType enum thus: > if (newname == "bibtex") > if (oldname == "bibtext") return true; > else return false; > ... > if (newname == "url" || newname == "htmlurl") > if (oldname == "url" || oldname == "htmlurl") return true; > else return false; > But that gets very ugly when you get more names involved. Hence the idea > of using the converter as a kind of helper method. This does not fit into the current design. Please do not soften it, if you wnat to go along the lines of your patch please do it completely, or use something that is compatible with the current design. For example, you could add an InsetBase::Code field to the CommandInfo struct. Then you can check very easily if the command is compatible to an inset by comparing the codes e.g. in the inset constructor. Tnis would be an extension of the current design: Simply store one more item in the info table, but there is no duplicated list of command names. > That said, I agree with you that maybe InsetCommandParams should be pure > virtual and that each kind of inset should subclass it for its own > purposes. And after looking very quickly at the code, I don't think this > would be very hard, though it would mean a lot of changes elsewhere, > e.g., in factory.C, where all the calls like: We investigated the pure virtual option and did not use it because it would require a lot of additional glue code (in insets, kernel, and controllers). We decided that there was no justification for this line noise. > InsetCommandParams p( "include") > would need to change to: > InsetIncludeCommandParams(p) > or perhaps the InsetCommandParams constructor could become a kind of > factory method. It is one already. Georg PS: Did you read the comments in the header? InsetCommandParams is in a transitional state, several methods that look ugly are supposed to go.
