* Some comments about changes in revno 110444
@ 2012-10-08 11:01 Eli Zaretskii
2012-10-08 12:53 ` Daniel Colascione
0 siblings, 1 reply; 3+ messages in thread
From: Eli Zaretskii @ 2012-10-08 11:01 UTC (permalink / raw)
To: Daniel Colascione; +Cc: emacs-devel
This hunk removes w32inevt from SOME_MACHINE_OBJECTS. Why was that
needed?
SOME_MACHINE_OBJECTS = dosfns.o msdos.o \
xterm.o xfns.o xmenu.o xselect.o xrdb.o xsmfns.o fringe.o image.o \
- fontset.o dbusbind.o \
+ fontset.o dbusbind.o cygw32.o \
nsterm.o nsfns.o nsmenu.o nsselect.o nsimage.o nsfont.o \
- w32.o w32console.o w32fns.o w32heap.o w32inevt.o \
+ w32.o w32console.o w32fns.o w32heap.o \
w32menu.o w32proc.o w32reg.o w32select.o w32term.o w32xfns.o \
w16select.o widget.o xfont.o ftfont.o xftfont.o ftxfont.o gtkutil.o \
xsettings.o xgselect.o termcap.o
To avoid confusion, I suggest to change the name of the variable here:
+ /* No, not utf-16-le: that one has a BOM. */
+ DEFSYM (Qutf_16_le, "utf-16le");
to Qutf_16le.
Isn't the below supposed to be done automagically by make-docfile,
which puts these in globals.h?
+EXFUN (Fcygwin_convert_path_to_windows, 2);
+EXFUN (Fcygwin_convert_path_from_windows, 2);
I don't think this (from w32select.h) is a good idea:
+#define HAVE_W32SELECT 1
We only have definitions for HAVE_* macros in config.h and
conf_post.h. Having them in other headers is confusing, IMO. Can we
move this to conf_post.h instead?
You moved the definitions of these from w32heap.c to w32fns.c:
+/* The major and minor versions of NT. */
+int w32_major_version;
+int w32_minor_version;
+int w32_build_number;
but they are still referenced by w32.c via get_* macros defined on
w32heap.h. Can we clean this up, please?
Regarding this:
+#ifdef NTGUI_UNICODE
+ wchar_t filename_buf[MAX_PATH + 1];
I believe Unicode names on Windows are not limited to MAX_PATH
characters.
It looks like there are some unnecessary hops here:
+ CHECK_STRING (prompt);
+ CHECK_STRING (dir);
+
+ dir = Fexpand_file_name (dir, Qnil);
+
+ if (STRINGP (filename))
+ filename = Ffile_name_nondirectory (filename);
+ else
+ filename = empty_unibyte_string;
+
+#ifdef CYGWIN
+ dir = Fcygwin_convert_path_to_windows (dir, Qt);
+ if (SCHARS (filename) > 0)
+ filename = Fcygwin_convert_path_to_windows (filename, Qnil);
+#endif
+ CHECK_STRING (dir);
+ CHECK_STRING (filename);
+
+ /* The code in file_dialog_callback that attempts to set the text
+ of the file name edit window when handling the CDN_INITDONE
+ WM_NOTIFY message does not work. Setting filename to "Current
+ Directory" in the only_dir_p case here does work however. */
+ if (SCHARS (filename) == 0 && ! NILP (only_dir_p))
+ filename = build_string ("Current Directory");
+
+ /* Convert the values we've computed so far to system form. */
+#ifdef NTGUI_UNICODE
+ to_unicode (prompt, &prompt);
+ to_unicode (dir, &dir);
+ to_unicode (filename, &filename);
AFAIU, inside Fcygwin_convert_path_to_windows, you convert the names
of the file and directory to UTF-16, then decode it back into a
multibyte string, and finally encode it again to UTF-16 when you call
to_unicode. Perhaps the back and forth conversion to and from UTF-16
can be avoided?
In any case, a call to CHECK_STRING after we already checked the
arguments seems both redundant and user-unfriendly: these tests are
designed to reject errors in arguments passed to primitives, so
signaling an error because something went wrong in processing valid
arguments doesn't sound like a good idea.
This error will not report the offending file name:
+ if (SBYTES (filename) + 1 > sizeof (filename_buf))
+ error ("filename too long");
I think report_file_error is better here.
Why do you need the fprintf part here:
#ifdef EMACSDEBUG
void
_DebPrint (const char *fmt, ...)
{
char buf[1024];
va_list args;
va_start (args, fmt);
vsprintf (buf, fmt, args);
va_end (args);
#if CYGWIN
fprintf (stderr, "%s", buf);
#endif
OutputDebugString (buf);
}
What happened to number 21 in the change below?
#define WM_EMACS_HIDE_CARET (WM_EMACS_START + 18)
#define WM_EMACS_SETCURSOR (WM_EMACS_START + 19)
#define WM_EMACS_PAINT (WM_EMACS_START + 20)
-#define WM_EMACS_BRINGTOTOP (WM_EMACS_START + 21)
-#define WM_EMACS_END (WM_EMACS_START + 22)
+#define WM_EMACS_BRINGTOTOP (WM_EMACS_START + 22)
+#define WM_EMACS_INPUT_READY (WM_EMACS_START + 23)
+#define WM_EMACS_END (WM_EMACS_START + 24)
Why did you need the call to notify_msg_ready below?
@@ -285,7 +309,7 @@ prepend_msg (W32Msg *lpmsg)
nQueue++;
lpNew->lpNext = lpHead;
lpHead = lpNew;
-
+ notify_msg_ready ();
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Some comments about changes in revno 110444
2012-10-08 11:01 Some comments about changes in revno 110444 Eli Zaretskii
@ 2012-10-08 12:53 ` Daniel Colascione
2012-10-08 13:50 ` Eli Zaretskii
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Colascione @ 2012-10-08 12:53 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 6053 bytes --]
On 10/8/2012 4:01 AM, Eli Zaretskii wrote:
> This hunk removes w32inevt from SOME_MACHINE_OBJECTS. Why was that
> needed?
Merge problem. Fixed.
> To avoid confusion, I suggest to change the name of the variable here:
>
> + /* No, not utf-16-le: that one has a BOM. */
> + DEFSYM (Qutf_16_le, "utf-16le");
>
> to Qutf_16le.
Good catch.
> Isn't the below supposed to be done automagically by make-docfile,
> which puts these in globals.h?
>
> +EXFUN (Fcygwin_convert_path_to_windows, 2);
> +EXFUN (Fcygwin_convert_path_from_windows, 2);
The earliest version of this code targeted Emacs 23, which doesn't have a
globals.h. Fixed.
> I don't think this (from w32select.h) is a good idea:
>
> +#define HAVE_W32SELECT 1
>
> We only have definitions for HAVE_* macros in config.h and
> conf_post.h. Having them in other headers is confusing, IMO. Can we
> move this to conf_post.h instead?
We only used HAVE_W32SELECT in two places, so I just replaced HAVE_W32SELECT
with the conditional we use to include the header --- WINDOWSNT || HAVE_NTGUI.
> You moved the definitions of these from w32heap.c to w32fns.c:
>
> +/* The major and minor versions of NT. */
> +int w32_major_version;
> +int w32_minor_version;
> +int w32_build_number;
>
> but they are still referenced by w32.c via get_* macros defined on
> w32heap.h. Can we clean this up, please?
The bigger issue is that w32heap.c is WINDOWSNT-only, but w32heap.h declares
useful facilities that apply more generally (and that have nothing to do with
the heap). I created a new w32common.h for these common declarations and left
the WINDOWSNT-specific heap stuff in w32heap.h.
> Regarding this:
>
> +#ifdef NTGUI_UNICODE
> + wchar_t filename_buf[MAX_PATH + 1];
>
> I believe Unicode names on Windows are not limited to MAX_PATH
> characters.
You can use special syntax to exceed that limit ---
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
--- but nobody does in practice. Bear in mind that this code only deals with
the file open dialog box, which (the last time I checked) didn't support long
file names in the first place.
Still, I've increased the limit to 32k + 1, which is a hard NT kernel limit, in
the NTGUI_UNICODE case.
> It looks like there are some unnecessary hops here:
>
> + CHECK_STRING (prompt);
> + CHECK_STRING (dir);
> +
> + dir = Fexpand_file_name (dir, Qnil);
> +
> + if (STRINGP (filename))
> + filename = Ffile_name_nondirectory (filename);
> + else
> + filename = empty_unibyte_string;
> +
> +#ifdef CYGWIN
> + dir = Fcygwin_convert_path_to_windows (dir, Qt);
> + if (SCHARS (filename) > 0)
> + filename = Fcygwin_convert_path_to_windows (filename, Qnil);
> +#endif
>
> + CHECK_STRING (dir);
> + CHECK_STRING (filename);
> +
> + /* The code in file_dialog_callback that attempts to set the text
> + of the file name edit window when handling the CDN_INITDONE
> + WM_NOTIFY message does not work. Setting filename to "Current
> + Directory" in the only_dir_p case here does work however. */
> + if (SCHARS (filename) == 0 && ! NILP (only_dir_p))
> + filename = build_string ("Current Directory");
> +
> + /* Convert the values we've computed so far to system form. */
> +#ifdef NTGUI_UNICODE
> + to_unicode (prompt, &prompt);
> + to_unicode (dir, &dir);
> + to_unicode (filename, &filename);
>
> AFAIU, inside Fcygwin_convert_path_to_windows, you convert the names
> of the file and directory to UTF-16, then decode it back into a
> multibyte string, and finally encode it again to UTF-16 when you call
> to_unicode. Perhaps the back and forth conversion to and from UTF-16
> can be avoided?
This code isn't on a hot path. I'm reluctant to optimize it: I'm not sure the
additional complexity would be worth it.
> In any case, a call to CHECK_STRING after we already checked the
> arguments seems both redundant and user-unfriendly: these tests are
> designed to reject errors in arguments passed to primitives, so
> signaling an error because something went wrong in processing valid
> arguments doesn't sound like a good idea.
>
> This error will not report the offending file name:
>
> + if (SBYTES (filename) + 1 > sizeof (filename_buf))
> + error ("filename too long");
>
> I think report_file_error is better here.
Fixed.
> Why do you need the fprintf part here:
> #ifdef EMACSDEBUG
> void
> _DebPrint (const char *fmt, ...)
> {
> char buf[1024];
> va_list args;
>
> va_start (args, fmt);
> vsprintf (buf, fmt, args);
> va_end (args);
> #if CYGWIN
> fprintf (stderr, "%s", buf);
> #endif
> OutputDebugString (buf);
> }
Because tracing on stderr is useful. Strings sent to OutputDebugString are
invisible unless Emacs is being run under a debugger.
> What happened to number 21 in the change below?
Bad merge. Fixed.
> #define WM_EMACS_HIDE_CARET (WM_EMACS_START + 18)
> #define WM_EMACS_SETCURSOR (WM_EMACS_START + 19)
> #define WM_EMACS_PAINT (WM_EMACS_START + 20)
> -#define WM_EMACS_BRINGTOTOP (WM_EMACS_START + 21)
> -#define WM_EMACS_END (WM_EMACS_START + 22)
> +#define WM_EMACS_BRINGTOTOP (WM_EMACS_START + 22)
> +#define WM_EMACS_INPUT_READY (WM_EMACS_START + 23)
> +#define WM_EMACS_END (WM_EMACS_START + 24)
>
> Why did you need the call to notify_msg_ready below?
>
> @@ -285,7 +309,7 @@ prepend_msg (W32Msg *lpmsg)
> nQueue++;
> lpNew->lpNext = lpHead;
> lpHead = lpNew;
> -
> + notify_msg_ready ();
The call to notify_msg_ready is needed because after a call to prepend_msg, a
message is now ready. It should have been there all along.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Some comments about changes in revno 110444
2012-10-08 12:53 ` Daniel Colascione
@ 2012-10-08 13:50 ` Eli Zaretskii
0 siblings, 0 replies; 3+ messages in thread
From: Eli Zaretskii @ 2012-10-08 13:50 UTC (permalink / raw)
To: Daniel Colascione; +Cc: emacs-devel
> Date: Mon, 08 Oct 2012 05:53:52 -0700
> From: Daniel Colascione <dancol@dancol.org>
> CC: emacs-devel@gnu.org
>
> > Why did you need the call to notify_msg_ready below?
> >
> > @@ -285,7 +309,7 @@ prepend_msg (W32Msg *lpmsg)
> > nQueue++;
> > lpNew->lpNext = lpHead;
> > lpHead = lpNew;
> > -
> > + notify_msg_ready ();
>
> The call to notify_msg_ready is needed because after a call to prepend_msg, a
> message is now ready. It should have been there all along.
I guess no one have seen a MSH_MOUSEWHEEL message in Emacs in a long,
long, LONG time ;-)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-10-08 13:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-08 11:01 Some comments about changes in revno 110444 Eli Zaretskii
2012-10-08 12:53 ` Daniel Colascione
2012-10-08 13:50 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).