Re: palette test? [Re: patchset #1448674]

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

On Tuesday 14 March 2006 08:34 am, Daniel J Sebald wrote:
> 
> 1) Is this test from color.c complete?
> 
>     if (save_pal.colorFormulae < 0
> 	|| sm_palette.colorFormulae != save_pal.colorFormulae
> 	|| sm_palette.colorMode != save_pal.colorMode
> 	|| sm_palette.formulaR != save_pal.formulaR
> 	|| sm_palette.formulaG != save_pal.formulaG
> 	|| sm_palette.formulaB != save_pal.formulaB
> 	|| sm_palette.positive != save_pal.positive
> 	|| sm_palette.colors != save_pal.colors) {

I think it is not complete.
It also has some strange tests (e.g. so far as I can see,
colorFormulae can never be anything but 37).


> 2) Would it make sense to replace the above lines of code in color.c with the function
> 
> palettes_differ(t_sm_palette *p1, t_sm_palette *p2)

You mean, replace it for the purpose of printing a message?  I don't know.

But I think this code has some problems in it right now:

>    save_pal = sm_palette;

copies current palette to save_pal, including pointers

>   if (sm_palette.color != NULL) {
>        free(sm_palette.color);
>        sm_palette.color = NULL;
>    }

Bad!  Now save_pal contains a pointer to an color array
that has just been freed.

>    sm_palette.color = gp_alloc( sm_palette.colors * sizeof(rgb_color),
>                                 "pm3d palette color");
>    /*  fill sm_palette.color[]  */
>    for (i = 0; i < sm_palette.colors; i++) {
>        gray = (double) i / (sm_palette.colors - 1);    /* rescale to [0;1] */
>        rgb1_from_gray( gray, &(sm_palette.color[i]) );
>    }

OK. We allocate and fill in a new color array.
But we cannot compare it to the old one, because that one
was freed prematurely.

>    /* let the terminal make the palette from the supplied RGB triplets */
>    term->make_palette(&sm_palette);

This is where we would like to implement Daniel's proposed
non-redundancy test:

    if (palettes_differ(&sm_palette, &sav_pal))
        term->make_palette(&sm_palette);

But in order to do that, the memory allocate/free needs to be cleaned up.
Also as we discussed, there needs to be a separate test for whether this
is the same terminal as we sent the previous palette to.

Further, Daniel has worried that some terminals might require reloading
the palette every time, even if it *is* the same.  I don't think that is
the case for any real terminal. Does anyone know differently?

What do you think?  It seems to me that this would be a much smaller
and cleaner change than Daniel's patch #1448674, and it fixes the use
for all terminals, not just x11.


-- 
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742