Re: Support for background transparency

[Eli Zaretskii <eliz@gnu.org>]

> From: Håkon Flatval <hkon20@hotmail.com>
> Date: Sat, 27 Nov 2021 23:01:39 +0000
> Cc: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> 
> I believe the gui_set_alpha_background​ function in frame.c​ should return some error message
> when the frame does not support 32 bits of depth, but am not sure how to do this best.

Why do we want an error message?  Why not simply do nothing and return
in that case?  Silently doing nothing when the display doesn't support
some feature is perfectly fine, IMO.

> Newbie question: I can't guarantee that this patch works perfectly with all build variations using GDK+Cairo,
> nor in all (desktop) environments where Emacs might be used. How, in general, is Emacs tested before
> release to ensure (most) such edge cases are caught?

We rely on people who track the development code to report any
problems they see.

> I'm currently working on implementing this feature for other XLib-based build configurations too, offline,
> planning to submit another patch for those in the not-so-distant future.

Thanks.

> +  else if (FLOATP (arg))
> +    {
> +      alpha = XFLOAT_DATA(arg);
> +      if (! (0 <= alpha && alpha <= 1.0))
> +	args_out_of_range (make_float(0.0), make_float(1.0));
> +    }
> +  else if (FIXNUMP (arg))
> +    {
> +      EMACS_INT ialpha = XFIXNUM (arg);
> +      if (! (0 <= ialpha && ialpha <= 100))
> +	args_out_of_range (make_fixnum (0), make_fixnum (100));
> +      alpha = ialpha / 100.0;
> +    }

We have XFLOATINT to do both of the above, so you could avoid
repetitions.

Also, instead of signaling an error, how about using clip_to_bounds to
limit the value to the correct range?  Once again, it is perfectly OK
to do that in display-related code, instead of signaling an error.
Moreover, signaling an error could potentially hang Emacs if this
function is called in some context where recovery is not possible.

> +  else
> +    wrong_type_argument (Qnumberp, arg);

IMO, the CHECK_NUMBER test should be up front.

> +  if (alpha != 1.0)
> +    {
> +      // Abort if frame does not support 32-bit color

I don't see any need to abort, see above.

Also, please use C-style comments, /* Like this.  */

> +      gtk_widget_set_visual(wtop, visual);
> +      gtk_widget_set_visual(wfixed, visual);
                             ^^
Our style is to leave one space between the function's name and the
opening parenthesis.

> --- a/src/termhooks.h
> +++ b/src/termhooks.h
> @@ -748,11 +748,13 @@ #define EVENT_INIT(event) memset (&(event), 0, sizeof (struct input_event))
>                                              const char *name,
>                                              const char *class);
>  \f
> -  /* Image hooks */
> +  /* Window hooks */

I'm not sure this comment replacement is for the better.