Re: footer navigation headers

[Patrice Dumas]

On Wed, Feb 17, 2021 at 08:25:53PM +0000, Gavin Smith wrote:
> On Sun, Feb 14, 2021 at 11:31:21PM +0000, Gavin Smith wrote:
> > On Sun, Feb 14, 2021 at 12:32:58PM -0800, Per Bothner wrote:
> > > A related problem is that the end-of-page header is relative to the
> > > final @subsection (and duplicates that internal header at the start
> > > of the @subsection).  In my opinion, the header should point to
> > > the next/previous/up of the @section (or @chapter for a chapter
> > > without sections).  This is more logical, I think, whether or not
> > > the @subsecton navigation headers are displayed.
> > 
> > Yes, I think this makes sense for --split=section.  Where there is more
> > than one node per output file it makes no sense to have a navigation
> > bar at the very end of the file that is relative to the last node only.
> > I think there are two sensible possibilities:  A navigation bar at
> > the top and possibly the bottom of the page relative to the @section,
> > or a navigation bar at the beginning of each node in the page.
> 
> I'm typing what I did to try to fix this as I do it to try to help
> other people learn how to fix similar problems like this.  (You shouldn't
> assume it's quick or easy for me to fix.)

Not sure if it is useful, but currently, the design is more to prepare
the directions through the call to 
 Texinfo::Structuring::elements_directions($self, $elements);
and only use then in Texinfo::Convert::HTML.  That could be much easier
for the implementation, to setup a new direction, for instance named 
'FirstInPageUp' and use that through SECTION_BUTTONS and similar.  I
think another reason to do it like that is to avoid formatting functions
needing to use the tree directly, such that it is easier to customize
the output.  The drawback is that there need to be a direction setup for
every conceivable use case.

> I created a minimal failing example:
> 
> \input texinfo  @c -*-texinfo-*-
> 
> @node Top
> @top top
> 
> @node chap1
> @chapter chap1
> 
> @node sec1
> @section sec1
> 
> @node chap2
> @chapter chap2
> 
> @node  sec2
> @section sec2
> 
> @bye
> 
> Procesing with ./texi2any.pl --html --split=chapter test.texi
> gave spurious warnings:

These are not spurious, the nodes cannot be reached as they are not in
any @menu nor in node directions.  It could be possible to remove this
warning if FORMAT_MENU is set to anything else than menu, though.

> test.texi:9: warning: node `sec1' unreferenced
> test.texi:15: warning: node `sec2' unreferenced
> 
> The output chap1.html had the problem you mentioned, with a link to chap1
> at the end of the file.
> 
> The next step was to find where this text was output.  I inserted print
> statements into parts of the code and ran the program several times
> to confirm this.  It is useful to use the print_tree function to check
> some structures.  This showed that the problem appeared before
> _default_format_element_footer was called, as the $element argument
> was the section, and not the chapter.  

As you found out later, $element is not a node nor a section, it is an
element, that in general contains both the node and the associated
section (and also may contain more than one section if not associated
with nodes).  This abstraction was needed to be able to handle in a
similar way the two cases of output driven by the nodes (Info format,
makeinfo in C HTML) and output driven by the sections (texi2html
default, HTML generated now).

> I had to confirm where it was
> being called, which I did by inserting more print statements.  I found
> that _default_format_element_footer was called several times and the
> footer was only output for some of them (when $end_page was set in
> that function).  It showed that I didn't understand what
> _convert_element_type was doing, as it looked like it was running once
> for each node, not for each output file (the comment at the top of
> the function, which I am sure that I wrote to help me to understand the
> code, says otherwise).

It is called for each 'element', which contains, in the normal classical 
case both the node and section.  This is documented in
Texinfo::Structuring, in particular split_by_node, split_by_section (and
split_pages) documentation.

As a side note, there is no real documentation of the HTML "API" as a
whole in particular because I was never convinced that the current state
is a good design.  This design is very much driven by the requirement to
have all texi2html customization possibilities in texi2any, and was not
really designed for itself.

> I was ready now to attempt changing the code.  I would have to
> be careful about the different configuration options (split v. non-split
> etc.).  I added an argument to the function, $supreme_element.  Now
> I went looking for code I could copy and paste to use this argument.
> I thought of calling _default_format_element_footer directly but had
> the thought that this function might be called twice for the top-level
> element and it wouldn't be able to distinguish between them.  At this
> point I took a break.
> 
> I thought that perhaps we could just call _default_format_element_footer
> once from the new location, as that function had a lot of logic in it.
> To minimize disruption, I thought that I could at first pass the wrong
> value of $supreme_element to it (the last element in a file, not the
> first).
> 
> I commented out the existing call to &{$self->{'format_element_footer'}}
> and copied the code to _default_format_end_file.  I saw there were
> $type and $content variables which weren't available in the new location.
> Checking, I saw the $type parameter wasn't used at all.  The $content
> was used for the WORDS_IN_PAGE feature.

I guess that the variables need to be there even if not used to have a
common prototype and a regularity in customizable functions available
informations.  I have not checked, but the type argument could be used
in customization functions.  I think that it contains the type from the
tree, for an element is should probably be 'element', except maybe for
special elements.  It is available in case the customization function
would need it, but may not be useful in the default formatting function.

>  This feature has confused many
> people in the past, and I went back to my old emails to see people talking
> about this.  I thought for a first implementation, I would just enable
> the footer unconditionally (everything that's been output to that
> output file not being easily available - the existing code counting the
> words in the last node only, I assume).

Indeed.

> Although trying to make minimally intrusive changes to the code, the
> changes were piling up.  I did a "git diff" to review.  After I removed
> old debugging print statements (with "git checkout --patch" (in fact, I
> have a bash alias for this, "GCO -p")), the changes were as follows:
> 
> ff --git a/tp/Texinfo/Convert/HTML.pm b/tp/Texinfo/Convert/HTML.pm
> index 2b6929b3a..b0bca385c 100644
> --- a/tp/Texinfo/Convert/HTML.pm
> +++ b/tp/Texinfo/Convert/HTML.pm
> @@ -4744,8 +4744,8 @@ sub _convert_element_type($$$$)
>      }
>    }
>    $result .= $content unless ($special_element);
> -  $result .= &{$self->{'format_element_footer'}}($self, $type, 
> -                                                 $element, $content);
> +  # $result .= &{$self->{'format_element_footer'}}($self, $type, 
> +  #                                                $element, $content);
>    return $result;
>  }
>  
> @@ -4803,14 +4803,14 @@ sub _default_format_element_footer($$$$)
>    } elsif ($self->get_conf('SPLIT') eq 'node') {
>      if ($self->get_conf('HEADERS')) {
>        my $no_footer_word_count;
> -      if ($self->get_conf('WORDS_IN_PAGE')) {
> +      if (0 or $self->get_conf('WORDS_IN_PAGE')) {
>          my @cnt = split(/\W*\s+\W*/, $content);
>          if (scalar(@cnt) < $self->get_conf('WORDS_IN_PAGE')) {
>            $no_footer_word_count = 1;
>          }
>        }
> -      $buttons = $self->get_conf('NODE_FOOTER_BUTTONS')
> -         unless ($no_footer_word_count);
> +      $buttons = $self->get_conf('NODE_FOOTER_BUTTONS');
> +         #unless ($no_footer_word_count);
>      }
>    } else {
>      $maybe_in_page = 1;
> @@ -6534,9 +6534,16 @@ sub _default_format_program_string($)
>    }
>  }
>  
> -sub _default_format_end_file($)
> +sub _default_format_end_file($;$)
>  {
>    my $self = shift;
> +  my $supreme_element = shift;
> +
> +  if ($supreme_element) {
> +    $result .= &{$self->{'format_element_footer'}}($self, undef,
> +                                                   $supreme_element, undef);
> +  }
> +
>    my $closing_sections_text = join('', 
> $self->close_registered_sections_level(0));
>    my $program_text = '';
>    if ($self->get_conf('PROGRAM_NAME_IN_FOOTER')) {
> 
> I still hadn't dealt with passing $supreme_element to
> _default_format_end_file.  I added this to the call to
> &{$self->{'format_end_file'}} (using the most recent node processed),
> and then figured it was time to try running the texi2any test suite,
> hoping that there weren't too many changes.
> 
> The quickest way, I've found, is to run "maintain/all_tests.sh generate"
> followed by "git diff".

I prefer to do make check followed by ./maintain/all_tests.sh diff.

> This led to syntax errors which I fixed (there was no such variable as
> $result in _default_format_end_file).
> 
> Unfortunately, this led to a massive amount of changes in the test results.
> At this point, I was out of steam for the evening.

I think that format_end_file is only supposed to format the very end of
the file, after the footer.  So it is probably not the place to call
format_element_footer differently.  It would probably be better to call
differently directly 'format_element_footer' in _convert_element_type?

One way to find the supreme element may be by following
element_prev until element->{'filename'} ne 
element->{'element_prev'}->{'filename'}.

I checked how the element is passed down to the formatting function
(which is format_button), and it is $self->{'current_element'}.
So, maybe a way to achieve what you want to do would be to use something
else, like $self->{'header_direction_element'} in format_button, set for
the header to 'current_element' and for the footer to the supreme
element.  As I said above, another possibility would be to setup a new
direction in and use it in the footer buttons one way or another,
instead of using another element than the current element in
format_button.

> If/when I come back to this, I'll be investigating why <hr> disappeared
> from the output in many places, before looking at the other changes in
> the test results.

-- 
Pat