Re: dumb stacking page breaker

[Joe Neeman]

On Thursday 12 July 2007 07:07, Nicolas Sceaux wrote:
> Joe,
>
> Here is a patch where a stacking-page-breaking function is
> implemented. May you review it, and tell me if there is something wrong
> or some bad coding, before I go on with documentation and regression
> tests? The first tests I have made so far are satisfying. (Also
> suggestions for a better name are welcome:)

The basic idea behind the inheritance in the page-breakers is that the 
Page_breaking class looks after the systems and handles 
compression/decompression, demerits, etc. The basic operation of a child 
class is:

Choose a starting and ending breakpoint and a number of systems. Call 
Page_breaking::set_current_breakpoints (or set_to_ideal_line_configuration) 
and the Page_breaking class will set some state. In general, there will be 
more than one line configuration that fits the constraints, so the child 
class can run a bunch of querys on the various configurations (eg. finding 
the minimum number of pages, spacing the systems in various ways).

The child class only specifies the index of the configuration and the 
Page_breaking class does the rest, including caching things for the case when 
the child class runs several different querys on the same configuration 
(except that the caching is broken, see below).

To fit with this, I'd suggest that you move 
Stacking_page_breaking::compute_page_breaks to something like 
Page_breaking::pack_systems_on_least_pages and change its interface to match 
the other system-spacing stuff (ie. take a configuration_index instead of the 
lines directly and return Spacing_result instead of just the systems per 
page). Then you can return the uncompressed/compressed line stuff to private 
within Page_breaking.

The looping logic in compute_page_breaks seems to me more convoluted than that 
in Page_breaking::min_page_count. As far as I can tell, though, they do 
exactly the same thing. Could you change the looping logic to match 
min_page_count? Also note that your last "for" loop is unnecessary. As per 
the comment in min_page_count, you can do
if (things don't fit on the last page)
{
  systems_per_page.back ()--;
  systems_per_page.push_back (1);
}

To do the "things don't fit on the last page," it might be worthwhile to add a 
Page_spacing::resize_page(Real new_height) method. Then we could reuse it in 
Page_breaking::min_page_count.

+  /* what is the purpose of cached_configuration_index_? it is never set,
+     save in clear_line_details_cache -- ns */

Good point, it should be set in the following lines after the comment:
if (cached_configuration_index_ != configuration_index)
  {
    cached_configuration_index_ = configuration_index;
    ...

Joe