emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Colascione
Subject: Re: [PATCH] system-type cygwin with window-system w32
Date: Mon, 18 Jul 2011 03:33:59 -0700
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:5.0) Gecko/20110624 Thunderbird/5.0

On 7/18/11 1:42 AM, Eli Zaretskii wrote:
>>
>> === 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.


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]