[lmi-commits] [lmi] master 53446f8 3/5: Import commentary from mailing list

[Greg Chicares]

branch: master
commit 53446f8aa7906daabc879837700bcb616ab01e64
Author: Gregory W. Chicares <address@hidden>
Commit: Gregory W. Chicares <address@hidden>

    Import commentary from mailing list
    
    Copied important parts of last month's mailing-list discussion into
    the source. That discussion was interrupted by external events, and
    importing it as temporary code annotations is a reliable way to make
    sure nothing is overlooked.
---
 wx_table_generator.cpp |  59 ++++++++++++++++++++-
 wx_table_generator.hpp | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/wx_table_generator.cpp b/wx_table_generator.cpp
index 7c7b007..55d5635 100644
--- a/wx_table_generator.cpp
+++ b/wx_table_generator.cpp
@@ -195,28 +195,64 @@ void 
wx_table_generator::do_compute_column_widths_if_necessary()
         else
             {
             total_fixed += i.width_;
+// Total width of all "fixed" columns, including the margins on both sides.
+//
+// This width_ is determined from a mask like "999,999".
+// (We should instead have a global map with the field names as keys
+// and their names and maximal width as values.)
+//
+// It's calculated from the mask, then increased to the header width, if
+// it's greater, and then increased by 2*column_margin_. All this happens in
+// wx_table_generator::add_column().
             }
         }
 
     if(total_width_ < total_fixed)
         {
+// As originally laid out, the table is too wide. Calculate the number
+// of pixels by which it overflows--for the whole table:
         auto const overflow = total_fixed - total_width_;
+// where total_width_ is the width of the page, e.g., 210mm for A4
+// and total_fixed is width of all fixed-width columns, as originally laid out
 
         // If we have only fixed columns, try to make them fit by decreasing
         // the margins around them if this can help, assuming that we can
-        // reduce them by up to half if really needed.
+        // reduce them to the extent necessary.
         if(!num_expand)
             {
+// Also calculate the number of pixels by which it overflows for each column
             // We need to round up in division here to be sure that all columns
             // fit into the available width.
             auto const overflow_per_column =
                 (overflow + num_columns - 1) / num_columns;
+// Now determine whether reducing the margins will make the table fit.
+// If that works, then do it; else don't do it, and print a warning.
+//
+// column_margin_ is the padding on each side of every column, so
+// the number of pixels between columns, as the table was originally
+// laid out, is two times column_margin_--which, as we just determined,
+// was too generous, so we're going to try reducing it.
+// Then this conditional compares
+//   the number of pixels by which we must shrink each column, to
+//   the number of pixels of padding between columns
+// Reducing the padding is a workable strategy if the desired reduction
+// is less than the padding.
+//
+// Is this as good as it can be, given that coordinates are integers?
             if(overflow_per_column <= 2 * column_margin_)
                 {
                 // We are going to reduce the total width by more than
                 // necessary, in general, because of rounding up above, so
                 // compensate for it by giving 1 extra pixel until we run out
                 // of these "underflow" pixels.
+// Defect: the number of pixels separating columns might now be zero.
+// '9' is five PDF pixels wide; do we need, say, two pixels between columns?
+//
+// Suggestion: change the
+//   overflow_per_column <= column_margin_
+// condition to something like:
+//    overflow_per_column <= column_margin_ - 4 // two pixels on each side
+//    overflow_per_column <= column_margin_ - 2 // one pixel on each side
                 auto underflow = overflow_per_column * num_columns - overflow;
 
                 for(auto& i : columns_)
@@ -242,6 +278,27 @@ void 
wx_table_generator::do_compute_column_widths_if_necessary()
                 // we're done.
                 return;
                 }
+// If overflow_per_column is 1, then column_margin_ -= 1
+// "           "          "  2,   "        "           1
+// "           "          "  3,   "        "           2
+// "           "          "  4,   "        "           2
+// The 'underflow' logic shrinks columns by the exact number of pixels
+// to use up all the available width. But the column_margin_ reduction
+// isn't exact due to truncation: when the margin is added (on both sides),
+// is the total of all (margin+column+margin) widths lower than the maximum,
+// so that this is just a small aesthetic issue, or is it too wide, so that
+// not everything fits?
+//
+// Answer:
+// This is an issue of aligning the column text, not of fitting, because the
+// margin is used when positioning the text inside the column width. And the
+// width is correct, so the worst that can happen here is that the text is
+// offset by 0.5 pixels -- but, of course, if we rounded it down, it would be
+// offset by 0.5 pixels in the other direction. So maybe we should write
+//
+//     column_margin_ -= overflow_per_column / 2;
+//
+// just because it's shorter and not necessarily worse (nor better).
             }
 
         warning()
diff --git a/wx_table_generator.hpp b/wx_table_generator.hpp
index 1bbe471..4131324 100644
--- a/wx_table_generator.hpp
+++ b/wx_table_generator.hpp
@@ -161,6 +161,143 @@ class wx_table_generator
     int row_height_;
     int column_margin_;
 
+//   is_centered_ is a member variable, initialized in the ctor
+//   is_hidden() is a member function, whose return value is dynamic
+// Should these really be implemented in those two different ways?
+// Wouldn't it be better to treat is_hidden() the same as is_centered_?
+//
+// Is this a struct only because we want its members to be publicly
+// accessible? But their values can also be changed by clients, and
+// isn't that undesirable?
+//
+// In wx_table_generator::do_output_values():
+//   if(align_right_)
+//   if(ci.is_centered_)
+// it seems that right-alignment is a quasi-global, while
+// center-alignment is a column_info data member. Historically, this
+// evolved because right-alignment was recently added to support
+// illustrations, while center-alignment was already used for group
+// quotes. But when code is complex for "historical reasons", it's
+// natural to ask whether it ought to be refactored for uniformity.
+
+// Under what circumstances might columns be hidden, centered, or clipped?
+//
+// General answer:
+//  In principle, all of those are independent. In practice, fixed width
+// columns are centered and variable width columns are clipped and only the
+// former can be hidden. But I don't think this low level class should impose
+// such high level constraints on its use, which is why it doesn't do it.
+//
+// - is_hidden()
+//
+// Apparently used only for group premium quotes, e.g.:
+//
+//             case e_col_total_face_amount:
+//                 if(!has_suppl_amount)
+//                     // Leave the header empty to hide this column.
+//                     break;
+//                 // Fall through
+//             ...
+//                 header = cd.header_;
+//
+// Some columns are conditionally hidden by should_show_column():
+//
+//     // May be overridden to return false if the given column shouldn't be 
shown
+//     // for the specific ledger values (currently used to exclude individual
+//     // columns from composite illustrations).
+//     virtual bool should_show_column(Ledger const& ledger, int column) const
+//
+// but that technique seems to be orthogonal to is_hidden() and used
+// only for illustration PDFs.
+// --No, it's not orthogonal, should_show_column() is used to decide whether
+// the column label should be left empty, making the column hidden.
+//
+//  - is_centered()
+//
+// This seems to be used only in one place:
+//
+//     if(ci.is_centered())
+//         {
+//         // Centre the text for the columns configured to do it.
+//         x_text += (width - dc_.GetTextExtent(s).x) / 2;
+//         }
+//
+// What exactly does it mean for a column to be "centered"? I think this
+// is a different concept than using "center" alignment for cells in a
+// spreadsheet column, which would give, e.g.:
+//     1
+//   11111
+// --No, it's exactly the same concept.
+// In spreadsheet terminology, almost all our columns are numeric, and our
+// numeric columns are right-aligned. But the function is documented thus:
+//
+//     // Return true if this column should be centered, rather than
+//     // left-aligned. Notice that this is ignored for globally right-aligned
+//     // tables.
+//
+// Is it then the case that:
+//  - for illustration PDFs, all columns are right-aligned, and
+//  - is_centered is used only for group quotes, where it really does
+//    mean the same thing as "center" alignment in a spreadsheet
+// ?
+// Answer: yes.
+//  This is indeed not as lucid as I'd like, but the alternative would to
+// modify the PDF quotes code to align all the columns explicitly, which I
+// preferred not to do as part of illustrations work. Maybe now, that this is
+// merged, it's indeed worth changing this.
+//  OTOH, unlike a spreadsheet, this class doesn't have any notion of numeric
+// or text values, so its align_right() method is still useful to globally
+// configure all columns to be right-aligned. Perhaps we could just add a
+// similar align_centre() method and call it from the group PDF quotes code
+// and continue to handle the variable width columns specially by
+// left-aligning them in any case?
+//
+// Apparently is_centered() always returns true (but is ignored)
+// for illustrations, and this comment inside the function body
+// applies to group quotes only:
+//
+//     // Fixed width columns are centered by default, variable width ones
+//     // are not as long strings look better with the default left
+//     // alignment.
+//
+// What sort of columns are not centered?
+// Answer: Variable width ones (only used by PDF quotes).
+//
+// - needs_clipping()
+//
+// And what sort of columns need to be clipped? As currently implemented,
+// this function is the logical negation of is_centered(), so only columns
+// that are not centered need clipping--but what columns are those? Only
+// the "Participant" column on group quotes?
+// Answer: Currently, yes, as it's the only variable width column.
+//
+// Does this all boil down to...
+//  - left-align and clip the group-quote "Participant" column
+//  - center all other group-quote columns
+//  - ignore all these accessors for illustration PDFs
+// ?
+// Answer: yes.
+//
+// And what does 'is_variable_width_' mean? As implemented, it means
+// that the constructor's 'width' argument was zero at the time of
+// construction. Is that in effect just another way of identifying
+// the "Participant" column?
+// Answer:
+//  No, as with "centered" above, it really means what it says: a variable
+// width column is one whose width is not fixed, i.e. not defined by the
+// widest string that can appear in it, but takes all the available space
+// remaining from the other, fixed width columns (in principle, there can be
+// more than one variable width column, even if currently this is not the
+// case).
+// The fundamental distinction is really
+// between fixed and variable width columns: the latter ones are always
+// left-aligned and need to be clipped, while the former ones are either
+// centered or right-aligned (if align_right() was called) and not clipped.
+// And I think things are reasonably simple seen from this point of view and
+// this is how you're supposed to see them, because it's how this class is
+// used, while the various accessors discussed above are just its
+// implementation details.
+
     class column_info
     {
       public: