Re: [lmi] Class wx_table_generator

[Vadim Zeitlin]

On Mon, 30 Apr 2018 23:24:29 +0000 Greg Chicares <address@hidden> wrote:

GC> Most output is written to the DC with DrawText(), but sometimes DrawLabel()
GC> is used instead. Let me explain how I understand this, and ask whether I've
GC> misunderstood anything. DrawText() is the lower-level function, so it's
GC> simple and fast: it just writes a string starting at a point on the canvas.
GC> OTOH, DrawLabel() seems to be an elaborated alternative, added later (it's
GC> never mentioned in 2005's _Cross-Platform GUI Programming with wxWidgets_);
GC> it figures out how to fit text inside a rectangle, and handles wrapping and
GC> alignment. No hard and fast rule governs the choice--e.g., columns can be
GC> centered manually (though that requires a calculation):
GC>     if(ci.is_centered())
GC>         x_text += (ci.col_width() - dc_.GetTextExtent(s).x) / 2;
GC>     dc_.DrawText(s, x_text, y_text);
GC> or by passing a flag (though that requires setting the rectangle):
GC>     dc_.DrawLabel(line, rect, wxALIGN_CENTER_HORIZONTAL);
GC> so we choose whichever is more convenient in each case. Right?

 Yes, absolutely.


GC> Are horizontal and vertical separators regarded as occupying zero width?
GC> I believe they are, as they are not counted when determining alignment or
GC> total width. I.e., they aren't infinitely narrow, but they're drawn only
GC> in empty space where no character happens to be printed.

 Yes again and for exactly the reason above.


GC> In output_header(), we return early if we're only measuring:
GC>     case oe_only_measure:
GC>         *pos_y += max_header_lines_ * row_height_;
GC>         return;
GC> but in that case the special code for bold headers
GC>     wxDCFontChanger header_font_setter(dc_);
GC> isn't taken into account. Does that work by design (i.e., because making a
GC> font bold with
GC>     dc_.GetFont().Bold();
GC> doesn't change its dc_.GetCharHeight()), or by accident (i.e., because we
GC> always happen to have enough space to accommodate a greater font height)?

 It is intentional, but definitely merits a comment, which I didn't write.
The fact is that even when drawing, we will always consume exactly as much
space as returned above, i.e. the height of the font is not taken into
account at all because do_output_values() increments the vertical position
by (fixed) row_height_ and not by dc_.GetCharHeight() or something similar.
So if bold variant of the font were significantly higher than the normal
version (I don't think this is possible, but I could conceivably be wrong),
the code would still be "correct", but the resulting output would have
overlapping rows. Hopefully this wouldn't go unnoticed, however, and so I
don't think it's worth accounting for this (again, IMHO, impossible) case
in the code -- but worth adding a comment explaining this.


GC> Where we use this clipper:
GC>     wxDCClipper clip
GC>         (dc_
GC>         ,wxRect
GC>             {wxPoint{x, y_top}
GC>             ,wxSize{ci.col_width() - column_margin(), row_height_}
GC>             }
GC>         );
GC> what happens if 'ci.col_width() - column_margin()' is negative? Does the
GC> wxDCClipper code force the negative to zero, so that everything is clipped?
GC> Or is that an error that we should prevent, by forcing zero ourselves?

 I honestly have no idea what happens when setting a clipping region with
negative width (I could test it tomorrow, but today I'd just like to send
this reply to answer your question without delay), but I think it would be
better to ensure that it doesn't happen in this code, which we (well, I)
indeed don't do right now, thanks for noticing this.

 In fact, the code in do_compute_column_widths() should ensure that the
column width is strictly greater than the column margin, but I'm not really
sure even of this any more... (I'm afraid that while making this code more
clear for you, you made it correspondingly less clear for me, notably the
use of column_margin() for accessing the value and column_margin_ to modify
it makes things very confusing IMHO). Assuming this is still the case, we
just need to check somewhere that the column initial width wasn't smaller
than column margin, i.e. 1em. I think it would be better to leave you to
add this check to avoid conflicts with your other changes, but please let
me know if you'd like me to do it instead.


GC> Lastly, I have a very general question that this function illustrates:
GC> 
GC>     wxRect wx_table_generator::cell_rect(std::size_t column, int y)
GC>     {
GC>         LMI_ASSERT(column < all_columns().size());
GC>     ...
GC>         return wxRect(x, y, all_columns().at(column).col_width(), 
row_height_);
GC> 
GC> Here, at() seems unnecessary, because the range was already tested above.
GC> But you use at() very often. Do you prefer at() to operator[] in all cases,
GC> because it replaces a segfault with orderly termination?

 Yes (except that an exception doesn't necessarily lead to termination,
although it might). I think the optimization of not checking the index
offered by operator[] can almost never be really justified. The only
situations where I do use it are either something known to definitely be
performance-sensitive or in a simple "for (int n = 0; n < v.size(); ++n)"
loop where the index is really, really certain to be valid.

 BTW, the rationale for having the assert here is that cell_rect() is a
public function and so it's better to throw an exception with a relatively
clear error message from it a.s.a.p. instead of throwing a less clear
(because more generic) std::exception from the private do_get_cell_x()
called from it.

GC> Or do you just write it by default in untested new code, intending to
GC> go back and replace it with operator[] after testing?

 No, not at all. I guess I could do it if profiling showed overhead due to
the use of at(), but this has never been the case so far...

 Regards,
VZ