Re: [lmi] do_compute_column_widths_if_necessary()

[Vadim Zeitlin]

On Sat, 21 Apr 2018 19:54:27 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-04-21 13:08, Vadim Zeitlin wrote:
GC> > On Sat, 21 Apr 2018 00:57:40 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC>     wxDCFontChanger set_header_font(dc_);
GC> [...]
GC> >  I think the confusion is due to the not very clear local variable name.
GC> > Perhaps it should have been called header_font_setter instead, would this
GC> > clarify its purpose?
GC> 
GC> Yes. Done. I didn't realize that wxDCFontChanger is like wxBusyCursor.

 All the wxSomethingDoer classes are like this, but wxBusyCursor predates
this convention and, anyhow, I'm not sure how could it be made to fit it
without butchering English too horribly.

GC> > GC> I'm also not sure what the name 
do_compute_column_widths_if_necessary()
GC> > GC> means--specifically the "if necessary" part. Is it unnecessary to call
GC> > GC> this function at all in some contexts? Or OTOH is it always necessary
GC> > GC> to call this function, but unnecessary to call it again after it has
GC> > GC> been called once? (I'm guessing it's the latter, and that the flag
GC> > GC> 'has_column_widths_' just means that the function has been called 
once.)
GC> > 
GC> >  Yes, this guess is completely correct.
GC> 
GC> Okay, then this flag is used for optimization: once this function has been
GC> called, it ought to be unnecessary to call it again.

 Well, actually it's always necessary to call it before using the column
widths. However it will only do something non-trivial the very first time
it's called.

GC> But how do we know that's actually unnecessary?

 So, IMO this question is just not formulated correctly because it is never
unnecessary, actually.

GC> AFAICT, it needs to be called after the last call to add_column()...so
GC> wouldn't it make sense for add_column() to assert that this flag is
GC> false, because any later call to add_column() would require calculating
GC> the column widths again?

 Yes, this would be an improvement.

 As usual, I tried to keep this code as simple as possible because this
seems to be more in line with your preferences. Left to myself, I would
probably do something to ensure that columns can't be added after computing
their widths at compile-time, but this would make things more complicated
and so I don't think you'd agree. But while a run-time check is not at all
as good as a compile-time one, it's still better than nothing, I guess.

GC> And how do we know where to call the function that sets column widths?
GC> It's called in
GC>   do_get_cell_x()
GC>   output_horz_separator()
GC>   output_header()
GC> all of which access elements of 'columns_', but not in these other
GC> functions
GC>   cell_rect()
GC>   do_output_values()
GC> that also access columns similarly.

 Well, the trivial answer to this question is that cell_rect() calls
do_get_cell_x() and do_output_values() calls cell_rect(), so it's not
necessary to call do_compute_column_widths() again in them. But yes, this
is not imposed in any way.

GC> How can we be sure it's called exactly when necessary?

 I can write a prototype doing just this, but it would involve using
separate classes and move semantics and I was rather discouraged by your
reaction to my previous attempts to do it like this, so I'm not sure if
it's really going to be useful... would it?

GC> Clearly it can't be called in the ctor, before add_columns() is ever
GC> called. Would it make sense to add an accessor like
GC> 
GC>     std::vector<column_info> get_columns()
GC>     {
GC>     do_compute_column_widths(); // Only called in this one place.
GC>     return columns_;
GC>     }
GC> 
GC> and use it everywhere do_compute_column_widths() is now explicitly
GC> called?

 Copying vector like this doesn't seem like a good idea, even if it's
small, but we could indeed use a get_columns_with_widths() accessor
returning a const reference after ensuring that the widths had been
computed.

 Generally speaking, I'm not convinced that such half-hearted attempts at
making it impossible to use the column widths before computing them are
worth it. Nothing of the above would prevent writing the code accessing
columns_ directly, we'd need to encapsulate it into its own class instead
of just using a std::vector<> for this. And then we could just provide
get_column_width(size_t col) method that would return the width of the
given column after ensuring that the widths had been computed. I'm not sure
if it's really worth doing it here, after all the code is quite simple and
only this class can use columns_, so it's closed, but if you think that the
current version is too unsafe, then I'd rather do it by properly
encapsulating everything rather than doing something in the middle between
the current (as simple as possible) code and safer version using multiple
classes.


GC> > it is very convenient to be able to pass the same values to
GC> > output_row() whether all of them are supposed to be shown or not.
GC> > This is not especially relevant for the illustrations generation
GC> > currently because everything is static (i.e. no columns are hidden
GC> > depending on some run-time conditions), but it allows
GC> > group_quote_pdf_generator_wx code to be simpler and more natural than
GC> > it would have otherwise been. I'd very much prefer to leave it like
GC> > this rather than having to carefully construct the proper vector,
GC> > matching the currently shown columns, to pass to output_row().
GC> 
GC> Can you provide more context? I'm still having trouble understanding this.
GC> I understand that some group-quote columns are hidden (there's one large
GC> set of columns that are potentially shown, and some are suppressed at
GC> run time, e.g. if their contents are all zero). But AIUI either a column
GC> is hidden (and all its rows are hidden), or shown (and all its rows are
GC> shown). I'm supposing, perhaps incorrectly, that there comes a moment
GC> when we know which columns are hidden (at which point we might simply
GC> remove them from the vector of columns),

 There is no vector of columns. There is a vector of rows, where each row
is an array of values for the given row. We could, in principle, go through
this vector and filter out the values that need to be hidden from each row
(which would need to become a vector and not a simple array instead). But
this is clearly less convenient than just always putting all the values in
the row values in group_quote_pdf_generator_wx::add_ledger() and then
deciding which of the columns do we want to show or not when calling
output_row() in save() later (notice that we don't know which columns
should be shown and which one shouldn't in add_ledger() itself yet).

 Alternatively, think of it in this way: we have some UI-independent data
and we have UI layer (PDF writer in this case). It is usually better to
keep data in its natural form (which means including all the columns here)
and then configure the UI to show it in the appropriate way (by hiding some
of the columns) rather than massaging the data into some special format
suitable for UI.

 Of course, it could be done in either way, but I see no reason at all to
change the current code and even if I were writing it again, I'd still do
it like this because it seems much more natural to me. Could you please
explain what problem do you have with doing it like this?

 Thanks,
VZ