Re: [lmi] Inversion of control

[Vadim Zeitlin]

On Thu, 13 Sep 2018 00:31:24 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-09-08 15:32, Vadim Zeitlin wrote:
GC> > On Sat, 8 Sep 2018 15:19:41 +0000 Greg Chicares <address@hidden> wrote:
GC> [...]
GC> > GC> Right now, I'm just asking you to tell me whether I'm on the right
GC> > GC> track.
GC> > 
GC> >  Yes, I think so. I.e. the most complicated is usually determining the 
"Fn"
GC> > parts of code and you've already done it.
GC> 
GC> I've written something that does appear to work:
GC>   http://git.savannah.nongnu.org/cgit/lmi.git/commit/?h=odd/pagination
GC> but it seems too ugly.

 The "paginate" class itself looks perfectly fine to me. The only thing I'm
not sure about is whether we really need to separate init() from print(),
i.e. what's wrong with just passing init() parameters to print() and
getting rid of the former entirely?

 But its use in page_with_tabular_report does seem rather too verbose to
me.

GC> I didn't actually do this...
GC> 
GC> > GC>  class page_with_tabular_report
GC> > GC>      :public numbered_page
GC> > GC>      ,protected using_illustration_table
GC> > GC> +    ,private paginator
GC> 
GC> ...because I don't see how I can implement the pagination class's virtuals
GC> as members of class page_with_tabular_report. The reason is that the
GC> tabular-report class has only member functions--it doesn't have data 
members,
GC> so AFAICT an instance has no state.

 But what exactly prevents us from adding it? It's true that originally the
page classes were not supposed to have any state, but it was only because I
thought it would make them simpler and not because if any overarching
philosophy. If it's now simpler to store some state in them, I see nothing
wrong with doing it.

GC> In my 'odd/pagination' branch, I guess that, fundamentally, I was struggling
GC> to invent a closure;

 Yes, you did create one manually.

GC> and that made me wonder whether a functional style would be better.

 The trouble is that we'd need to pass 6 callbacks now, which is too many
IMO. It would be probably better if we had fewer of them, e.g. I originally
thought to have a single function printing either a row or a separator and
not having {pre,post}lude() at all. This would bring down the number of
callbacks to 3 which would make it much more reasonable to use them.

GC> But I'm not at all sure that it would be much different: lambdas
GC> can capture state, but class page_with_tabular_report doesn't have any state
GC> to capture--or, alternatively, the required state isn't the state of a class
GC> instance, but rather the arguments passed to all the member functions.

 Yes, but lambdas have no problems with capturing local variables or
parameters.

GC> But I don't see how I can write a conjectured virtual member like
GC>     void page_with_tabular_report::print_a_data_row() override
GC> that captures the state of a different member function, namely
GC>     void page_with_tabular_report::render() override

 In the "functional style" you don't have a virtual print_a_data_row() to
override in the first place. Instead, you have just a print() function,
which takes the same parameters as paginate::init() takes now as well as a
number of callbacks corresponding to the different virtual methods.

 So the code would basically look like

    print_with_page_breaks
        // This is the replacement of open_page()
        ([&](int year) -> int
            {
            if(0 != year) next_page(writer);
            return render_or_measure_fixed_page_part
                (table_gen
                ,writer
                ,interpolate_html
                ,oe_render
                );
            }
        // This is the replacement of print_a_data_row()
        ,[&](int pos, int year) -> int
        {
            table_gen.output_row
                (pos
                ,visible_values(ledger, interpolate_html, year)
                );
            return pos;
        }
        // Don't pass the callback for close_page() as it can just have
        // a default "do-nothing" value which is all that we need here.
        );

Note that this requires passing position and year as parameter to the
functions and returning the new position from them because they, being
functions and not objects, can't have any state now. But IMHO this is not a
drawback in this case as the state is simple and it's easy to manage it
inside print_with_page_breaks() itself rather than in some object
(functional programming fans would tell you that it's never a drawback, but
I'd be content with a less sweeping statement).

GC> Now that I have something that does have the virtue of actually working, can
GC> you say how it might be reimplemented cleanly?

 I see 2 possibilities:

1. With the current approach, just add the state to the page class itself
   and implement paginator virtual functions in this class.

2. If you prefer not to store any state in the page class and don't mind
   using the lambdas (I don't know why would you, but...), switch to the
   functional approach which would be especially nice if we could reduce
   the number of callbacks.


GC> I have only one question about the original implementation: concerning
GC> its next_page() function, did I guess correctly here...
GC> 
GC>         void open_page        () override
GC>         {
GC>             // "if": guesswork--next_page() seems to have been called 
already.
GC>             if(0 != year_) outer_.next_page(writer_);

 Yes, you did guess correctly. next_page() is always called by
pdf_illustration::render_all() before starting a new page which is
convenient for the simple pages (which were all of them when this code was
initially written) but does admittedly make things slightly more awkward
for the more complex multi-page ones. We could change this by removing the
calls to next_page() from render_all() and ensuring that each render()
implementation starts by calling next_page() but I'm not if it would really
be an improvement.

 Please let me know if you think it would and would like me to actually do
this,
VZ