Hi Andreas, (Answering both of your messages in one.)
Andreas Delmelle wrote: > On Sep 16, 2008, at 12:05, Vincent Hennebert wrote: > >> >> I think this is negligible, precisely because we’re not talking about >> a lot of missing cells. This would start to be noticeable if we were >> dealing with huge empty tables, and in that case there’s probably >> something wrong with the source document. > > Not necessarily. Maybe the author was just trying to be creative > (avoiding a trip through SVG, by using tables as a medium-resolution > grid; something like that?) I know users can be very creative when it comes to using tables. Still. Assuming there’s a use case for the example you describe, this will probably be to achieve a fancy layout in a magazine-like document. Not a 10 thousand page document then, and the overhead won’t be noticeable. Also, let’s not forget that we’re dealing with a corner case here. In most cases empty table-cell elements /will/ be added by the XSLT stylesheet that produces the FO document anyway. The fact that this issue occurred only recently is a confirmation of that. Hence why the mentioned overhead isn’t really an issue, and why specific code should be kept to a minimum. > Sounds about right to me. Besides that, the instance size of > a TableCell > is large enough to justify not generating any more of these than > strictly necessary. Let alone increase the amount of memory used by the > FO tree even further if we would also start generating implicit empty > Blocks... Now /that/ is maybe the place for optimizations: improve the FO tree. For example make sure that no object is created if a property has its default value; or use a common read-only object for the default value. All the FO tree would benefit from that kind of optimization. But I’m not a specialist of that area... >>> RowPainter is area-generation stage. My solution is just making sure >>> areas are generated with the information that is available anyway. It's >>> not doing anything in a place where it shouldn't do it. > > Ultimately, that is the only place where this should have effects, I think. > Seems like we don't really need dummy nodes for anything layout-related, > so why precisely should they exist at that point? > Unless a darn' *good* answer to this question is given (i.e. a point > other than a rigid adherence to the 'no-duplication' rule) But this is exactly that. A rigid, strict, blind adherence to the ‘no-duplication’ rule. Because code duplication almost always is a sign that something is wrong in the architecture. Because something that looks like just a little redundancy generally reveals a deeper problem, that grows with time and eventually may totally prevent the implementation of new features. >> At any rate, this is not worth >> the additional maintenance required by specific code. > > Could be right here, but such considerations are always worth some > thought... I already partly answered that above by mentioning the corner case; more below. > What is the more precise description of this 'additional maintenance' in > this case? > Is it really /that/ much that it justifies the generation of additional > relatively large and long-lived objects so early in the process? > (Objects that basically correspond to nothing at all; maybe due to code > that was added just to make the code look cleaner... ;-)) What do you mean? > If still yes, then is there really no other approach? There is almost always another approach ;-). As I said I didn’t have the opportunity to think much about that, and that’s the idea that immediately came to my mind. Now there’s probably a way to handle that in GridUnit, but the getCell() method only is already called in quite a few places. > If no, then would it be better to do it now (before refactoring the > layoutengine) or after? > > Maybe if we leave it, but take note for now, a better solution will > become apparent once the refactoring is done (?) Maybe, but usually the discussion is carried out prior to the commit... And in that case proper TODOs are placed at proper places in the code. And possibly a bug report opened. > <snip /> >> [Jeremias: ] >>> RowPainter is area-generation stage. My solution is just making sure >>> areas are generated with the information that is available anyway. It's >>> not doing anything in a place where it shouldn't do it. >> >> RowPainter deals with rows, not cells. All it does is select the cells >> for which it’s time to create areas, then it delegates to TableCellLM >> the actual area creation. With the change we’re discussing there are now >> two places where areas for cells are created: in TableCellLM /and/ in >> RowPainter. This is redundant, confusing and increases the maintenance >> code. > > Hmm... maybe an idea to consolidate those parts into a CellPainter, > then? Just a vague idea (?) Yes, something like that, probably. > Also an additional object, but for the empty cells, can be tailored to > be as lean/light as possible, and it is only created at the appropriate > time in the process. > The idea I offered earlier (a special type of FONode to represent empty > cells) would still generate completely unnecessary TableCellLMs, and > would be a pest to implement, if I estimate correctly (?) Right. This is another form of redundancy, so not really desirable. > As such, there are always going to be 'unfinished' pieces of code, or > parts that can be improved upon. The base rule remains: if it works, and > it didn't work before, the balance is positive. If we take that simple > rule away, this will in the long run be counter-productive, I think. I have to strongly disagree with that. If it works, and it didn’t work before, /and/ the code is kept as clean, readable and maintainable as before, /then/ the balance is positive. Look: alright, the implementation of missing cells is not optimal, but it’s no big deal. Then we have to fix the current shortcomings of border resolution. Another slightly sub-optimal change, but again no big deal. Then we implement this functionality. And then that feature. Every time a small, maybe-not-optimal-change-but-no-big-deal. And we end up with a completely unmaintainable code base, where the only solution to further implement features is to re-write it from scratch. But I believe I’m repeating myself. Vincent
