On Wed, Apr 02, 2008 at 03:58:37PM -0400, Richard Heck wrote: >>> But (a) none of this code would vanish if you rolled ICP into >>> InsetCommand. All of that information would still be needed, and I'd >>> still want at least the second and third method to be static. >>> >> >> Why oh why? What's wrong with mathed that it can live without it? >> >> Why would we ever need to have 'isCompatibleCommand'? Just because you >> need some kind of call back from the 'generic' ICP::read/write routines? >> This is exactly a sign that the stuff is _not_ generic. >> >> > No, we don't need a callback, and I don't know about mathed---that is a > part of the code I dare not even examine, and if you say mathed doesn't > need it I'll believe you. But adding this method solved a crash in the > inset stuff, one I inadvertently triggered by entering an ill-formed > parameter string via the mini-buffer. (I don't recall the bug number. I can > find it if you like.) The short version is that the parameters for a > particular type of inset need to know what commands it accepts, if only so > that it can do error-checking.
And for that the "generic" code in InsetCommandParams calls some static function in InsetSpecificFoo to get a list of acceptable parameters. This form of "customizing" is unusual and nowhere else used in LyX. Moreover, it is conceptionally not needed if InsetSpecificFoo is able to read _its_ parameters. > (An instance of ICP does know for which kind > of inset it is the parameters; this is just an alternative to subclasses, > hence the factory; there's no substantial issue there.) The point is: The more unusual concepts you add to any non-trivial body of code, the higher the maintanance costs are. So _even if the "unusual" solution works_ it should _not be used_ as long as a more common approach works similarily well. "Faking inheritance" _is_ unusual in C++. > The reason the > method is static is because of the reason discussed in the other thread: An > instance of ICP (or even of InsetGraphicParams) can float free of any > inset; so we can't call isCompatibleCommand() on an instance. This "free floating" would work with "InsetFoo" and "InsetFooParams", too. No need to "derive" from ICP. > All this code used to be in ICP itself, but it seemed more sensible to have > each inset know what commands it can accept. And what parameters those > commands require, and of what types. There is a lot more flexibility here > now than there used to be. None of this even needs to be hard-coded. You > could have user-defined command insets that accepted arbitrary parameters, > all described in module files, kind of like how custom flex insets work. Never, ever, enter code for flexibility you are not ready to use immediately. Keep flexibility in mind, but do not create code for it unless you actually need it. > It's just that these would be command insets. If you're going to do that, > you need a very general, and very flexible, system for storing parameters. I don't see how this possibly can beat per-inset parameter structures in flexibility. > This is all on my to-do list, and we're part of the way there. So we're > going to have that kind of system, either as ICP or InsetFlexCommandParams. > Why not just use it? Because we have be severely burned by overengineering in the past. The GUII excess has effectively stalled any decent GUI development for half a decade. 1.6 will be the first release that comes with fundamnetally new GUI elements, and they were only possibly by not using the old corset. >>> This was the design choice Georg et al made. They could have subclassed >>> ICP and implemented the factory in a different way. >>> >> >> No, I need at most inset specific data container which can store >> each parameter in its respective "native" format (say lengths as >> Length) and not everything in a std::map<std::string, docstring>. >> >> > Sure. If you want to go to inset specific parameter classes, I have no > strong objection; I suggested something like this implementation myself to > Georg in the conversations to which I keep referring. My idea was to keep > ICP as a virtual base class and subclass it for each inset; in many cases, > such as InsetLabel, that will be sufficient. In other cases, you could do > other things. Or maybe a different architecture is better. If so, then so. > > But I think what you suggest will make the code harder to understand, > because each inset will work differently from the rest. Why would that need to be like that? For all I can tell 50 "different" mathed insets are working more uniformly than 11 InsetCommand based insets. Insets only work different if someone makes them like that. > I say this from > experience. I understand very well how all the InsetCommand insets work; > understanding one is pretty much understanding all. This is not true of > InsetGraphics and InsetExternal. We could debate how much of a cost this > is. But it is a cost. And many of the some advantages of the approach you > are suggesting could be gained within ICP by associating type information > with parameters. Most of that rooted in history, not in technology. The "big" text insets have all been written by different people at very different times. This still shows, even if people have been working on "inset unification" for quite a while. In contrast to that, mathed was hit usually only by a single person at any given time, with long periods of dormancy inbetween. >> Restricting yourselves to this approach leeds to the kind of trouble >> you are currently discussing with Bo: An embedded object seems to >> be more than a string, and suddenly major infrastructure changes >> _outside_ the "guilty" inset are needed just because it does not fit >> well into the abstraction. > > Yes, I agree, to some extent. I've made the same point in a different > thread. The problem is that the string representation is constraining. But > this is not just an issue for InsetBibtex; it's a general problem, and it > could have a general solution, if we wanted it to have one; there is no > reason that everything has to be a string. Right. And the solution is "Use the respective inset's native representation". If it is a string, well, it's a string. If it is a "Length" is should be stored as such _even_ if a Lenght has a string representation. Same for file names. > But I disagree that embedded objects are parameters. In particular, I think > using an EmbeddedFile object to represent a pair of strings is overkill > that ultimately makes the code less comprehensible and less maintainable, > because (a) changes to the EmbeddedFile stuff could in principle affect the > inner workings of the insets and (b) you can't work on the inset without > figuring out the (extremely complex) EmbeddedFile code. (Somewhere in there > is the reason kpsewhich support got broken, and there are still bugs > elsewhere that we have for similar reasons.) As far as I'm concerned, it's > no argument that the EmbeddedFile objects are already lying around. They > should do the work they need to do and leave the rest of the inset alone. > And I'd feel exactly the same way about this if we were talking about > InsetBibtexParams. This issue has nothing to do with ICP. Isn't EmbeddedFile all about representing an "Embedded file"? If there are problems, they need to be fixed there. But I can't imagine a solution getting easier by requiring it to _additionally_ fit into a map<string, string>. Andre'
