From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Daniel Colascione Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] system-type cygwin with window-system w32 Date: Mon, 18 Jul 2011 03:33:59 -0700 Message-ID: <4E240C17.4020102@gmail.com> References: <4E2377E2.1020804@gmail.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig8328D0DC2503A26917ADAF6E" X-Trace: dough.gmane.org 1310985739 8125 80.91.229.12 (18 Jul 2011 10:42:19 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 18 Jul 2011 10:42:19 +0000 (UTC) Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Jul 18 12:42:15 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 1QilHC-0003e9-Vb for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2011 12:42:15 +0200 Original-Received: from localhost ([::1]:47665 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QilHC-00050B-4s for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2011 06:42:14 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:34491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qil9V-0003D2-0W for emacs-devel@gnu.org; Mon, 18 Jul 2011 06:34:18 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qil9T-0006cl-7e for emacs-devel@gnu.org; Mon, 18 Jul 2011 06:34:16 -0400 Original-Received: from mail-iw0-f169.google.com ([209.85.214.169]:47101) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qil9Q-0006cL-Ho; Mon, 18 Jul 2011 06:34:12 -0400 Original-Received: by iwn8 with SMTP id 8so3232911iwn.0 for ; Mon, 18 Jul 2011 03:34:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:x-enigmail-version:content-type; bh=zUx8Jsiz1JwIS1Kcs9bIhptdGJNdYCRTrzLs/BF4SZg=; b=eZuKg7e1N1Zl7h66LPwrkjCZ02us9H7gb64MCtCEA4DP24QdThpwQNrxJFIGEakr+L LpDLMB1NrFKe7kJbUds3po2nBu2y9UuWkmESpz9sn2XKcpCeiUkXQAXH3LhaT7WaPPPh e835BIkvIwLigDNhJ3ZsOVrMqPmvkaUfReEvw= Original-Received: by 10.231.205.197 with SMTP id fr5mr675619ibb.198.1310985251380; Mon, 18 Jul 2011 03:34:11 -0700 (PDT) Original-Received: from [192.168.1.2] (c-24-18-179-193.hsd1.wa.comcast.net [24.18.179.193]) by mx.google.com with ESMTPS id f13sm2845011ibe.44.2011.07.18.03.34.08 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 18 Jul 2011 03:34:10 -0700 (PDT) User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:5.0) Gecko/20110624 Thunderbird/5.0 In-Reply-To: X-Enigmail-Version: 1.2 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 209.85.214.169 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:142098 Archived-At: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig8328D0DC2503A26917ADAF6E Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 7/18/11 1:42 AM, Eli Zaretskii wrote: >> >> =3D=3D=3D 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); >=20 > 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); >=20 > 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 =3D (OFNOTIFY *)lParam; >> + OFNOTIFYW * notify =3D (OFNOTIFYW *)lParam; >> /* Detect when the Filter dropdown is changed. */ >> if (notify->hdr.code =3D=3D CDN_TYPECHANGE >> || notify->hdr.code =3D=3D CDN_INITDONE) >> @@ -5869,7 +5992,7 @@ >> if (notify->lpOFN->nFilterIndex =3D=3D 2) >> { >> CommDlg_OpenSave_SetControlText (dialog, FILE_NAME_TEXT_FIELD,= >> - "Current Directory"); >> + L"Current Directory"); >=20 > 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 compatib= ility) >> - we end up with the old file dialogs. Define a big enough struct fo= r 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; >=20 > 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 =3D WCSDATA (filter); >=20 > What is WCSDATA? I don't see it defined anywhere. Apologies if I'm > blind. It's in cygw32.h. >> - file =3D DECODE_FILE (build_string (filename)); >> + filename =3D conv_filename_from_w32_unicode (filename_buf, 0)= ; >=20 > 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); >=20 > 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. >=20 >> +#if CYGWIN >> + >> +#endif >=20 > ??? Oops. >=20 >> =3D=3D=3D 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 >=20 > 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. >=20 > Still, a few comments. >=20 >> + htext =3D GlobalAlloc (GMEM_MOVEABLE | GMEM_DDESHARE, bytes); >> + if (!htext) >> + error ("GlobalAlloc: %s", w32_strerror (GetLastError ())); >=20 > 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)) >> + { >=20 > 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 !=3D WM_EMACS_CLIPBOARD_DATA) { >> + eassert (0); >=20 > 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. >=20 >> -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); >=20 > 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; >=20 > 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= =2E >> -#ifdef F_SETOWN >> - fcntl (connection, F_SETOWN, getpid ()); >> -#endif /* ! defined (F_SETOWN) */ >> - >> -#ifdef SIGIO >> - if (interrupt_input) >> - init_sigio (connection); >> -#endif /* ! defined (SIGIO) */ >=20 > 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 >=20 > 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 =3D (HANDLE)get_osfhandle (w32_evt_pipe[1]); >=20 > 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); >=20 > 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. >=20 > Finally, this will eventually need additions to NEWS and perhaps to > the user manual. Eventually, sure. --------------enig8328D0DC2503A26917ADAF6E Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (Darwin) iEYEARECAAYFAk4kDB0ACgkQ17c2LVA10Vv9qwCgltl8Q9sxbpfiRgwYk1me8jjl eyEAnjphqUIT3kCzgQBQ6RkbnFwvJ4TI =W8iA -----END PGP SIGNATURE----- --------------enig8328D0DC2503A26917ADAF6E--