[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] system-type cygwin with window-system w32
|
From: |
Eli Zaretskii |
|
Subject: |
Re: [PATCH] system-type cygwin with window-system w32 |
|
Date: |
Mon, 18 Jul 2011 02:13:40 -0400 |
> Date: Sun, 17 Jul 2011 17:01:38 -0700
> From: Daniel Colascione <address@hidden>
>
> 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...
Re: [PATCH] system-type cygwin with window-system w32, Eli Zaretskii, 2011/07/18