From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] system-type cygwin with window-system w32 Date: Mon, 18 Jul 2011 04:42:24 -0400 Message-ID: References: <4E2377E2.1020804@gmail.com> Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: dough.gmane.org 1310980402 8609 80.91.229.12 (18 Jul 2011 09:13:22 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 18 Jul 2011 09:13:22 +0000 (UTC) Cc: emacs-devel@gnu.org To: Daniel Colascione Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Jul 18 11:13:17 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Qijt6-0007f6-K7 for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2011 11:13:16 +0200 Original-Received: from localhost ([::1]:58058 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qijt5-0006FR-Rx for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2011 05:13:15 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:35266) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QijPH-0006aU-8H for emacs-devel@gnu.org; Mon, 18 Jul 2011 04:42:28 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QijPF-0005N2-AW for emacs-devel@gnu.org; Mon, 18 Jul 2011 04:42:26 -0400 Original-Received: from fencepost.gnu.org ([140.186.70.10]:51434) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QijPF-0005My-0P for emacs-devel@gnu.org; Mon, 18 Jul 2011 04:42:25 -0400 Original-Received: from eliz by fencepost.gnu.org with local (Exim 4.71) (envelope-from ) id 1QijPE-0004Rm-QK; Mon, 18 Jul 2011 04:42:24 -0400 In-reply-to: <4E2377E2.1020804@gmail.com> (message from Daniel Colascione on Sun, 17 Jul 2011 17:01:38 -0700) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 140.186.70.10 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:142092 Archived-At: > > === 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.