[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[lmi-commits] [lmi] master 53446f8 3/5: Import commentary from mailing l
[lmi-commits] [lmi] master 53446f8 3/5: Import commentary from mailing list
Sat, 21 Apr 2018 05:51:17 -0400 (EDT)
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
@@ -195,28 +195,64 @@ void
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
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.
+// 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
// we're done.
+// 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?
+// 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).
diff --git a/wx_table_generator.hpp b/wx_table_generator.hpp
index 1bbe471..4131324 100644
@@ -161,6 +161,143 @@ class wx_table_generator
+// 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():
+// 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:
+// // Leave the header empty to hide this column.
+// // 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
+// // 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:
+// // 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.:
+// --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?
+// 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
+// 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.