Re: [PATCH] system-type cygwin with window-system w32
Eli Zaretskii
> From: Daniel Colascione <dan.col...@gmail.com>
>
> This patch makes it possible to use the NT GUI with a Cygwin core Emacs.
Thanks. A few comments:
> === modified file 'lib/filemode.c'
> --- lib/filemode.c 2011-02-20 10:51:50 +0000
> +++ lib/filemode.c 2011-07-16 11:34:30 +0000
Why did you need to change filemode.c? Does it have anything to do
with Cygwin on w32?
> === modified file 'lisp/term/w32-win.el'
> --- lisp/term/w32-win.el 2011-05-04 14:03:16 +0000
> +++ lisp/term/w32-win.el 2011-07-17 23:04:45 +0000
These look just white-space changes. If so, please leave them out of
the patch.
> +(defconst w32-clipboard-format-html
> + (w32-register-clipboard-format "HTML Format")
> + "The system-specific numeric ID of the HTML clipboard format.")
> +
> +(defconst w32-clipboard-html-header
> + (concat "Version:0.9\r\n"
> + "StartHTML:%0006d\r\n"
> + "EndHTML:%0006d\r\n"
> + "StartFragment%0006d\r\n"
> + "EndFragment:%0006d\r\n"))
> +
> +(defconst w32-clipboard-html-fragment-prefix
> + (concat "<!DOCTYPE HTML>\r\n"
> + "<html><head><title></title></head><body>\r\n"
> + "<!--StartFragment-->\r\n"
> + "<pre%s>"
> +))
What is this (and related) stuff about? Why do you need to use HTML
wrt the clipboard?
> +#define t(...) \
> + ({ \
> + fprintf (stderr, "T:%s:%u: ", \
> + __FUNCTION__, __LINE__); \
> + fprintf (stderr, __VA_ARGS__); \
> + fputc ('\n', stderr); \
> + })
> +
What is this stuff about?
> -/* Equivalent of strerror for W32 error codes. */
> -char *
> -w32_strerror (int error_no)
> -{
> - static char buf[500];
I don't like the idea of moving this to w32fns.c, because it doesn't
belong there. Can you come up with an alternative idea?
> +#if EMACSDEBUG
> +const char*
> +w32_name_of_message (UINT msg)
Why is this needed?
> +
> + /* DebPrint (("w32_msg_pump: %s time:%u\n", */
> + /* w32_name_of_message (msg.message), msg.time)); */
> +
Can this be removed? These DebPrint messages are a PITA when
debugging, so if it isn't absolutely necessary, let's not add new
ones.
This is based on reviewing only a part of the patch, I will have more
later. The patch is very large and complicated, and the lack of a
ChangeLog that describes the changes, particularly those which move
code between different files, does not help...
Daniel Colascione
Thanks for reviewing the patch.
On 7/17/11 11:13 PM, Eli Zaretskii wrote:
> Why did you need to change filemode.c? Does it have anything to do
> with Cygwin on w32?
S_ISCTG and such aren't being defined under Cygwin, causing compilation
errors. There's probably a better way to deal with the underlying problem.
> What is this (and related) stuff about? Why do you need to use HTML
> wrt the clipboard?
Windows uses HTML as a data interchange format --- supporting it as a
clipboard format allows formatting to be preserved in pastes into other
programs. This code could easily be structured as a separate package,
however, and I'll end up doing that.
See http://msdn.microsoft.com/en-us/library/ms649015%28v=vs.85%29.aspx
>> -/* Equivalent of strerror for W32 error codes. */
>> -char *
>> -w32_strerror (int error_no)
>> -{
>> - static char buf[500];
>
> I don't like the idea of moving this to w32fns.c, because it doesn't
> belong there. Can you come up with an alternative idea?
The fundamental problem is that we now have two Windows platforms:
WINDOWSNT and (CYGWIN && HAVE_NTGUI). The common code has to live
somewhere; with my patch, we only build w32.o in the NTEMACS case
because w32.c contains mostly compatibility wrappers; the
non-compatibility portions I moved to w32fns.c, which we compile in both
cases. Cygwin-NT-specific code goes in the new file cygw32.c.
Another option would be to further refactor the Win32 code into distinct
and explicit WINDOWSNT and HAVE_NTGUI files and introduce common headers
for common functionality. This approach would involve even more code
movement, however, which is why I initially avoided it.
>> +#define t(...) \
>> + ({ \
>> + fprintf (stderr, "T:%s:%u: ", \
>> + __FUNCTION__, __LINE__); \
>> + fprintf (stderr, __VA_ARGS__); \
>> + fputc ('\n', stderr); \
>> + })
>> +
>
> What is this stuff about?
Debug scaffolding --- in this case, generally useful, I think, at least
as a replacement for the numerous bespoke tracing macros scattered
everywhere in the code.
>
>> -/* Equivalent of strerror for W32 error codes. */
>> -char *
>> -w32_strerror (int error_no)
>> -{
>> - static char buf[500];
>
> I don't like the idea of moving this to w32fns.c, because it doesn't
> belong there. Can you come up with an alternative idea?
>
>> +#if EMACSDEBUG
>> +const char*
>> +w32_name_of_message (UINT msg)
>
> Why is this needed?
Debug scaffolding.
>> +
>> + /* DebPrint (("w32_msg_pump: %s time:%u\n", */
>> + /* w32_name_of_message (msg.message), msg.time)); */
>> +
>
> Can this be removed? These DebPrint messages are a PITA when
> debugging, so if it isn't absolutely necessary, let's not add new
> ones.
Sure. I'd actually prefer, though, to leave the existing tracing, but
move it all to a common macro so that the debug spam is easier to
disable and enable as needed.
>
> This is based on reviewing only a part of the patch, I will have more
> later. The patch is very large and complicated, and the lack of a
> ChangeLog that describes the changes, particularly those which move
> code between different files, does not help...
Of course. It's a work in progress --- a first stab, really. Once I
clean up the code a bit, I'll put it into a form that's easier to consume.
Daniel Colascione
> Could you please explain this part of the patch:
>
>> /* Create the dialog with PROMPT as title, using DIR as initial
>> directory and using "*" as pattern. */
>> - dir = Fexpand_file_name (dir, Qnil);
>> - strncpy (init_dir, SDATA (ENCODE_FILE (dir)), MAX_PATH);
>> - init_dir[MAX_PATH] = '\0';
>> - unixtodos_filename (init_dir);
>> -
>> - if (STRINGP (default_filename))
>> - {
>> - char *file_name_only;
>> - char *full_path_name = SDATA (ENCODE_FILE (default_filename));
>> -
>> - unixtodos_filename (full_path_name);
>> -
>> - file_name_only = strrchr (full_path_name, '\\');
>> - if (!file_name_only)
>> - file_name_only = full_path_name;
>> - else
>> - file_name_only++;
>> -
>> - strncpy (filename, file_name_only, MAX_PATH);
>> - filename[MAX_PATH] = '\0';
>> - }
>> - else
>> - filename[0] = '\0';
>> -
>> - /* 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 (filename[0] == 0 && ! NILP (only_dir_p))
>> - strcpy (filename, "Current Directory");
>> + to_unicode (Fexpand_file_name (dir, Qnil), &dir);
>> +
>> + to_unicode (build_string ("All Files (*.*)\0*.*\0Directories\0*|*\0\0"),
>> + &filter);
>
> AFAICT, to_unicode encodes the file name in UTF-16. If so, this will
> not work in the native Windows build, because it does not use Unicode
> APIs for file names.
Not today, no.
> In the original code, ENCODE_FILE would use the
> ANSI encoding, not UTF-16. So, unless I'm missing something, the
> replacement is not equivalent to the original, and could break the
> native build.
Yes, the change would make Emacs Unicode-only --- but every Windows OS
in common use supports unicode. Why would requiring unicode support be
a problem? IIUC, Unicode is strictly a superset of all the single-byte
encoding schemes supported by Windows.
> If my analysis is correct, could you please explain the rationale for
> this change?
The change needed to be made anyway --- we need to translate between NT
and Cygwin paths now --- so why not transition to unicode at the same
time? (I want to stub out the path conversion functions for native NT
builds.)
Eli Zaretskii
> === 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.
Daniel Colascione
>> S_ISCTG and such aren't being defined under Cygwin, causing compilation
>> errors. There's probably a better way to deal with the underlying problem.
>
> I'm curious why it didn't work for you.
I didn't look into why it didn't work. I can do some investigation, but
I'm not very familiar with how gnulib stuff actually works.
> I'd prefer a separate file common to w2 and Cygwin-on-w32, if that's
> needed. w32fns.c tries to be as similar to xfns.c as possible, so
> putting there stuff that's not relevant would be a disadvantage.
Fair enough. I'll move some code around; would you object to having
w32.c, cygw32.c, and ntw32.c and corresponding headers?
>>>> +#define t(...) \
>>>> + ({ \
>>>> + fprintf (stderr, "T:%s:%u: ", \
>>>> + __FUNCTION__, __LINE__); \
>>>> + fprintf (stderr, __VA_ARGS__); \
>>>> + fputc ('\n', stderr); \
>>>> + })
>>>> +
>>>
>>> What is this stuff about?
>>
>> Debug scaffolding --- in this case, generally useful, I think, at least
>> as a replacement for the numerous bespoke tracing macros scattered
>> everywhere in the code.
>
> Fine, but (a) please see if there's no macro already available that
> can be used instead;
I didn't see anything suitable.
and (b) let's have this a separate changeset.
Fair enough, though I'll keep it in the patch for now just to make
debugging easier. (gdb under Cygwin is problematic at best, IME.)
>>> This is based on reviewing only a part of the patch, I will have more
>>> later. The patch is very large and complicated, and the lack of a
>>> ChangeLog that describes the changes, particularly those which move
>>> code between different files, does not help...
>>
>> Of course. It's a work in progress --- a first stab, really. Once I
>> clean up the code a bit, I'll put it into a form that's easier to consume.
>
> My point was that there are several issues here that need to be
> discussed before you invest too much energy into them. So please
> consider starting these discussion sooner rather than later, and the
> additional information I didn't find in the ChangeLog would be
> instrumental at that time.
Sure, but there's also something to be said for building a prototype as
well.
Daniel Colascione
>>
>> === 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.
I moved these prototypes to w32select.h.
>> - if (!hprevinst)
>> - {
>> - w32_init_class (hinst);
>> - }
>> + w32_init_class (hinst);
>
> Not sure why the test was deleted here. Can you explain?
hprevinst isn't trivially available under Cygwin, and I don't see what
the test is buying us: class registration is inexpensive.
>> - 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...
> 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.
Let's confine this discussion to the other subthread.
>> -/* 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?
Once we unconditionally use unicode APIs, we don't need to lie about the
structure size anymore. NEWOPENFILENAME's only purpose was to fill in
for a newer version of OPENFILENAME.
>> + file_details.lpstrFilter = WCSDATA (filter);
>
> What is WCSDATA? I don't see it defined anywhere. Apologies if I'm
> blind.
It's in cygw32.h.
>> - 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?)
Correct; this disparity is one reason I'm confident I broke the NT
build. :-) to_unicode and from_unicode, I'll put in common w32 code.
The NT build will have no-op versions of the conv_filename_ functions,
whereas the Cygwin versions will use the Cygwin path functions.
>> - _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.
See above; I plan to just #define away the difference on the NT build.
>
>> +#if CYGWIN
>> +
>> +#endif
>
> ???
Oops.
>
>> === 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.
The clipboard changes are integral to the cygwin-w32 port; without the
new clipboard implementations, applications that try to paste
Emacs-owned clipboard data will hang until the Emacs main thread begins
pumping messages, which will usually happen only when the user actually
interacts with Emacs somehow.
>
> 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).
Well, it's better than what we used to do much of the time, which was to
not check error codes at all. How would you suggest changing the
messages? We don't know any better than the user how to interpret what
went wrong, but it's important to communicate that there was a problem,
and if we do that, we might as well provide relevant information.
>> +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?
I'll have to add a comment explaining what's going on here. In the
meantime: the main thread doesn't usually pump messages because it's
blocked on select(2) instead. If we don't pump messages for the
clipboard window, other applications hang when they try to interact with
Emacs-owned clipboard content. Because win32 windows have thread
affinity, most interactions with a particular window have to happen on
the same thread, so it makes sense to hoist the clipboard infrastructure
to its own thread.
We could just the UI thread for this purpose instead of a dedicated one,
but that too tightly couples the win32 clipboard and HAVE_NTGUI, I
think. What if I want to create a GUI-less Emacs that can nevertheless
can interact with the system clipboard?
>> + 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?
Yes. Because the call to GetMessage above is predicated on
WM_EMACS_CLIPBOARD_DATA, we should never pull a message of a different
type out of the queue. The purpose of this loop is to block and wait
for the main thread to render some clipboard content, and we shouldn't
be doing anything else in the meantime.
>
>> -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)?
Let's have this discussion in the other subthread.
>> 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?
The new clipboard uses WM_EMACS_MT_CALL (indirectly via
call_on_main_thread) to run the Lisp code responsible for actually
rendering clipboard content. The jump through the atimer code ensures
that we're running in a sane place (i.e., in a place that any other
filter could run) instead of wherever we happen to have received an event.
>> -#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?
The section was #if 0ed out and it wouldn't compile anyway ---
connection doesn't exist in w32term.c. This block of code was a
holdover from xterm.c, and we shouldn't keep it around.
>> +#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?
We use USE_W32_SELECT to compile in the self-pipe wakeup mechanism.
When the Cygwin bug I mentioned is fixed and signals begin working
again, we can solve the same problem more cleanly (and with the normal
select(2)) using signals, so I think it makes sense to isolate the
workaround using its own preprocessor macro.
>> +#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?
USE_W32_SELECT is only necessary on Unixish systems like Cygwin anyway.
>> +#endif /* USE_W32_SELET */
> ^^^^^
> A typo.
Thanks.
>> + 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.
Eventually, sure.
Daniel Colascione
>> Date: Mon, 18 Jul 2011 02:41:08 -0700
>> From: Daniel Colascione <dan.col...@gmail.com>
>> CC: emacs...@gnu.org
>>
>> The 9X family is long dead.
>
> Not in the 3rd world, where there are still many machines running it.
> Or so we were told last time this issue popped up. RMS personally
> asked not to drop W9X on this behalf.
I don't recall that discussion. Can we reopen it? According to [1],
Windows 98 has a market share of 0.03%. I suspect there are more VMS
users, and we dropped support for that OS.
Besides: shouldn't we be encouraging computationally indigent third
world users to switch to use free operating systems instead? 9X users
have no protection against security vulnerabilities.
>> We shouldn't forgo technical improvement on the account of a dead
>> system.
>
> I didn't say we should stop the improvement. You will find quite a
> few Emacs features that use APIs which are unavailable on W9X. But
> they do it in a way that lets Emacs still run on W9X and produce
> reasonable results, be that some fallback or just plain message saying
> that this nifty feature is not available. (Of course, disabling menus
> or file access cannot use the latter fire escape, because they are too
> basic and without them Emacs would become unusable.)
These fallbacks involve significant complexity, and they're
lightly-tested at best. I'd prefer to eliminate the complexity involved
in supporting these alleged 9X users by simply dropping the support.
Moving to all-Unicode APIs would improve Emacs' internationalization
support _and_ simplify the codebase.
>>> and (2) going Unicode means that all the existing APIs
>>> used by Emacs will have to be switched to Unicode.
>>
>> Why not do it piecemeal? We can directly call Unicode APIs where
>> appropriate.
>
> I'm not sure I understand the details of your proposal. The situation
> that worries me is this:
>
> . user uses the file dialog to return a file name in UTF-16 which
> includes characters not available in the system codepage (this is
> quite possible on NTFS)
>
> . the file name is passed to file-attributes or insert-file-name or
> some other primitive that accepts file names
It'd be straightforward to locate all calls to CreateFile and such and
update them to use unicode APIs. Cygwin supports UTF-8 filenames natively.
But even if we don't --- why does it matter? You can create files using
the NT native API that can't be opened using Win32 calls; it doesn't
cause a problem in practice. Likewise, users who have strange
filesnames might not be able to use them with all Emacs features right
away, but they'll be able to work with more reasonable filenames just as
they did before.
> . if the underlying file APIs used by these primitives (`stat',
> `open', `opendir', etc.) are not Unicode, these primitives will
> fail in weird ways, like "file does not exist" etc., that would
> just confuse the user.
I'd prefer to go all-Unicode because I don't think unencodable filenames
are common enough to warrant much concern here. Any file users can
manipulate today, they'll be able to manipulate with a
partially-Unicodeized Emacs.
Still, if we can't do that, then as a temporary measure, we can still
use Unicode APIs (the 9X discussion notwithstanding), but as a temporary
measure, filter their results so that we reject filenames that can't be
used with the system codepage.
Eli Zaretskii
> > Or so we were told last time this issue popped up. RMS personally
> > asked not to drop W9X on this behalf.
>
> I don't recall that discussion. Can we reopen it?
Let's wait for Richard to chime in.
> > I didn't say we should stop the improvement. You will find quite a
> > few Emacs features that use APIs which are unavailable on W9X. But
> > they do it in a way that lets Emacs still run on W9X and produce
> > reasonable results, be that some fallback or just plain message saying
> > that this nifty feature is not available. (Of course, disabling menus
> > or file access cannot use the latter fire escape, because they are too
> > basic and without them Emacs would become unusable.)
>
> These fallbacks involve significant complexity, and they're
> lightly-tested at best. I'd prefer to eliminate the complexity involved
> in supporting these alleged 9X users by simply dropping the support.
I'd prefer that as well, as soon as we drop W9X support.
> > . user uses the file dialog to return a file name in UTF-16 which
> > includes characters not available in the system codepage (this is
> > quite possible on NTFS)
> >
> > . the file name is passed to file-attributes or insert-file-name or
> > some other primitive that accepts file names
>
> It'd be straightforward to locate all calls to CreateFile and such and
> update them to use unicode APIs.
I'm afraid it isn't straightforward. I suspect there's a lot of
supporting code that still assumes unibyte characters. But I'll
welcome patches in that area (if we agree to drop W9X support).
> Cygwin supports UTF-8 filenames natively.
I know that, but it isn't relevant to the native w32 build, because
that needs to use UTF-16, not UTF-8.
> But even if we don't --- why does it matter? You can create files using
> the NT native API that can't be opened using Win32 calls; it doesn't
> cause a problem in practice. Likewise, users who have strange
> filesnames might not be able to use them with all Emacs features right
> away, but they'll be able to work with more reasonable filenames just as
> they did before.
But switching to Unicode doesn't make sense _unless_ you want to
support "strange file names": all the non-strange file names are
already supported under the current ``ANSI'' APIs. It's when I want
to see file names with characters not from my system locale that I
need Unicode.
> > . if the underlying file APIs used by these primitives (`stat',
> > `open', `opendir', etc.) are not Unicode, these primitives will
> > fail in weird ways, like "file does not exist" etc., that would
> > just confuse the user.
>
> I'd prefer to go all-Unicode because I don't think unencodable filenames
> are common enough to warrant much concern here.
Sorry, I disagree. I have quite a few on my system, for example. If
you want to know how I got them, then they came with zip files that
included dictionaries in all kinds of languages, I brought them in
when I worked in a Windows port of Ispell. And that's just one
example that came to my mind, I'm sure I would find more if I cared to
look. E.g., I remember seeing file names with Kanji characters at
some point.
IMO, a half-broken feature is worse than an absent feature.
Especially if the breakage reveals itself as subtly as "file does not
exist" when I just selected it from a dialog.
> Any file users can manipulate today, they'll be able to manipulate
> with a partially-Unicodeized Emacs.
See above: for those, the Unicode interfaces give no advantage.
> Still, if we can't do that, then as a temporary measure, we can still
> use Unicode APIs (the 9X discussion notwithstanding), but as a temporary
> measure, filter their results so that we reject filenames that can't be
> used with the system codepage.
But then this is just complication with no benefits, isn't it?
Daniel Colascione
>> Date: Mon, 18 Jul 2011 03:33:59 -0700
>>>> - {
>>>> - w32_init_class (hinst);
>>>> - }
>>>> + w32_init_class (hinst);
>>>
>>> Not sure why the test was deleted here. Can you explain?
>>
>> hprevinst isn't trivially available under Cygwin, and I don't see what
>> the test is buying us: class registration is inexpensive.
>
> effect is to always call the function, as you wanted, right? So I
> would rather we left the code alone.
We'd still need the variable with your proposal, and I don't see what
the existing behavior has, even in the NT case.
>>>> + 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).
>>
>> Well, it's better than what we used to do much of the time, which was to
>> not check error codes at all. How would you suggest changing the
>> messages?
>
> How about calling memory_full?
>
> Or maybe error ("Not enough memory <TO DO WHATEVER THIS CODE DOES>") ?
The error isn't necessarily fatal --- and in general (speaking to other
instances of w32_strerror in the patch) we don't always know what
exactly went wrong. It'd be nice to give users an opportunity to figure
it out. Maybe we can recognize a subset of error codes and forward
those to memory_full.
>> We could just the UI thread for this purpose instead of a dedicated one,
>
> This is what I had in mind as the alternative, yes.
>
>> What if I want to create a GUI-less Emacs that can nevertheless
>> can interact with the system clipboard?
>
> GUI-less Emacs normally doesn't interact with the clipboard, so
> there's no need to choose a design that complicates things just
> because we would like to make this feature available on a single
> platform.
The complexity has to be present regardless.
> Anyway, I'm hardly an expert on this particular issue (i.e. Windows
> GUI and the message pump). I'm just worried by the fact that we will
> have 2 threads calling GetMessage; in my experience this could lead to
> hard-to-debug problems.
Calling GetMessage in two threads is very common and well-supported, and
won't by itself cause problems. If anything, separating the message
loops makes the program more robust --- each message loop is less
complex and thereby easier to understand.
Daniel Colascione
> Daniel Colascione wrote:
>> I'll have to add a comment explaining what's going on here. In the
>> meantime: the main thread doesn't usually pump messages because it's
>> blocked on select(2) instead.
>
> incoming WM_xxx messages?
>
> --- grischka
Wow. Thanks for the tip. /dev/windows doesn't appear to be well-documented
externally, but from fhandler_windows.cc in the Cygwin tree:
/*
The following unix-style calls are supported:
open ("/dev/windows", flags, mode=0)
- create a unix fd for message queue.
read (fd, buf, len)
- return next message from queue. buf must point to MSG
structure, len must be >= sizeof (MSG). If read is set to
non-blocking and the queue is empty, read call returns -1
immediately with errno set to EAGAIN, otherwise it blocks
untill the message will be received.
write (fd, buf, len)
- send a message pointed by buf. len argument ignored.
ioctl (fd, command, *param)
- control read()/write() behavior.
ioctl (fd, WINDOWS_POST, NULL): write() will PostMessage();
ioctl (fd, WINDOWS_SEND, NULL): write() will SendMessage();
ioctl (fd, WINDOWS_HWND, &hWnd): read() messages for
hWnd window.
select () call marks read fd when any message posted to queue.
*/
If it works, /dev/windows would allow us to get rid of not only the self-pipe
and the clipboard thread, but the UI thread as well, though it'd be easier to
keep the last of these for compatibility for the NT build. Thanks.
By the way: why do we use a separate UI thread in the NT case at all? AIUI, we
can do everything we need asynchronously via overlapped IO, so we should never
have to block and not pump messages.
Daniel Colascione
> I'm afraid it isn't straightforward. I suspect there's a lot of
> supporting code that still assumes unibyte characters. But I'll
> welcome patches in that area (if we agree to drop W9X support).
I meant that we can convert to UTF-8 (or our internal multibyte
encoding) until the very last moment before calling a system function;
we know UTF-8 support works already, and the conversation preserves
everything.
>> Cygwin supports UTF-8 filenames natively.
>
> I know that, but it isn't relevant to the native w32 build, because
> that needs to use UTF-16, not UTF-8.
>
>> But even if we don't --- why does it matter? You can create files using
>> the NT native API that can't be opened using Win32 calls; it doesn't
>> cause a problem in practice. Likewise, users who have strange
>> filesnames might not be able to use them with all Emacs features right
>> away, but they'll be able to work with more reasonable filenames just as
>> they did before.
>
> But switching to Unicode doesn't make sense _unless_ you want to
> support "strange file names": all the non-strange file names are
> already supported under the current ``ANSI'' APIs. It's when I want
> to see file names with characters not from my system locale that I
> need Unicode.
Unicode APIs are also more modern in a sense --- new ANSI APIs aren't
being created anymore.
> [snip]
>
> See above: for those, the Unicode interfaces give no advantage.
>
>> Still, if we can't do that, then as a temporary measure, we can still
>> use Unicode APIs (the 9X discussion notwithstanding), but as a temporary
>> measure, filter their results so that we reject filenames that can't be
>> used with the system codepage.
>
> But then this is just complication with no benefits, isn't it?
Not necessarily --- this approach would allow us to gradually migrate to
Unicode without introducing the possibility of delivering
incomprehensible error messages to users. When we've updated all the
relevant bits, we can just remove the filtering and have a pure Unicode
Emacs.
Daniel Colascione
> Daniel Colascione wrote:
>> If it works, /dev/windows would allow us to get rid of not only the
>> self-pipe
>> and the clipboard thread, but the UI thread as well, though it'd be
>> easier to
>> keep the last of these for compatibility for the NT build. Thanks.
>
> Already tested here.
Do you have a patch handy?
>> By the way: why do we use a separate UI thread in the NT case at all?
>> AIUI, we
>> can do everything we need asynchronously via overlapped IO, so we
>> should never
>> have to block and not pump messages.
>
> The 'sys_select' wrapper DOES pump messages, just for the wrong
> thread. So it needs the thread because it has the thread. :)
Of course. I suspect the initial motivation behind the separate thread
is a long, somnolent story involving QUIT.
> Well, there are two other things needed:
> * peek for messages in the QUIT macro (say via ELSE_PENDING_SIGNALS)
> which is for C-g to interrupt lisp.
I don't think this approach alone would be sufficient. I may be wrong,
but I think the Windows window manager will consider a program to be
"unresponsive" if it stops actually pumping messages; I don't think
peeking is sufficient. Also, [1] says that we shouldn't delay pumping
messages even if we're able to guarantee we'll get around to them later.
(Running lisp code is indistinguishable from sleep in this case.)
Using PeekMessage in lisp code instead of actually pumping messages
would be like telling your credit card company, "Yeah, I got the bill,
and I'll get around to responding sometime in the next two years". I
don't think it'd go over well.
Actually, on that note, I should add a timeout to my WM_RENDERFORMAT
code: we shouldn't block other applications forever merely because Emacs
is spinning in Lisp.
> * break command_loop_1() such that it can be used to handle just one
> event which is to handle scrollbar messages because the widgets
> run their own message loop deep in windows. Otherwise all the
> scrolling would happen only after you release the mouse button.
I doubt that the scrollbar is the only special case we'd need to consider.
On 7/18/2011 11:52 AM, grischka wrote:
> Daniel Colascione wrote:
>> This requirement is incompatible
>> with using lisp code to render clipboard content: we might have
>> received a
>> window message at an inopportune time for calling back into Lisp, and
>> we can't
>> delay the response to WM_RENDERFORMAT by re-queuing or somesuch.
>> Today's NT
>> clipboard code doesn't have to address the issue because it never
>> calls into Lisp.
>
> Well, you can do this if you have a function to pump elisp
> events, say drain_lisp_queue():
>
> That would be basically command_loop_1() with
> while (1)
> replaced by
> while (detect_input_pending())
>
> Then, in window_proc:
> case WM_RENDERFORMAT:
> queue_lisp_event(); //I guess it's "kbd_buffer_store_event_hold"
> drain_lisp_queue();
> break;
>
> Or such.
>
I don't understand how this approach helps. The problem, AIUI, isn't
that we have Lisp events, but that we read input and wait for processes
in many places, and it's hard to be confident that each place we pump
messages is a safe place to process lisp code. I don't understand how
draining the lisp event queue would help.
[1] http://blogs.msdn.com/b/oldnewthing/archive/2006/02/10/529525.aspx