[lmi] Class wx_table_generator [Was: master 783f4dd 5/9: Consolidate and revise documentation]

[Greg Chicares]

On 2018-04-28 16:39, Vadim Zeitlin wrote:
> On Sat, 28 Apr 2018 01:22:34 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> Every time I open 'wx_table_generator.cpp' I go back to this nested class
> GC> to refresh my still-evolving understanding.
> 
>  I think we have a real problem with this class and no amount of
> documentation is going to change this because there must be something
> fundamentally very wrong with it to require spending so much time and
> effort on something so trivial. I don't know how to solve it though because
> while I see that this class might not be very elegant, I don't see how to
> avoid it considering the requirements we have for the different tables in
> the group quotes and the illustrations and, most of all, I just don't see
> any complexity in it: it's just information about a column table, including
> its title, width and whether this width is computed dynamically or is
> fixed.
> 
>  Sometimes I think that maybe all my attempts to explain it only make
> things worse because it's really that simple.
> 
>  What I really wish to know is what would you do instead.

Yes, I think the best way forward is for me to propose something concrete.
And the best way for me to do that is to step away from it for perhaps a
few weeks, then return with a fresh perspective. For now, let me ask some
very low-level questions, just to make sure I understand everything class
wx_table_generator does.

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

Are horizontal and vertical separators regarded as occupying zero width?
I believe they are, as they are not counted when determining alignment or
total width. I.e., they aren't infinitely narrow, but they're drawn only
in empty space where no character happens to be printed. E.g., here:
     do_output_horz_separator(left_margin_, x, *pos_y - 1);
     do_output_horz_separator(left_margin_, x, *pos_y    );
the lines seem to be about one pixel thick, because drawing two of them one
pixel apart gives the appearance of a single thicker line; and they don't
collide with any text merely because we draw them within the interline
spacing.

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

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

Lastly, I have a very general question that this function illustrates:

    wxRect wx_table_generator::cell_rect(std::size_t column, int y)
    {
        LMI_ASSERT(column < all_columns().size());
    ...
        return wxRect(x, y, all_columns().at(column).col_width(), row_height_);

Here, at() seems unnecessary, because the range was already tested above.
But you use at() very often. Do you prefer at() to operator[] in all cases,
because it replaces a segfault with orderly termination? Or do you just
write it by default in untested new code, intending to go back and replace
it with operator[] after testing?