Some comments about changes in revno 110444

Some comments about changes in revno 110444

[Eli Zaretskii]

This hunk removes w32inevt from SOME_MACHINE_OBJECTS.  Why was that
needed?

   SOME_MACHINE_OBJECTS = dosfns.o msdos.o \
     xterm.o xfns.o xmenu.o xselect.o xrdb.o xsmfns.o fringe.o image.o \
  -  fontset.o dbusbind.o \
  +  fontset.o dbusbind.o cygw32.o \
     nsterm.o nsfns.o nsmenu.o nsselect.o nsimage.o nsfont.o \
  -  w32.o w32console.o w32fns.o w32heap.o w32inevt.o \
  +  w32.o w32console.o w32fns.o w32heap.o \
     w32menu.o w32proc.o w32reg.o w32select.o w32term.o w32xfns.o \
     w16select.o widget.o xfont.o ftfont.o xftfont.o ftxfont.o gtkutil.o \
     xsettings.o xgselect.o termcap.o

To avoid confusion, I suggest to change the name of the variable here:

  +  /* No, not utf-16-le: that one has a BOM.  */
  +  DEFSYM (Qutf_16_le, "utf-16le");

to Qutf_16le.

Isn't the below supposed to be done automagically by make-docfile,
which puts these in globals.h?

  +EXFUN (Fcygwin_convert_path_to_windows, 2);
  +EXFUN (Fcygwin_convert_path_from_windows, 2);

I don't think this (from w32select.h) is a good idea:

  +#define HAVE_W32SELECT 1

We only have definitions for HAVE_* macros in config.h and
conf_post.h.  Having them in other headers is confusing, IMO.  Can we
move this to conf_post.h instead?

You moved the definitions of these from w32heap.c to w32fns.c:

  +/* The major and minor versions of NT.  */
  +int w32_major_version;
  +int w32_minor_version;
  +int w32_build_number;

but they are still referenced by w32.c via get_* macros defined on
w32heap.h.  Can we clean this up, please?

Regarding this:

  +#ifdef NTGUI_UNICODE
  +  wchar_t filename_buf[MAX_PATH + 1];

I believe Unicode names on Windows are not limited to MAX_PATH
characters.

It looks like there are some unnecessary hops here:

  +    CHECK_STRING (prompt);
  +    CHECK_STRING (dir);
  +
  +    dir = Fexpand_file_name (dir, Qnil);
  +
  +    if (STRINGP (filename))
  +      filename = Ffile_name_nondirectory (filename);
  +    else
  +      filename = empty_unibyte_string;
  +
  +#ifdef CYGWIN
  +    dir = Fcygwin_convert_path_to_windows (dir, Qt);
  +    if (SCHARS (filename) > 0)
  +      filename = Fcygwin_convert_path_to_windows (filename, Qnil);
  +#endif

  +    CHECK_STRING (dir);
  +    CHECK_STRING (filename);
  +
  +    /* The code in file_dialog_callback that attempts to set the text
  +       of the file name edit window when handling the CDN_INITDONE
  +       WM_NOTIFY message does not work.  Setting filename to "Current
  +       Directory" in the only_dir_p case here does work however.  */
  +    if (SCHARS (filename) == 0 && ! NILP (only_dir_p))
  +      filename = build_string ("Current Directory");
  +
  +    /* Convert the values we've computed so far to system form.  */
  +#ifdef NTGUI_UNICODE
  +    to_unicode (prompt, &prompt);
  +    to_unicode (dir, &dir);
  +    to_unicode (filename, &filename);

AFAIU, inside Fcygwin_convert_path_to_windows, you convert the names
of the file and directory to UTF-16, then decode it back into a
multibyte string, and finally encode it again to UTF-16 when you call
to_unicode.  Perhaps the back and forth conversion to and from UTF-16
can be avoided?

In any case, a call to CHECK_STRING after we already checked the
arguments seems both redundant and user-unfriendly: these tests are
designed to reject errors in arguments passed to primitives, so
signaling an error because something went wrong in processing valid
arguments doesn't sound like a good idea.

This error will not report the offending file name:

  +    if (SBYTES (filename) + 1 > sizeof (filename_buf))
  +      error ("filename too long");

I think report_file_error is better here.

Why do you need the fprintf part here:

  #ifdef EMACSDEBUG
  void
  _DebPrint (const char *fmt, ...)
  {
    char buf[1024];
    va_list args;

    va_start (args, fmt);
    vsprintf (buf, fmt, args);
    va_end (args);
  #if CYGWIN
    fprintf (stderr, "%s", buf);
  #endif
    OutputDebugString (buf);
  }

What happened to number 21 in the change below?

   #define WM_EMACS_HIDE_CARET            (WM_EMACS_START + 18)
   #define WM_EMACS_SETCURSOR             (WM_EMACS_START + 19)
   #define WM_EMACS_PAINT                 (WM_EMACS_START + 20)
  -#define WM_EMACS_BRINGTOTOP            (WM_EMACS_START + 21)
  -#define WM_EMACS_END                   (WM_EMACS_START + 22)
  +#define WM_EMACS_BRINGTOTOP            (WM_EMACS_START + 22)
  +#define WM_EMACS_INPUT_READY           (WM_EMACS_START + 23)
  +#define WM_EMACS_END                   (WM_EMACS_START + 24)

Why did you need the call to notify_msg_ready below?

  @@ -285,7 +309,7 @@ prepend_msg (W32Msg *lpmsg)
     nQueue++;
     lpNew->lpNext = lpHead;
     lpHead = lpNew;
  -
  +  notify_msg_ready ();

Re: Some comments about changes in revno 110444

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 6053 bytes --]

On 10/8/2012 4:01 AM, Eli Zaretskii wrote:
> This hunk removes w32inevt from SOME_MACHINE_OBJECTS.  Why was that
> needed?

Merge problem. Fixed.

> To avoid confusion, I suggest to change the name of the variable here:
> 
>   +  /* No, not utf-16-le: that one has a BOM.  */
>   +  DEFSYM (Qutf_16_le, "utf-16le");
> 
> to Qutf_16le.

Good catch.

> Isn't the below supposed to be done automagically by make-docfile,
> which puts these in globals.h?
> 
>   +EXFUN (Fcygwin_convert_path_to_windows, 2);
>   +EXFUN (Fcygwin_convert_path_from_windows, 2);

The earliest version of this code targeted Emacs 23, which doesn't have a
globals.h. Fixed.

> I don't think this (from w32select.h) is a good idea:
> 
>   +#define HAVE_W32SELECT 1
> 
> We only have definitions for HAVE_* macros in config.h and
> conf_post.h.  Having them in other headers is confusing, IMO.  Can we
> move this to conf_post.h instead?

We only used HAVE_W32SELECT in two places, so I just replaced HAVE_W32SELECT
with the conditional we use to include the header --- WINDOWSNT || HAVE_NTGUI.

> You moved the definitions of these from w32heap.c to w32fns.c:
> 
>   +/* The major and minor versions of NT.  */
>   +int w32_major_version;
>   +int w32_minor_version;
>   +int w32_build_number;
> 
> but they are still referenced by w32.c via get_* macros defined on
> w32heap.h.  Can we clean this up, please?

The bigger issue is that w32heap.c is WINDOWSNT-only, but w32heap.h declares
useful facilities that apply more generally (and that have nothing to do with
the heap). I created a new w32common.h for these common declarations and left
the WINDOWSNT-specific heap stuff in w32heap.h.

> Regarding this:
> 
>   +#ifdef NTGUI_UNICODE
>   +  wchar_t filename_buf[MAX_PATH + 1];
> 
> I believe Unicode names on Windows are not limited to MAX_PATH
> characters.

You can use special syntax to exceed that limit ---
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
--- but nobody does in practice.  Bear in mind that this code only deals with
the file open dialog box, which (the last time I checked) didn't support long
file names in the first place.

Still, I've increased the limit to 32k + 1, which is a hard NT kernel limit, in
the NTGUI_UNICODE case.

> It looks like there are some unnecessary hops here:
> 
>   +    CHECK_STRING (prompt);
>   +    CHECK_STRING (dir);
>   +
>   +    dir = Fexpand_file_name (dir, Qnil);
>   +
>   +    if (STRINGP (filename))
>   +      filename = Ffile_name_nondirectory (filename);
>   +    else
>   +      filename = empty_unibyte_string;
>   +
>   +#ifdef CYGWIN
>   +    dir = Fcygwin_convert_path_to_windows (dir, Qt);
>   +    if (SCHARS (filename) > 0)
>   +      filename = Fcygwin_convert_path_to_windows (filename, Qnil);
>   +#endif
> 
>   +    CHECK_STRING (dir);
>   +    CHECK_STRING (filename);
>   +
>   +    /* The code in file_dialog_callback that attempts to set the text
>   +       of the file name edit window when handling the CDN_INITDONE
>   +       WM_NOTIFY message does not work.  Setting filename to "Current
>   +       Directory" in the only_dir_p case here does work however.  */
>   +    if (SCHARS (filename) == 0 && ! NILP (only_dir_p))
>   +      filename = build_string ("Current Directory");
>   +
>   +    /* Convert the values we've computed so far to system form.  */
>   +#ifdef NTGUI_UNICODE
>   +    to_unicode (prompt, &prompt);
>   +    to_unicode (dir, &dir);
>   +    to_unicode (filename, &filename);
> 
> AFAIU, inside Fcygwin_convert_path_to_windows, you convert the names
> of the file and directory to UTF-16, then decode it back into a
> multibyte string, and finally encode it again to UTF-16 when you call
> to_unicode.  Perhaps the back and forth conversion to and from UTF-16
> can be avoided?

This code isn't on a hot path.  I'm reluctant to optimize it: I'm not sure the
additional complexity would be worth it.

> In any case, a call to CHECK_STRING after we already checked the
> arguments seems both redundant and user-unfriendly: these tests are
> designed to reject errors in arguments passed to primitives, so
> signaling an error because something went wrong in processing valid
> arguments doesn't sound like a good idea.
> 
> This error will not report the offending file name:
> 
>   +    if (SBYTES (filename) + 1 > sizeof (filename_buf))
>   +      error ("filename too long");
> 
> I think report_file_error is better here.

Fixed.

> Why do you need the fprintf part here:
>   #ifdef EMACSDEBUG
>   void
>   _DebPrint (const char *fmt, ...)
>   {
>     char buf[1024];
>     va_list args;
> 
>     va_start (args, fmt);
>     vsprintf (buf, fmt, args);
>     va_end (args);
>   #if CYGWIN
>     fprintf (stderr, "%s", buf);
>   #endif
>     OutputDebugString (buf);
>   }

Because tracing on stderr is useful.  Strings sent to OutputDebugString are
invisible unless Emacs is being run under a debugger.

> What happened to number 21 in the change below?

Bad merge. Fixed.

>    #define WM_EMACS_HIDE_CARET            (WM_EMACS_START + 18)
>    #define WM_EMACS_SETCURSOR             (WM_EMACS_START + 19)
>    #define WM_EMACS_PAINT                 (WM_EMACS_START + 20)
>   -#define WM_EMACS_BRINGTOTOP            (WM_EMACS_START + 21)
>   -#define WM_EMACS_END                   (WM_EMACS_START + 22)
>   +#define WM_EMACS_BRINGTOTOP            (WM_EMACS_START + 22)
>   +#define WM_EMACS_INPUT_READY           (WM_EMACS_START + 23)
>   +#define WM_EMACS_END                   (WM_EMACS_START + 24)
> 
> Why did you need the call to notify_msg_ready below?
> 
>   @@ -285,7 +309,7 @@ prepend_msg (W32Msg *lpmsg)
>      nQueue++;
>      lpNew->lpNext = lpHead;
>      lpHead = lpNew;
>   -
>   +  notify_msg_ready ();

The call to notify_msg_ready is needed because after a call to prepend_msg, a
message is now ready.  It should have been there all along.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: Some comments about changes in revno 110444

[Eli Zaretskii]

> Date: Mon, 08 Oct 2012 05:53:52 -0700
> From: Daniel Colascione <dancol@dancol.org>
> CC: emacs-devel@gnu.org
> 
> > Why did you need the call to notify_msg_ready below?
> > 
> >   @@ -285,7 +309,7 @@ prepend_msg (W32Msg *lpmsg)
> >      nQueue++;
> >      lpNew->lpNext = lpHead;
> >      lpHead = lpNew;
> >   -
> >   +  notify_msg_ready ();
> 
> The call to notify_msg_ready is needed because after a call to prepend_msg, a
> message is now ready.  It should have been there all along.

I guess no one have seen a MSH_MOUSEWHEEL message in Emacs in a long,
long, LONG time ;-)