[Groff] Re: grohtml patches

[Gaius Mulley]

Werner LEMBERG <address@hidden> writes:

Hi Werner,

> 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?

ok, I'll look into this, it would be a neater solution.

> > > 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.

ok, I'm not sure this is as easy as clean as it sounds. Maybe it would
require quite a few support methods which effectively duplicate
libgroff ?

> 
> 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().

ok, again I'll examine C++ in more detail and exploit these code
removal mechanisms :-)


> . 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.
sure

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

ok

> . 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.

ok, yes will do. The debugging code is really useful - I guess we can
pull it out when the grohtml (vertical space bugs are fixed) dust settles.

> . 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.

yes certainly. It is good to find another use for these modifications!


> 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.

Thanks for the bug reports. Yes the vertical spacing is bad, I hope all the
vertical bugs can be solved together,

Gaius