Re: new frame-parameter "alpha"

[Stefan Monnier <monnier@iro.umontreal.ca>]

> The attached patch adds the window (frame) opacity feature to GNU Emacs.
> The user can control the frame opacity via a frame parameter  'alpha'.
> If you like the code, I'll start to contact major  contributors.

I do not personally care for transparency, and I'm wondering if it's not
better handled by the (compositing) window-manager, but if some people
like this feature, I'm not opposed to it.

A few comments on the code:

> +  (defun set-alpha (alpha &optional frame)
> +    "Set the opacity of FRAME to ALPHA.  First argument ALPHA 
> +should range from 0 (invisible) to 100 (completely opaque).
> +You can also use an floating point number 0.0 to 1.0.
> +When called interactively, prompt for the value of the opacity to set.
> +FRAME defaults to the selected frame.  To get the frame's current
> +alpha value state, use `frame-parameters'."
> +    (interactive "XWindow Opacity (0-100 or 0.0-1.0) : ")
> +    (modify-frame-parameters frame
> +                             (list (cons 'alpha alpha))))

Is this necessary/important?

> +#ifdef HAVE_X_WINDOWS
> +/* Lower limit value of frame transparency.  */
> +
> +Lisp_Object Vframe_alpha_lower_limit;
> +#endif
> +
>  #endif

Only use #ifdef conditional compilation if either it's necessary to get
the code to compile, or for code which is inherently only meaningful for
a particular configuration.  Since Vframe_alpha_lower_limit makes sense
with other GUIs than X11, it should not be protected with #ifdef
HAVE_X_WINDOWS.
 
> +#ifdef HAVE_X_WINDOWS
> +Lisp_Object Qalpha;
> +#endif

Same here and various other places.

> +#ifdef HAVE_X_WINDOWS
> +void
> +x_set_alpha (f, arg, oldval)
> +     struct frame *f;
> +     Lisp_Object arg, oldval;
> +{
> +  double alpha = 1.0;
> +  double newval[2];
> +  int i, ialpha;
> +  Lisp_Object item;
> +
> +  for( i=0; i<2; i++ )
> +    newval[i] = 1.0;
> +
> +  for( i=0; i<2; i++ )
> +    {
> +      if( CONSP (arg) )
> +        {
> +          item = CAR (arg);
> +          arg  = CDR (arg);
> +        }
> +      else
> +        item=arg;
> +
> +      if( !NILP (item) )
> +        {
> +          if( FLOATP(item) )
> +            {
> +              alpha = XFLOAT_DATA (item);
> +              if ( alpha < 0.0 || 1.0 < alpha )
> +                args_out_of_range (make_float (0.0), make_float (1.0));
> +            }
> +          else if( INTEGERP(item) )
> +            {
> +              ialpha = XINT (item);
> +              if ( ialpha < 0 || 100 < ialpha )
> +                args_out_of_range (make_number (0), make_number (100));
> +              else
> +                alpha = ialpha / 100.0;
> +            }
> +          else
> +            wrong_type_argument (Qnumberp, item);
> +        }
> +      newval[i]=alpha;
> +    }
> +
> +  for ( i=0; i<2; i++ )
> +    f->alpha[i] = newval[i];
> +
> +  BLOCK_INPUT;
> +  x_set_frame_alpha (f);
> +  UNBLOCK_INPUT;
> +
> +  return;
> +}
> +#endif

Explain why you do things twice.
Also, note the coding conventions we use and try to reproduce them.
E.g. we place a space before open parentheses but no space after (and
no space before the close parenthesis either).

> +#ifdef HAVE_X_WINDOWS
> +  /* Opacity of the Frame  */
> +  double alpha[2];
> +#endif

The comment should describe what the field holds, so in this case it
should describe *both* entries of the table.

Also in the docstring, don't just use "alpha" but "alpha transparency"
or something like that, for people who don't know what you mean by "alpha".


        Stefan