Re: [ gnuplot-Patches-1191202 ] Fix multiplot problem with epslatex terminal

[Ethan M. <merritt@u.washington.edu>]

> >Comment By: Hans-Bernhard Broeker (broeker)
> Date: 2005-07-19 14:04
> 
> The idea of a multi-function term->private is flawed, IMHO.
>  Even the name is, to some extent.  I had reasons to suggest
> calling the one being invented here term->startlayer() instead.

That is essentially what I proposed originally, and I understood
you to be opposed to it at that time.
Here is your earlier response:
 
On Thu, 10 Mar 2005 
Hans-Bernhard Broeker <br...@ph...> wrote
> Ethan Merritt wrote:
>
> > How about a terminal entry point (*term->layer)((int) layernumber) 
> > that the core code would call at each stage of the plot process, once
> > per layer?

> What I'm a little bit worried about is, as always, the conceptual
> clarity of our terminal layer API.  So far, the terminal API is,
> essentially, just a rather generic vector drawing API --- all the way to
> the old "gnuplot library" which exported this API for generic usage.
> A new API entry like the above would, effectively, specialize the
> interface to gnuplot as the client, and kill all remaining hopes of one
> day splitting up the program into well-separated layers of functionality.

So now I am rather confused as to your actual preference.

> Trying to cram all "terminal-specific somethings" into a
> single function is not API design --- that's outright
> refusal to actually _do_ some interface design.  If some
> terminals may conceivably have their own special way of
> handling a particular job, then the gnuplot way to tell the
> core about that is by exporting a non-NULL terminal API
> entry.  

Here again I would appreciate it if you could clarify your
thoughts on the tradeoffs that inevitably arise when introducing
new options.  In recent posts you have expressed great
concern about code bloat and the resulting size of the gnuplot
binary.  Since every new terminal API call introduces a function
pointer slot into ~50 driver entry tables, I would have thought you
to oppose proliferation of new terminal API functions. And to
mitigate this by conditionally coding each one would require a massive
pass over all terminal drivers to include the appropriate conditional
code blocks in the terminal entry table.

But if that's the way you prefer to go, then I see an argument for
at least three new API calls to cover existing use:

 /* This is the one that triggered the discussion.
  * Used by epslatex for front/back text.
  * Might be used by other terminal types if it were available
  */
 term->layer(int layer);

 /* Used by postscript to flag code sections for postprocessing by awk.
  * Also requested multiple times as an enhancement for storing plot
  * info (scaling, axis origin) in bitmap output formats
  */ 
 term->comment(TBOOLEAN global_or_inline, const char *comment_text)

 /* Used by post.trm to optimize the color box (called from color.c)
  * Could be generalized to draw arbitrary rectangles with gradient
  * fill.
  */
 term->gradient_fill(int xl, int xh, int yl, int yh, struct gradient *g)

> The only remaining question then is whether the 
> default, non-special version of that function then becomes a
> default terminal API handler (like do_arrow() or do_point()
> in term.c), or the core code gets to look like
> 
>    if (term->do_special_job_x)
>      term->do_special_job_x(parameters);
>   else
>      default_handler_for_job_x(parameters);


Yes, exactly.  That is functionally how the current code in color.c
works, but the routines are not wrapped as terminal entry calls.
It has a generic default routine for any terminal that supports
pm3d filled rectangles: 
    draw_inside_color_smooth_box_bitmap()
and a special case for the postscript driver:
    draw_inside_color_smooth_box_postscript()

As it happens, we have a bug report outstanding that the colorbox
produced by this postscript special case code does not display 
properly in recent ghostscript/ghostview versions. So another
option is to get rid of it altogether, assuming the generic routine
doesn't suffer the same problem.  How much optimization is it worth
just to draw the colorbox?


		Ethan


> ----------------------------------------------------------------------
> 
> Comment By: Ethan Merritt (sfeam)
> Date: 2005-07-19 02:15
> 
> Message:
> Logged In: YES 
> user_id=235620
> 
> I think you are kind of missing the point of this new
> mechanism.  Your change to color.c still leaves
> PostScript-specific code there; it just changes the output
> command from "sprintf(..." to "term->private(...".   That is
> not an improvement IMHO.  The way to do this correctly would
> be something like:
> 
>     if (term->private)
>         (term->private)(TERM_PRIVATE_DRAW_COLORBOX);
>     else
>         draw_inside_color_smooth_box_bitmap()
> 
> And then move the original special case PostScript routine
> draw_inside_color_smooth_box_postscript entirely into the
> postscript driver.
> 
> However,  this doesn't work yet for a couple of reasons.
> 
> (1) In this particular case we really need to know more than
> the fact that the current terminal supports *some*
> term->private operation; we need to know that it
> specifically supports this one.  So I think we need to
> modify term->private to return success or failure.
> 
> (2) In this particular case we actually need to pass 4
> parameters (cb_x_from cb_x_to cb_y_from cb_y_to).  This
> points up the possibility that term->private should take a
> generic pointer-to-structure as a 2nd parameter rather than
> the (const char *) in my version.  I have a feeling that
> would be a headache to get past all the compilers we have to
> deal with, but it is almost certainly more portable than
> your suggestion to use va_dcl and varargs.
> 
> So I welcome your help in polishing this further, but I
> think we have some more work before the code in color.c can
> be handled cleanly.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Harald Harders (harders)
> Date: 2005-07-19 01:41
> 
> Message:
> Logged In: YES 
> user_id=207272
> 
> Your patch is a good improvement. But it still has
> postscript-specific code in color.c which also could be
> avoided. Have a look at the new version. 
> - It makes term->private even more universal by adding
> variable parameter lists.
> - gppsfile is removed from term_api.h since it is only used
> in term.c, now.
> 
> Please have a look at the source code where I have written
> "HH: Comment by Harald Harders" (term.c and post.trm). The
> return value of the function PS_RememberFont is lost since
> the pointer points to a local variable. If the contents is
> still there it is by accident.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Ethan Merritt (sfeam)
> Date: 2005-07-18 21:07
> 
> Message:
> Logged In: YES 
> user_id=235620
> 
> I think I will let the dust settle a bit more before
> applying this, but have a look at this revised version that
> deals with the postscript-specific code in pm3d as well.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Harald Harders (harders)
> Date: 2005-07-15 20:44
> 
> Message:
> Logged In: YES 
> user_id=207272
> 
> Ethan's patch works fine. I have applied some modifications:
> - The 'test' command did not use the LaTeX \gplbacktext
> command. This has been added.
> - term/README missed a description of the (*private)
> function. Fixed.
> - The constants TERM_PRIVATE_0 etc. weren't too descriptive.
> They may likely leed to confusion within short time. I have
> renamed them so that they are called what they do. If
> another terminal will need similar things it just can use
> the same constants. If new functionality will be needed,
> another constant may be added.
> - The function EPSLATEX_private used an int as parameter. I
> have changed it to t_termprivate which it was before, too.
> 
> In my opinion, this patch can go to CVS.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Harald Harders (harders)
> Date: 2005-07-14 09:39
> 
> Message:
> Logged In: YES 
> user_id=207272
> 
> At a first glance, the new patch looks good. I have not yet
> been able to test it. The 'private' function should be
> mentioned in term/README.
> 
> By the way: 'if (term->name == "epslatex")' was caused by
> programming C++ the whole day. ;-) Of course it's nonsense in C.
> 
> I also think that providing <terminal>_put_text() the
> information if text is front or back could be a good
> approach to avoid any terminal-specific code in graphics.c.
> But it appears to be a large change in the code.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Ethan Merritt (sfeam)
> Date: 2005-07-14 07:27
> 
> Message:
> Logged In: YES 
> user_id=235620
> 
> I have reverted the non-working code in cvs and regenerated
> a cleaned-up version of the previous patch. This should make
> it much more obvious what is being added, and where.  I've
> renamed the terminal API function from "sync" to "private".
>  Is that better?  I really don't care much what it's called.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Hans-Bernhard Broeker (broeker)
> Date: 2005-07-13 18:41
> 
> Message:
> Logged In: YES 
> user_id=27517
> 
> While I'm not particularly fond of calling it "sync", I find
> Ethan's patch much more easily agreeable than the previous
> ones.   I think it should go in like that, possibly renamed
> to "start_layer" or similar (layer stopping shouldn't be
> needed --- it's implicit in closing the plot via
> term->text() or a change to some other layer).
> 
> ----------------------------------------------------------------------
> 
> Comment By: Ethan Merritt (sfeam)
> Date: 2005-07-13 07:47
> 
> Message:
> Logged In: YES 
> user_id=235620
> 
> Neither the code currently in cvs nor the code in this patch
> actually *does* anything, because the test condition
>    if (term->name == "epslatex")
> is totally bogus.  Given that no-one has even noticed this
> non-functionality, I'm still not convinced that the whole
> idea is worth trampling on the core/terminal code layering.
> 
> Nevertheless, if you really want to pursue it, please have a
> look at the patch I have just uploaded.  It implements a new
> terminal API call as discussed previously on the mailing
> list.  I'm not thrilled about this API, since it's used only
> by epslatex, but at least it keeps the code in the core
> routines perfectly generic.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Harald Harders (harders)
> Date: 2005-05-29 20:01
> 
> Message:
> Logged In: YES 
> user_id=207272
> 
> I agree that terminal-specific code should be avoided in
> graph*.c. But in my opinion, this patch already is an
> improvement because it avoids the usage of terminal specific
> variables in these files. With this patch, gnuplot can be
> compiled without using post.trm.
> 
> If the routines <terminal>_put_text() knew if the printed
> text is 'back' of 'front' it would be easy to remove all
> terminal-specific code from graphics.c and graph3d.c. But at
> the moment, this is not the case.
> 
> Does anybody have an idea how to remove all
> terminal-specific code from non-terminal-specific files? By
> the way: pm3d.c also contains terminal-specific code (all
> lines containing the variable gppsfile).
> 
> ----------------------------------------------------------------------
> 
> Comment By: Ethan Merritt (sfeam)
> Date: 2005-05-17 18:04
> 
> Message:
> Logged In: YES 
> user_id=235620
> 
> I would still much rather see a fix that removed all
> epslatex-specific code from graphics.c and graph3d.c.  This
> patch would introduce yet more terminal-specific code, IMHO
> making things worse rather than better.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Petr Mikulik (mikulik)
> Date: 2005-05-17 17:28
> 
> Message:
> Logged In: YES 
> user_id=31505
> 
> This patch applies OK, can it go to cvs now? 
> 
> ----------------------------------------------------------------------
> 
> You can respond by visiting: 
> https://sourceforge.net/tracker/?func=detail&atid=302055&aid=1191202&group_id=2055
> 

-- 
Ethan A Merritt       merritt@u.washington.edu
Biomolecular Structure Center
Mailstop 357742
University of Washington, Seattle, WA 98195