Re: [PATCH] system-type cygwin with window-system w32

[Eli Zaretskii <eliz@gnu.org>]

> 
> === modified file 'src/w32.h'
> --- src/w32.h	2011-05-04 14:03:16 +0000
> +++ src/w32.h	2011-07-17 14:01:09 +0000
> @@ -133,9 +133,6 @@
>  extern void syms_of_w32term (void);
>  extern void syms_of_w32fns (void);
>  extern void globals_of_w32fns (void);
> -extern void syms_of_w32select (void);
> -extern void globals_of_w32select (void);
> -extern void term_w32select (void);
>  extern void syms_of_w32menu (void);
>  extern void globals_of_w32menu (void);
>  extern void syms_of_fontset (void);

Why was it necessary to remove these prototypes? These functions are
still called outside of the source file where they are defined,
AFAICT.

> -  if (!hprevinst)
> -    {
> -      w32_init_class (hinst);
> -    }
> +  w32_init_class (hinst);

Not sure why the test was deleted here.  Can you explain?

> -      OFNOTIFY * notify = (OFNOTIFY *)lParam;
> +      OFNOTIFYW * notify = (OFNOTIFYW *)lParam;
>        /* Detect when the Filter dropdown is changed.  */
>        if (notify->hdr.code == CDN_TYPECHANGE
>  	  || notify->hdr.code == CDN_INITDONE)
> @@ -5869,7 +5992,7 @@
>  	  if (notify->lpOFN->nFilterIndex == 2)
>  	    {
>  	      CommDlg_OpenSave_SetControlText (dialog, FILE_NAME_TEXT_FIELD,
> -					       "Current Directory");
> +					       L"Current Directory");

Any changes that use the Unicode APIs unconditionally should be tested
on Windows 9X before we adopt them.  I would like to avoid the kind of
breakage that we fixed a couple of months ago (wrt to font APIs)
through a non-trivial effort which needed a motivated and able
individual with access to W9X.  We were lucky to have such help in
that case, but we need to avoid pushing that luck.

So changes like that need (a) a very good reason, and (b) given that
such a reason is provided, some kind of verification that the result
still works on W9X, where one needs a special DLL to have Unicode
support.

> -/* Since we compile with _WIN32_WINNT set to 0x0400 (for NT4 compatibility)
> -   we end up with the old file dialogs. Define a big enough struct for the
> -   new dialog to trick GetOpenFileName into giving us the new dialogs on
> -   Windows 2000 and XP.  */
> -typedef struct
> -{
> -  OPENFILENAME real_details;
> -  void * pReserved;
> -  DWORD dwReserved;
> -  DWORD FlagsEx;
> -} NEWOPENFILENAME;

Why was this removed?

> +    file_details.lpstrFilter = WCSDATA (filter);

What is WCSDATA?  I don't see it defined anywhere.  Apologies if I'm
blind.

> -	file = DECODE_FILE (build_string (filename));
> +        filename = conv_filename_from_w32_unicode (filename_buf, 0);

AFAICS, conv_filename_from_w32_unicode will only be compiled on
Cygwin, so it cannot be used unconditionally here.  (And likewise with
from_unicode and to_unicode?)

> -	  _snprintf (buffer, 16, "%d", system_status.BatteryLifePercent);
> +	  snprintf (buffer, 16, "%d", system_status.BatteryLifePercent);

This breaks the non-MinGW build, I think.  The MS library doesn't have
snprintf, at least not in all versions we support.

> +#if CYGWIN
> +
> +#endif

???

> === modified file 'src/w32select.c'
> --- src/w32select.c	2011-06-24 21:25:22 +0000
> +++ src/w32select.c	2011-07-17 20:32:49 +0000

I understand that these changes are an enhancement for clipboard
operations.  If so, they should be in a separate changeset, and I
would appreciate some discussion of the rationale and the
implementation strategy to go with it.

Still, a few comments.

> +  htext = GlobalAlloc (GMEM_MOVEABLE | GMEM_DDESHARE, bytes);
> +  if (!htext)
> +    error ("GlobalAlloc: %s", w32_strerror (GetLastError ()));

Such cryptic error messages are not useful, because users are not
required to know what GlobalAlloc is.  Please modify the text to be
more palatable to mere mortals (here and elsewhere in this part of the
patch).

> +static void
> +wait_for_clipboard_render ()
> +{
> +  MSG msg;
> +  while (GetMessage (&msg, clipboard_owner,
> +                     WM_EMACS_CLIPBOARD_DATA,
> +                     WM_EMACS_CLIPBOARD_DATA))
> +    {

I'm not sure it is a good idea to call GetMessage in yet another
thread.  Won't this get in the way of the main message pump?  How
would we ensure they are synchronized and not step on each other's
toes?

> +      if (msg.message != WM_EMACS_CLIPBOARD_DATA) {
> +        eassert (0);

Did you really intend to crash when a message other that
WM_EMACS_CLIPBOARD_DATA is received?  Why?

> -static void
> -setup_config (void)
> -{
> -  const char *coding_name;
> -  const char *cp;
> -  char *end;
> -  int slen;
> -  Lisp_Object coding_system;
> -  Lisp_Object dos_coding_system;
> -
> -  CHECK_SYMBOL (Vselection_coding_system);

AFAICT, this portion of the code is just deleted.  Will the
replacement be 100% compatible, including on Windows 9X (where AFAIK
the clipboard does not work in UTF-16)?

> +  return make_number (RegisterClipboardFormatW (to_unicode (format,

Once again: using Unicode APIs should be tested on W9X first.

>        switch (msg.msg.message)
>  	{
> +        case WM_EMACS_MT_CALL:
> +          {
> +            EMACS_TIME interval;
> +            void* data;
> +
> +            EMACS_SET_SECS_USECS (interval, 0, 0);
> +            memcpy (&data, &msg.rect, sizeof(data));
> +            start_atimer (ATIMER_RELATIVE, interval,
> +                          (atimer_callback)msg.msg.lParam, data);
> +          }
> +          break;

What is this part about?

> -#ifdef F_SETOWN
> -  fcntl (connection, F_SETOWN, getpid ());
> -#endif /* ! defined (F_SETOWN) */
> -
> -#ifdef SIGIO
> -  if (interrupt_input)
> -    init_sigio (connection);
> -#endif /* ! defined (SIGIO) */

Are you sure these are never used in any Windows build?

> +#ifdef USE_W32_SELECT

Is this supposed to be defined only in the Cygwin build?  If not, then
could you please explain why there's a need for two `select' calls?

> +#ifdef USE_W32_SELECT
> +  if (pipe (w32_evt_pipe)) {
> +    fatal ("pipe: %s", strerror (errno));
> +  }
> +  fcntl (w32_evt_pipe[0], F_SETFD, FD_CLOEXEC);
> +  fcntl (w32_evt_pipe[1], F_SETFD, FD_CLOEXEC);
> +  w32_evt_write = (HANDLE)get_osfhandle (w32_evt_pipe[1]);

Unless this is for Cygwin only, there should be #ifdef's for using
`pipe', F_SETFD, and FD_CLOEXEC.  If it is for Cygwin only (which I
understand is the case), why use USE_W32_SELECT and not a
Cygwin-specific macro?

> +#endif /* USE_W32_SELET */
                     ^^^^^
A typo.

> +  Fprovide (intern_c_string ("w32"), Qnil);

I think this should be for Cygwin only, and its name should reflect
that.  If you think otherwise, I'd appreciate any arguments you have
for making it a general-purpose feature.

Finally, this will eventually need additions to NEWS and perhaps to
the user manual.

Thanks again for working on this.