Re: [PATCH 3/4] Implement cygw32

[Daniel Colascione]

On 12/29/11 9:29 AM, Eli Zaretskii wrote:
>> Date: Thu, 29 Dec 2011 06:03:20 -0800
>> From: Daniel Colascione <address@hidden>
>>
>> --- a/lisp/faces.el
>> +++ b/lisp/faces.el
>> @@ -89,7 +89,7 @@ ALTERNATIVE2 etc."
>>  ;; This is defined originally in xfaces.c.
>>  (defcustom face-font-registry-alternatives
>>    (mapcar (lambda (arg) (mapcar 'purecopy arg))
>> -  (if (eq system-type 'windows-nt)
>> +  (if (featurep 'w32)
>>        '(("iso8859-1" "ms-oemlatin")
>>      ("gb2312.1980" "gb2312" "gbk" "gb18030")
>>      ("jisx0208.1990" "jisx0208.1983" "jisx0208.1978")
> 
> Why not use window-system (the function) here?

Isn't it a little early for window-system at this point?  This
particular block doesn't have an immediate effect anyway: it just
registers a set of registry synonyms, which will be just as valid later.

>> --- a/lisp/frame.el
>> +++ b/lisp/frame.el
>> @@ -522,7 +522,7 @@ The optional argument PARAMETERS specifies additional 
>> frame parameters."
>>         (ns-initialize-window-system))
>>       (make-frame `((window-system . ns)
>>                     (display . ,display) . ,parameters)))
>> -    ((eq system-type 'windows-nt)
>> +    ((eq window-system 'w32)
>>       ;; On Windows, ignore DISPLAY.
>>       (make-frame parameters))
>>      (t
> 
> Why not window-system the function?  You do want to continue
> supporting multi-tty in this configuration, right?

I've tested multi-tty and it works fine. There's no support for multiple
displays, but that's a given with w32.

> 
>> --- a/lisp/international/mule-cmds.el
>> +++ b/lisp/international/mule-cmds.el
>> @@ -2655,7 +2655,8 @@ See also `locale-charset-language-names', 
>> `locale-language-names',
>>      ;; On Windows, override locale-coding-system,
>>      ;; default-file-name-coding-system, keyboard-coding-system,
>>      ;; terminal-coding-system with system codepage.
>> -    (when (boundp 'w32-ansi-code-page)
>> +    (when (and (eq system-type 'windows-nt)
>> +               (boundp 'w32-ansi-code-page))
>>        (let ((code-page-coding (intern (format "cp%d" w32-ansi-code-page))))
> 
> Can you explain the need for this change?

Cygwin communicates locale information through environment variables as
most unix systems do.  The coding system derived from this locale
information is the one that should be used to configure Emacs' language
environment because it's the one Cygwin expects to be used with its
APIs.  When Emacs is configured with system-type cygwin and
window-system w32, it still provides w32-ansi-code-page.  The hunk above
forces Emacs to use the Windows code page only when it's really a native
Windows program.

> 
>> --- a/lisp/loadup.el
>> +++ b/lisp/loadup.el
>> @@ -207,15 +207,18 @@
>>        (load "term/common-win")
>>        (load "term/x-win")))
>>  
>> -(if (eq system-type 'windows-nt)
>> +(if (or (eq system-type 'windows-nt)
>> +        (featurep 'w32))
>>      (progn
>> -      (load "w32-vars")
>>        (load "term/common-win")
>> +      (load "w32-vars")
> 
> Did you really need this order change?  If yes, why?  If not, I'd
> prefer to leave the original order, as changing it could potentially
> cause unintended consequences.

No, moving w32-vars back on top shouldn't cause a problem.  It really
doesn't belong there though: shouldn't we load the more general code
first, then let the platform-specific code muck with it?

>>        (load "term/w32-win")
>> -      (load "ls-lisp")
>>        (load "disp-table")
>> -      (load "dos-w32")
>> -      (load "w32-fns")))
>> +      (load "w32-common-fns")
>> +      (when (eq system-type 'windows-nt)
>> +        (load "w32-fns")
>> +        (load "ls-lisp")
>> +        (load "dos-w32"))))
> 
> Likewise here: at the very least, keep the order in the group of
> packages loaded for windows-nt.

I can play with the order a bit, but to avoid code duplication, I'd
strongly prefer to maintain the separation of the code in w32-common-fns
and w32-fns.  This separation necessarily involves changing the order in
which some definitions appear, but the difference hasn't seemed to cause
any trouble so far.

>> --- a/lisp/mouse.el
>> +++ b/lisp/mouse.el
>> @@ -1147,7 +1147,7 @@ regardless of where you click."
>>    (or mouse-yank-at-point (mouse-set-point click))
>>    (let ((primary
>>       (cond
>> -      ((eq system-type 'windows-nt)
>> +      ((eq (framep (selected-frame)) 'w32)
>>         ;; MS-Windows emulates PRIMARY in x-get-selection, but not
>>         ;; in x-get-selection-value (the latter only accesses the
>>         ;; clipboard).  So try PRIMARY first, in case they selected
> 
> You mean, the Cygwin build that uses w32 windowing will be unable to
> support X selections?  That would be a pity.

Why would it? Emacs in this configuration is a native Windows
application and it doesn't know a thing about X selections.  Any
improvements to the X selection emulation would apply equally well to
both cygwin and windows-nt configurations of the w32 window-system.

>> +(defun w32-handle-dropped-file (window file-name)
>> +  (let ((f (if (eq system-type 'cygwin)
>> +               (cygwin-convert-path-from-windows file-name t)
>> +             (subst-char-in-string ?\\ ?/ file-name)))
>> +        (coding (or file-name-coding-system
>> +                    default-file-name-coding-system)))
> 
> What is the default value of default-file-name-coding-system in the
> Cygwin build?  There could be a conflict here between e.g. UTF-8 in
> Cygwin and the Windows codepage.

In Cygwin, default-file-name-coding-system will almost always be
utf-8-unix because the default system locale is UTF-8.

This particular function is a bit hairy, however: FILE-NAME is _already_
decoded.  The only reason we're mucking with the coding system here is
that dnd-handle-one-url unconditionally decodes its argument using (or
file-name-coding-system default-file-name-coding-system), so we have to
re-encode what we give it.  (I don't want to bind
file-name-coding-system because that would affect the entire file
loading process.)

> Also, it would be good to have a doc string for this function.

Sure thing.

> 
>> +DEFUN ("cygwin-convert-path-to-windows",
>> +       Fcygwin_convert_path_to_windows, Scygwin_convert_path_to_windows,
>> +       1, 2, 0,
>> +       doc: /* Convert PATH to a Windows path.  If ABSOLUTE-P if
>> +               non-nil, return an absolute path.*/)
>> +  (Lisp_Object path, Lisp_Object absolute_p)
>> +{
>> +  return from_unicode (
>> +    conv_filename_to_w32_unicode (path, absolute_p == Qnil ? 0 : 1));
>> +}
>> +
>> +DEFUN ("cygwin-convert-path-from-windows",
>> +       Fcygwin_convert_path_from_windows, Scygwin_convert_path_from_windows,
>> +       1, 2, 0,
>> +       doc: /* Convert a Windows path to a Cygwin path.  If ABSOLUTE-P
>> +               if non-nil, return an absolute path.*/)
>> +  (Lisp_Object path, Lisp_Object absolute_p)
>> +{
>> +  return conv_filename_from_w32_unicode (to_unicode (path, &path),
>> +                                         absolute_p == Qnil ? 0 : 1);
>> +}
> 
> These two should probably be documented in the ELisp manual.

Along with Changelog entries, NEWS entries, and so on.

>>        char buffer[16];
>> -      _snprintf (buffer, 16, "%d", system_status.BatteryLifePercent);
>> +      snprintf (buffer, 16, "%d", system_status.BatteryLifePercent);
>>        load_percentage = build_string (buffer);
> 
> If you want to use snprintf without the leading underscore, please add
> a suitable #define somewhere for the benefit of the MSVC compiler,
> which doesn't have snprintf, only _snprintf.

Good catch. I'll add a preprocessor macro.

> Btw, will the above code be used by the Cygwin build?  Does that
> include default-printer-name etc.?

The battery status functionality works fine under the Cygwin build, as
does default-printer-name. (I haven't tried to actually print anything.)

signature.asc
Description: OpenPGP digital signature