Re: Optional support for GDI+ on Windows (emacs-28)

[Eli Zaretskii <eliz@gnu.org>]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Tue, 31 Mar 2020 17:35:30 +0200
> 
> > Thanks.  Was this version supposed to take care of the review
> > comments to the previous one?  If so, perhaps you sent the wrong
> > patch, because it looks like all the issues are still there in this
> > version.
> 
> Regarding your comments:
>  
> * I do not know what is supposed to replace the code regarding search
>   for terminal background colors. If this code is wrong, it is also
>   wrong in the code that uses the PNG library (src/image.c).
>       if (STRINGP (specified_bg)
> 	  ? FRAME_TERMINAL (f)->defined_color_hook (f,

This resolves to w32_defined_color.

>                                                     SSDATA (specified_bg),
>                                                     &color,
>                                                     false,
>                                                     false)
> 	  : (FRAME_TERMINAL (f)->query_frame_background_color (f, &color),
>              true))

And this resolves to w32_query_frame_background_color.  See w32term.c
around line 7250.  Like I said, image.c is a largely
platform-independent code, whereas w32image.c isn't, so calling
w32-specific functions directly is OK.  But this is not an important
comment, so if you want commonality with image.c, I won't object.
It's just slightly sub-optimal, because each call needs to dereference
a function pointer.

> * Regarding the use of WCHAR in filenames, because this component only
>   works when GDI+ is linked in, w32_unicode_filenames is always 1 and
>   there is no need for additional translations.

w32_unicode_filenames is exposed to Lisp and can be set to nil at
runtime.  Only its default value is guaranteed to be non-nil on modern
Windows platforms.  My question was what to do about that, since in
general we aren't supposed to use Unicode file names when that
variable is nil.

Btw, I just now spotted a more serious problem with w32_load_image: it
should encode the file name before it calls filename_to_utf16.  (The
same problem exists in ns_load_image, AFAICT.)  You will see that
png_load_body, for example, encodes the file name it receives from the
caller, before using it in libpng calls.  In general, we cannot pass
the internal Emacs representation of file-name strings to system APIs.

> * Regarding the use of pointers to string data, once more, I am using
>   the same code that is used in the image.c file to extract the image
>   data from the user's input. If that code is wrong, then it is so in
>   the PNG driver
>         /* Read from memory.  */
>       tbr.bytes = SDATA (specified_data);
>       tbr.len = SBYTES (specified_data);
>       tbr.index = 0;
>   in the JPEG driver
>       jpeg_memory_src (&mgr->cinfo, SDATA (specified_data),
> 		     SBYTES (specified_data));
>   in the GIF driver, etc, etc.

You are missing my point, but it isn't worth arguing about it, so
let's drop this part.

> * Regarding stylistic conventions, all have been fixed.

Not all: some spaces are still missing between the function name and
the opening parenthesis.  One example:

      status = GdiplusStartup(&token, &input, &output);
                            ^^

Also, some comments still don't leave 2 spaces after the final
period.  Example:

   /* Assume that the image has a property item of type PropertyItemEquipMake.
      Get the size of that property item. */
                                        ^^

And there are still one-line blocks in braces.  Example:

          if (frame < 0 || frame >= frameCount)
            {
              status = GenericError;
            }
          else

> * Regarding the use of HAVE_NTGUI and unconditional removal of
>   PNG/JPEG/etc, it has been replaced with a flag HAVE_GDIPLUS which is
>   optionally selected at configuration time with --with-gdiplus, which
>   defaults to NO.

Like I said: this must not be a compile-time condition, we should
decide whether GDI+ is supported at runtime, and we should provide
variables to control whether GDI+ is used for each supported image
format.  HAVE_GDIPLUS should guard code which uses GDI+, but it should
NOT decide whether that code is actually used.

Btw, the same issue is with SHCreateMemStream -- it should be called
through a function pointer that gets populated at runtime after
loading shlwapi.dll, and for the same reason: the function is not
available on all the versions we want to support.

> * The static variable has to be initialized to 0 because it explicitely
>   describes a condition (library has not been initiated) that is
>   meaningful.

Static variables are automatically initialized to zero, you don't need
to do that explicitly (this is a minor nit).

> So, what exactly is missing?

The main issue remains the compile-time decision whether to use GDI+
in your patch, as opposed to run-time decision, and the related
variables, that I'd like to see.  I don't know who told you something
that could be interpreted in the other direction; if that was
something I wrote, please accept my sincere apologies -- I never meant
anything to that effect.

Thanks.