[Groff] Re: grohtml patches

[Werner LEMBERG]

Gaius,


thanks a lot for your new stuff.  I still have a list of items which I
ask you to fix (and a list of new bugs :-).

> sure ok, I've moved round_width into post-grohtml and introduced a
> virtual method `round_width' into the printer class. I've then
> provided an identity round_width method to all device drivers and a
> html specific method to post-grohtml.

Thanks.  I'm not completely happy how it is implemented -- isn't it
possible to provide a default version in libdriver/printer.cpp so that
only grohtml needs additional code?

> > what do you think about adding keywords to the grohtml DESC file
> > to control the name of gs?  As you know, the binary name of gs
> > depends on the platform; there are at least four possibilities
> > according to the gs man page: gs, gswin32c, gswin32, and gsos2.
> > The configure script could then contain some code to automatically
> > adjust that.
>
> . done, ./configure tries to detect where gs lives and updates
>   devhtml/DESC appropriately. It could be improved by introducing a
>   --withgs=/usr/bin/gs option though..

I don't think it is a good idea to add the `image_generator' keyword
to libgroff instead to the grohtml driver.  Which other driver needs
`image_generator'?  It seems best to me to move that code to grohtml.

Some other requests and questions:

. Is it really necessary to make node::is_tag() a pure virtual
  function so that all derived classes have to provide its own
  is_tag() version?  I think you could save some code if you allow
  derived classes to use node::is_tag().

. Please compile with -Wall and fix the order of member initializers.

. `is_a_diversion' is an ugly name.  Please rename it to
  `diversion_flag' or something similar.

. Please remove the stop() function (or put it into #ifdef DEBUGGING).

. It seems to me that the design of diversion handling for HTML
  basically works.  Do you really think that we still need a command
  line option `-D' for troff to debug the HTML troff state?  Then
  please surround related code with #ifdef DEBUGGING also.

. What I like is that you've introduced the
  input_stack::get_div_level() function -- in the troff TODO list you
  can find this:

    Number register to give the diversion level.

  Can you add this?  Since \n[.z] gives the name of the current
  diversion, \n[.Z] would be a nice name to return the diversion level
  (I'm open to other suggestions, of course).  Only two or three lines
  of code are necessary for this, I think.


Here the bug list:

. There is a (new?) problem.  Have a look at section 15.4 of pic.ms;
  you can see

    sh { anything ...

     }

  instead of

    sh { anything ... }

. In section 21.1 of pic.ms, you can see this:

    TEXT     A string enclosed in double quotes.  A double quote
             within TEXT must be preceded by a backslash.  Instead of
             TEXT you can use
               sprintf ( TEXT [, <expr> ...] )
             except after the `until' and `last' keywords, and after
             all ordinal keywords (`th' and friends).

  but I think it should be this:

    TEXT     A string enclosed in double quotes.  A double quote
             within TEXT must be preceded by a backslash.  Instead of
             TEXT you can use

               sprintf ( TEXT [, <expr> ...] )

             except after the `until' and `last' keywords, and after
             all ordinal keywords (`th' and friends).

  similar to other `.DS ... .DE' environments.

. The opposite happens in section 21.2:

    <expr> ::=

      VARIABLE

      NUMBER

      <place> <place-attribute>

      <expr> <op> <expr>

      - <expr>

      ( <any-expr> )

      ! <expr>

      <func1> ( <any-expr> )

      <func2> ( <any-expr> , <any-expr> )

      rand ( )

  The correct rendering would be

    <expr> ::=
      VARIABLE
      NUMBER
      <place> <place-attribute>
      <expr> <op> <expr>
      - <expr>
      ( <any-expr> )
      ! <expr>
      <func1> ( <any-expr> )
      <func2> ( <any-expr> , <any-expr> )
      rand ( )

. [BTW, the big vertical spaces in centered figure captions covering
   multiple lines are still here, but I assume that you know that.]

. A worst case rendering is still the groff_char man page...  I wonder
  which code must be added to groff_char.man to make the HTML output
  acceptable.


    Werner