From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Adrian Robert Newsgroups: gmane.emacs.devel Subject: Re: a review of the merge (Re: Emacs.app merged) Date: Wed, 16 Jul 2008 21:25:34 -0400 Message-ID: References: <1C66F1FC-BF82-4365-944D-ADCC4D1F435C@gmail.com> <200807160925.m6G9PuVj012462@sallyv1.ics.uci.edu> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 (Apple Message framework v926) Content-Type: multipart/alternative; boundary=Apple-Mail-70--419950230 X-Trace: ger.gmane.org 1216257992 29433 80.91.229.12 (17 Jul 2008 01:26:32 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 17 Jul 2008 01:26:32 +0000 (UTC) Cc: emacs- devel To: Dan Nicolaescu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Jul 17 03:27:18 2008 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1KJIGw-0003zY-Vi for ged-emacs-devel@m.gmane.org; Thu, 17 Jul 2008 03:27:17 +0200 Original-Received: from localhost ([127.0.0.1]:57787 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KJIG3-0002jS-G4 for ged-emacs-devel@m.gmane.org; Wed, 16 Jul 2008 21:26:11 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KJIFx-0002j2-I6 for emacs-devel@gnu.org; Wed, 16 Jul 2008 21:26:05 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KJIFv-0002ii-OO for emacs-devel@gnu.org; Wed, 16 Jul 2008 21:26:05 -0400 Original-Received: from [199.232.76.173] (port=39145 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KJIFv-0002id-Ii for emacs-devel@gnu.org; Wed, 16 Jul 2008 21:26:03 -0400 Original-Received: from an-out-0708.google.com ([209.85.132.241]:2738) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KJIFu-00063Q-Sv for emacs-devel@gnu.org; Wed, 16 Jul 2008 21:26:03 -0400 Original-Received: by an-out-0708.google.com with SMTP id c38so164486ana.84 for ; Wed, 16 Jul 2008 18:26:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:cc:message-id:from:to :in-reply-to:content-type:mime-version:subject:date:references :x-mailer; bh=LvpkUSTa4hITOk1f7hJqXrVuPppafMof4a5NkxfVDWc=; b=IZfMaXQ/5EzJqsg8gsFIQAd9ZTINc3smp0Y5KHvGr8cc0ApRLIQboYoQIqkzFpFRwA trjBEj29KWII3fAS6kPUid3U7Tc9rXTaAgpfVfonxbRB7xdsD3h7LZpjZGI3RXFgc5o/ l6jAYqPg01kIVeXAytgB49SOKl8+fG7iqd5mg= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=cc:message-id:from:to:in-reply-to:content-type:mime-version:subject :date:references:x-mailer; b=HktstsR2beGHQmDO5uNbjntEfrPjEdD/trAZpVK5bIPEDPR86AMbT7ph6NATnRFpCX UC7paTLsMVzyINkhfyVMnlqV3+bme//8xpzeLJX1JnvzpM9z9/5/PuDtZI6MR81GLS2f VzznWw0+7O73jscQjoweeu+O/4DbvfMpmS5+c= Original-Received: by 10.100.225.19 with SMTP id x19mr3188353ang.151.1216257961066; Wed, 16 Jul 2008 18:26:01 -0700 (PDT) Original-Received: from ?10.0.1.200? ( [97.73.30.9]) by mx.google.com with ESMTPS id 5sm9235191ywl.4.2008.07.16.18.25.48 (version=TLSv1/SSLv3 cipher=RC4-MD5); Wed, 16 Jul 2008 18:26:00 -0700 (PDT) In-Reply-To: <200807160925.m6G9PuVj012462@sallyv1.ics.uci.edu> X-Mailer: Apple Mail (2.926) X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 2) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:100856 Archived-At: --Apple-Mail-70--419950230 Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit On Jul 16, 2008, at 5:25 AM, Dan Nicolaescu wrote: > Adrian Robert writes: > >> http://cortex.med.cornell.edu/~arobert/emacs-app/ >> diffBeforeMerge.patch > > I looked over this patch and I wrote quite a few comments. Can you > please look over and try to address them? It would be good if you > could > find a system to keep track of these comments so that they don't get > lost. Some of the comments from the previous rounds have been > lost :-(. Thanks for the comments and sorry if some previous ones have been inadvertently left unaddresses. >> The newly-added 'nextstep' directory contains additional information. >> FOR-RELEASE in that directory contains a list of TODOs before release >> of 23.1. > > Why not just use admin/FOR-RELEASE? It's easier to look in just one > place. This is up to the maintainers. I thought while the port is settling down and there are a lot of issues, it might be good to keep them from cluttering up the admin file. > Also please get rid of the nextstep/compile file, it's not a good idea > to have to document another way of building emacs, especially since > ./configure; make should be good enough for this platform. I'm working on this... > +2008-07-15 Adrian Robert > + > + * Emacs.clr: New file, add support for X color names to NS display > + implementations. > > Is this really needed? All other platforms do just fine by > definining x-colors in elisp. Actually, the Carbon and Windows ports put these defs in source code, macfns.c and w32fns.c respectively. Keeping them in a separate data file seems a little cleaner, and also fits well with the way color lists are handled in NeXTstep. But the defs can be moved into source code if that is preferred. > Index: lib-src/Makefile.in > =================================================================== > RCS file: /sources/emacs/emacs/lib-src/Makefile.in,v > retrieving revision 1.163 > diff -a -u -r1.163 Makefile.in > --- lib-src/Makefile.in 9 May 2008 23:19:10 -0000 1.163 > +++ lib-src/Makefile.in 15 Jul 2008 16:50:14 -0000 > @@ -144,0 +144,8 @@ > +#ifdef HAVE_NS > +.m.o: > +#ifdef GNUSTEP > + $(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) -fgnu-runtime -Wno-import - > fconstant-string-class=NSConstantString $< > +#else > + $(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) $< > +#endif > +#endif > > No need for the extra HAVE_NS conditional. Done. > +#if defined(COCOA) > +mac-fix-env: ${srcdir}/mac-fix-env.m > + $(CC) -o mac-fix-env ${srcdir}/mac-fix-env.m -prebind -framework > Foundation > +#endif > > No need to make it conditional. And shouldn't this use > NS_IMPL_COCOA instead of COCOA ? GNUSTEP and COCOA were used instead of the NS_IMPL ones in configure. Changed everywhere for consistency. > Index: lisp/facemenu.el > =================================================================== > RCS file: /sources/emacs/emacs/lisp/facemenu.el,v > retrieving revision 1.104 > diff -a -u -r1.104 facemenu.el > --- lisp/facemenu.el 6 May 2008 07:57:34 -0000 1.104 > +++ lisp/facemenu.el 15 Jul 2008 16:52:14 -0000 > @@ -460,10 +460,11 @@ > (defun facemenu-read-color (&optional prompt) > "Read a color using the minibuffer." > (let* ((completion-ignore-case t) > + (require-match (not (eq window-system 'ns))) > (col (completing-read (or prompt "Color: ") > (or facemenu-color-alist > (defined-colors)) > - nil t))) > + nil require-match))) > (if (equal "" col) > nil > col))) > > Why do this? > > This does not seem specific to NS, if it is not, then it should be > discussed and checked in separately. This is so users can enter colors in numeric format, such as ARGBD0FFFFFF. The NS port interprets such formats to allow alpha specification. > Index: lisp/frame.el > ... > > Is this necessary? Can NS make frames on another display? If not, > then this should not be needed. The problem this solves is requesting a GUI frame from a terminal session using make-frame-on-display. As it was, only display formats compatible with X-Windows were accepted. This allows anything non-nil to work, since right now only one display is supported under NS. > Index: lisp/loadup.el > =================================================================== > RCS file: /sources/emacs/emacs/lisp/loadup.el,v > retrieving revision 1.169 > diff -a -u -r1.169 loadup.el > --- lisp/loadup.el 21 Jun 2008 01:38:37 -0000 1.169 > +++ lisp/loadup.el 15 Jul 2008 16:53:09 -0000 > @@ -212,3 +212,6 @@ > (if (featurep 'mac-carbon) > (progn > (load "term/mac-win"))) > +(if (featurep 'ns-windowing) > + (progn > + (load "emacs-lisp/easymenu") ;; for platform-related menu > adjustments > > RMS was very much against having different platforms change the menus. > Is that policy changed? If not, then this should not be needed. > And there's no need to use easymenu to modify menus. This is not done by default, only when ns-extended-platform-support is turned on. And it makes only very minor modifications, for purpose of enhancing platform consistency. Anyway, if this is retained, one option would be to move it out into a separately-loaded file (not included in dumped emacs). Another would be to manually do what easymenu does (but this would be ugly). > + (load "emacs-lisp/easy-mmode") > > Same as above, this is for adding platform specific behavior. Removed as per other discussion. > Index: lisp/startup.el > =================================================================== > RCS file: /sources/emacs/emacs/lisp/startup.el,v > retrieving revision 1.494 > diff -a -u -r1.494 startup.el > --- lisp/startup.el 2 Jul 2008 01:49:01 -0000 1.494 > +++ lisp/startup.el 15 Jul 2008 16:54:18 -0000 > @@ -182,3 +182,6 @@ > and VALUE is the value which is given to that frame parameter > \(most options use the argument for this, so VALUE is not present).") > > +(defconst command-line-ns-option-alist > + '(("-NSAutoLaunch" 1 ns-ignore-1-arg) > + ("-NXAutoLaunch" 1 ns-ignore-1-arg) > [snip] > > Can this be put somewhere else? It would be better if all other > platforms > do not have to load this definition. > + ;; Add the long NS options to longopts. > + (setq tem command-line-ns-option-alist) > + (while tem > + (if (string-match "^--" (car (car tem))) > + (setq longopts (cons (list (car (car tem))) longopts))) > + (setq tem (cdr tem))) > > Can this be avoided and use the generic code for command line > processing? NS has followed the X GUI in both these cases, using ns- prefix to distinguish variables for option lists, etc. that are specific to the NS platform, as X- is used to indicate X-windows. I do realize that w32 and mac (Carbon) seem to deal with their arguments in ways not involving this file. The question is, should every platform be done in the same way as X, or should X and NS be changed to whatever mac and w32 are doing? > +#ifdef HAVE_NS > +abbrev.o buffer.o callint.o cmds.o dispnew.o editfns.o fileio.o > frame.o \ > + fontset.o indent.o insdel.o keyboard.o macros.o minibuf.o msdos.o > process.o \ > + scroll.o sysdep.o term.o widget.o window.o xdisp.o xfaces.o > xfns.o \ > + xterm.o xselect.o sound.o: nsgui.h > +nsfns.o: nsfns.m charset.h nsterm.h nsgui.h frame.h window.h > buffer.h \ > + dispextern.h nsgui.h fontset.h $(INTERVAL_SRC) keyboard.h > blockinput.h \ > + atimer.h systime.h epaths.h termhooks.h coding.h systime.h $ > (config_h) > +nsmenu.o: nsmenu.m termhooks.h frame.h window.h dispextern.h \ > + nsgui.h keyboard.h blockinput.h atimer.h systime.h buffer.h \ > + nsterm.h $(config_h) > +nsterm.o: nsterm.m blockinput.h atimer.h systime.h syssignal.h > nsterm.h \ > + nsgui.h frame.h charset.h ccl.h dispextern.h fontset.h > termhooks.h \ > + termopts.h termchar.h disptab.h buffer.h window.h keyboard.h \ > + $(INTERVAL_SRC) process.h coding.h $(config_h) > +nsselect.o: nsselect.m blockinput.h nsterm.h nsgui.h frame.h $ > (config_h) > +nsimage.o: nsimage.m nsterm.h > +nsfont.o: nsterm.h dispextern.h frame.h lisp.h $(config_h) > > Better make this unconditional. Here I aped what the Carbon port does just above these lines. If it's wrong, which example should I follow? > Index: src/frame.c > ... > Fselect_window (XFRAME (frame)->selected_window, Qnil); > > +#ifdef NS_IMPL_COCOA > + /* term gets no other notification of this */ > + if (for_deletion) > + Fraise_frame(Qnil); > +#endif > > Why isn't his needed for other platforms too? I don't know. I would be happy to get rid of it if I knew. > +#ifndef HAVE_NS /* PENDING: ensure font attrs change goes through */ > > Better use "FIXME" instead of "PENDING". Are there other options besides FIXME? I use PENDING to flag something that is not necessarily a bug or even needing change, but that needs to be considered. What about TODO? > Index: src/fringe.c > =================================================================== > RCS file: /sources/emacs/emacs/src/fringe.c,v > retrieving revision 1.52 > diff -a -u -r1.52 fringe.c > --- src/fringe.c 14 May 2008 07:49:32 -0000 1.52 > +++ src/fringe.c 15 Jul 2008 16:59:29 -0000 > @@ -482,4 +482,4 @@ > static Lisp_Object *fringe_faces; > static int max_fringe_bitmaps; > > -static int max_used_fringe_bitmap = MAX_STANDARD_FRINGE_BITMAPS; > +int max_used_fringe_bitmap = MAX_STANDARD_FRINGE_BITMAPS; > > > Why? This is not in the ChangeLog? Undo please if not needed. Used for memory allocation handling in nsterm.m (ns_draw_fringe_bitmap(), from line 2163). It is in the ChangeLog: * fringe.c (max_used_fringe_bitmap): Make public. > Index: src/getloadavg.c > =================================================================== > RCS file: /sources/emacs/emacs/src/getloadavg.c,v > retrieving revision 1.53 > diff -a -u -r1.53 getloadavg.c > --- src/getloadavg.c 8 Jan 2008 20:44:33 -0000 1.53 > +++ src/getloadavg.c 15 Jul 2008 16:59:32 -0000 > > This file comes from gnulib, we try not to change it here. It > should go there first. > Why wasn't this needed until now? This should not have anything to > do with NS... I don't know, probably it was never working. I actually am not sure if it is now, but anyway the original is using #ifdef NeXT to check whether mach.h is in a subdirectory, but it seems all NeXT-derived systems put it in the same place (e.g., my OS X 10.5.4 machine here). ACTUALLY, it looks like this file is not even used anymore (grep thru Makefile.in), so probably it should just be removed. > Index: src/keyboard.c > =================================================================== > RCS file: /sources/emacs/emacs/src/keyboard.c,v > retrieving revision 1.960 > diff -a -u -r1.960 keyboard.c > --- src/keyboard.c 26 Jun 2008 04:24:36 -0000 1.960 > +++ src/keyboard.c 15 Jul 2008 17:00:59 -0000 > @@ -7305,3 +7312,7 @@ > void > handle_async_input () > { > +#ifdef BSD4_1 > + extern int select_alarmed; > +#endif > + > > Please remove this or use HAVE_NS instead, BSD4_1 is not defined in > the emacs tree anymore. This probably causes big problems with the > input if this code is needed. > > @@ -7317,6 +7328,9 @@ > if (nread <= 0) > break; > > +#ifdef BSD4_1 > + select_alarmed = 1; /* Force the select emulator back to > life */ > +#endif > > Same here. > > @@ -7335,3 +7349,6 @@ > signal (signo, input_available_signal); > #endif /* USG */ > > +#ifdef BSD4_1 > + sigisheld (SIGIO); > +#endif > > Same here. > > @@ -7348,3 +7366,6 @@ > handle_async_input (); > #endif > > +#ifdef BSD4_1 > + sigfree (); > +#endif > > And here. These all must be spurious merge conflicts. Removed. Sorry about this. > @@ -8044,7 +8070,12 @@ > && SYMBOLP (XSYMBOL (def)->function) > && ! NILP (Fget (def, Qmenu_alias))) > def = XSYMBOL (def)->function; > +#ifdef HAVE_NS > + /* prefer 'super' bindings */ > + tem = Fwhere_is_internal (def, Qnil, Qsuper, Qt, Qt); > +#else > tem = Fwhere_is_internal (def, Qnil, Qt, Qnil, Qt); > +#endif > > > Please run this by Stefan, not sure if we want to have a platform do > something different here. Yes, I was working to make this only happen when ns-extended-platform- support-mode is on, but it's a bit tricky since this only gets called once. > Index: src/keymap.c > =================================================================== > RCS file: /sources/emacs/emacs/src/keymap.c,v > retrieving revision 1.374 > diff -a -u -r1.374 keymap.c > --- src/keymap.c 5 Jun 2008 05:44:11 -0000 1.374 > +++ src/keymap.c 15 Jul 2008 17:01:22 -0000 > @@ -111,3 +111,6 @@ > > extern Lisp_Object Voverriding_local_map; > > +#ifdef HAVE_NS > +extern Lisp_Object Qalt, Qcontrol, Qhyper, Qmeta, Qsuper; > +#endif > > Please get the changes in this file approved by Stefan, they look a > bit suspicious. OK, being worked on. > Index: src/lisp.h > =================================================================== > RCS file: /sources/emacs/emacs/src/lisp.h,v > retrieving revision 1.631 > diff -a -u -r1.631 lisp.h > --- src/lisp.h 11 Jul 2008 14:20:06 -0000 1.631 > +++ src/lisp.h 15 Jul 2008 17:01:36 -0000 > @@ -28,3 +28,7 @@ > #define P_(proto) () > #endif > > +#ifdef NS_IMPL_GNUSTEP > +/* This conflicts with functions in the GNUstep libraries. */ > +#define hash_remove emacs_hash_remove > +#endif /* NS_IMPL_GNUSTEP */ > > Sounds odd, but if this is indeed true, better rename the function in > emacs to avoid extra #ifdefs. Yes, hash_put, hash_remove, etc. are all used in those libs. Only hash_remove is declared so publicly within emacs though. > Index: src/terminfo.c > =================================================================== > RCS file: /sources/emacs/emacs/src/terminfo.c,v > retrieving revision 1.24 > diff -a -u -r1.24 terminfo.c > --- src/terminfo.c 14 May 2008 07:49:52 -0000 1.24 > +++ src/terminfo.c 15 Jul 2008 17:03:07 -0000 > @@ -24,4 +24,7 @@ > so that we do not need to conditionalize the places in Emacs > that set them. */ > > +/* Causes a conflict on OS X 10.3 .*/ > +#ifndef NS_IMPL_COCOA > char *UP, *BC, PC; > +#endif > > Does "emacs -nw" work after doing this? How come this wasn't a > problem with the Carbon port? Yes, and I don't know. It's possible something is different in the includes brought in by Carbon vs. Cocoa. > Index: src/xdisp.c > =================================================================== > RCS file: /sources/emacs/emacs/src/xdisp.c,v > retrieving revision 1.1233 > diff -a -u -r1.1233 xdisp.c > --- src/xdisp.c 15 Jul 2008 15:45:05 -0000 1.1233 > +++ src/xdisp.c 15 Jul 2008 17:05:45 -0000 > @@ -22539,7 +22576,10 @@ > /* Switch the display of W's cursor on or off, according to the value > of ON. */ > > -static void > +#ifndef HAVE_NS > +static > +#endif > +void > update_window_cursor (w, on) > struct window *w; > int on; > > Why? It doesn't seem to be used outside this file. nsterm.m uses it for cursor blink. Yes, I know, it should use the generic mechanism for this, and that is a longstanding TODO. Help with it is welcome. Once that's done this can be removed. thanks for the comments, Adrian --Apple-Mail-70--419950230 Content-Type: text/html; charset=US-ASCII Content-Transfer-Encoding: quoted-printable
On Jul 16, 2008, = at 5:25 AM, Dan Nicolaescu wrote:

Adrian = Robert <adrian.b.robert@gmail.com> = writes:

http://cortex.med.cornell.edu/~arobert/emacs-app/= diffBeforeMerge.patch

I looked over this patch and I = wrote quite a few comments.  Can you
please look over and try to = address them?  It would be good if you could
find a system to = keep track of these comments so that they don't get
lost.  Some = of the comments from the previous rounds have been lost = :-(.

Thanks for the comments and = sorry if some previous ones have been inadvertently left = unaddresses.



The newly-added 'nextstep' = directory contains additional information.
FOR-RELEASE in that directory contains a list of TODOs = before release
of = 23.1.

Why not just use admin/FOR-RELEASE? It's = easier to look in just one = place.

This is up to the = maintainers.  I thought while the port is settling down and there = are a lot of issues, it might be good to keep them from cluttering up = the admin file.



Also please get rid of the nextstep/compile file, = it's not a good idea
to have to document another way of building = emacs, especially since
./configure; make should be good enough for = this platform.

I'm working on = this...



+2008-07-15  Adrian Robert <Adrian.B.Robert@gmail.com>+
+ = * Emacs.clr: New file, add support for X color names to NS = display
+ = implementations.

Is this really needed?  All other = platforms do just fine by definining x-colors in = elisp.

Actually, the Carbon and = Windows ports put these defs in source code, macfns.c and w32fns.c = respectively.  Keeping them in a separate data file seems a little = cleaner, and also fits well with the way color lists are handled in = NeXTstep.  But the defs can be moved into source code if that is = preferred.



Index: = lib-src/Makefile.in
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
RCS file: /sources/emacs/emacs/lib-src/Makefile.in,v
retrieving = revision 1.163
diff -a -u -r1.163 Makefile.in
--- = lib-src/Makefile.in 9 May 2008 23:19:10 -0000 = 1.163
+++ lib-src/Makefile.in 15 Jul 2008 16:50:14 -0000
@@ = -144,0 +144,8 @@
+#ifdef HAVE_NS
+.m.o:
+#ifdef = GNUSTEP
+ = $(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) -fgnu-runtime -Wno-import = -fconstant-string-class=3DNSConstantString $<
+#else
+ $(CC) -c = $(CPPFLAGS) $(ALL_CFLAGS) $<
+#endif
+#endif

No need for = the extra HAVE_NS = conditional.

Done.



+#if = defined(COCOA)
+mac-fix-env: ${srcdir}/mac-fix-env.m
+ $(CC) -o = mac-fix-env ${srcdir}/mac-fix-env.m -prebind -framework = Foundation
+#endif

No need to make it conditional.  And = shouldn't this use NS_IMPL_COCOA instead of COCOA = ?

GNUSTEP and COCOA were used = instead of the NS_IMPL ones in configure.  Changed everywhere for = consistency.



Index: = lisp/facemenu.el
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3DRCS file: /sources/emacs/emacs/lisp/facemenu.el,v
retrieving = revision 1.104
diff -a -u -r1.104 facemenu.el
--- = lisp/facemenu.el = 6 May 2008 07:57:34 -0000 1.104
+++ = lisp/facemenu.el = 15 Jul 2008 16:52:14 -0000
@@ -460,10 +460,11 @@
(defun = facemenu-read-color (&optional prompt)
  "Read a color = using the minibuffer."
  (let* ((completion-ignore-case = t)
+ = (require-match (not (eq window-system 'ns)))
(col = (completing-read (or prompt "Color: ")
=       (or facemenu-color-alist
=   (defined-colors))
- =       nil t)))
+ =       nil require-match)))
=     (if (equal "" col)
nil
=       col)))

Why do = this?

This does not seem specific to NS, if it is not, then it = should be
discussed and checked in = separately.

This is so users can = enter colors in numeric format, such as ARGBD0FFFFFF.  The NS port = interprets such formats to allow alpha = specification.



Index: lisp/frame.el
...

Is this = necessary?  Can NS make frames on another display?  If = not,
then this should not be = needed.

The problem this solves is = requesting a GUI frame from a terminal session using = make-frame-on-display.  As it was, only display formats compatible = with X-Windows were accepted.  This allows anything non-nil to = work, since right now only one display is supported under = NS.



Index: = lisp/loadup.el
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
R= CS file: /sources/emacs/emacs/lisp/loadup.el,v
retrieving revision = 1.169
diff -a -u -r1.169 loadup.el
--- lisp/loadup.el 21 Jun = 2008 01:38:37 -0000 1.169
+++ lisp/loadup.el 15 Jul = 2008 16:53:09 -0000
@@ -212,3 +212,6 @@
(if (featurep = 'mac-carbon)
    (progn
=       (load "term/mac-win")))
+(if = (featurep 'ns-windowing)
+    (progn
+ =      (load "emacs-lisp/easymenu")  ;; for = platform-related menu adjustments

RMS was very much against = having different platforms change the menus.
Is that policy changed? =  If not, then this should not be needed.
And there's no need to = use easymenu to modify menus.

This = is not done by default, only when ns-extended-platform-support is turned = on.  And it makes only very minor modifications, for purpose of = enhancing platform consistency.  Anyway, if this is retained, one = option would be to move it out into a separately-loaded file (not = included in dumped emacs).  Another would be to manually do what = easymenu does (but this would be = ugly).



+      (load = "emacs-lisp/easy-mmode")

Same as above, this is for adding = platform specific = behavior.

Removed as per other = discussion.



Index: = lisp/startup.el
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
R= CS file: /sources/emacs/emacs/lisp/startup.el,v
retrieving revision = 1.494
diff -a -u -r1.494 startup.el
--- lisp/startup.el 2 Jul = 2008 01:49:01 -0000 1.494
+++ lisp/startup.el 15 Jul = 2008 16:54:18 -0000
@@ -182,3 +182,6 @@
and VALUE is the value = which is given to that frame parameter
\(most options use the = argument for this, so VALUE is not present).")

+(defconst = command-line-ns-option-alist
+  '(("-NSAutoLaunch" 1 = ns-ignore-1-arg)
+    ("-NXAutoLaunch" 1 = ns-ignore-1-arg)
[snip]

Can this be put somewhere else? =  It would be better if all other platforms
do not have to load = this definition.
+ =      ;; Add the long NS options to = longopts.
+      (setq tem = command-line-ns-option-alist)
+      (while = tem
+ = (if (string-match "^--" (car (car tem)))
+ =    (setq longopts (cons (list (car (car tem))) = longopts)))
+ = (setq tem (cdr tem)))

Can this be avoided and use the = generic code for command line = processing?


NS = has followed the X GUI in both these cases, using ns- prefix to = distinguish variables for option lists, etc. that are specific to the NS = platform, as X- is used to indicate X-windows.  I do realize that = w32 and mac (Carbon) seem to deal with their arguments in ways not = involving this file.  The question is, should every platform be = done in the same way as X, or should X and NS be changed to whatever mac = and w32 are = doing?



+#ifdef HAVE_NS
+abbrev.o buffer.o callint.o = cmds.o dispnew.o editfns.o fileio.o frame.o \
+  fontset.o = indent.o insdel.o keyboard.o macros.o minibuf.o msdos.o process.o \
+ =  scroll.o sysdep.o term.o widget.o window.o xdisp.o xfaces.o xfns.o = \
+  xterm.o xselect.o sound.o: nsgui.h
+nsfns.o: nsfns.m = charset.h nsterm.h nsgui.h frame.h window.h buffer.h \
+ =  dispextern.h nsgui.h fontset.h $(INTERVAL_SRC) keyboard.h = blockinput.h \
+  atimer.h systime.h epaths.h termhooks.h = coding.h systime.h $(config_h)
+nsmenu.o: nsmenu.m termhooks.h = frame.h window.h dispextern.h \
+  nsgui.h keyboard.h = blockinput.h atimer.h systime.h buffer.h \
+  nsterm.h = $(config_h)
+nsterm.o: nsterm.m blockinput.h atimer.h systime.h = syssignal.h nsterm.h \
+  nsgui.h frame.h charset.h ccl.h = dispextern.h fontset.h termhooks.h \
+  termopts.h termchar.h = disptab.h buffer.h window.h keyboard.h \
+  $(INTERVAL_SRC) = process.h coding.h $(config_h)
+nsselect.o: nsselect.m blockinput.h = nsterm.h nsgui.h frame.h $(config_h)
+nsimage.o: nsimage.m = nsterm.h
+nsfont.o: nsterm.h dispextern.h frame.h lisp.h = $(config_h)

Better make this = unconditional.

Here I aped what = the Carbon port does just above these lines.  If it's wrong, which = example should I = follow?



Index: src/frame.c
...
=   Fselect_window (XFRAME (frame)->selected_window, = Qnil);

+#ifdef NS_IMPL_COCOA
+  /* term gets no other = notification of this */
+  if (for_deletion)
+ =    Fraise_frame(Qnil);
+#endif

Why isn't his = needed for other platforms too?

I = don't know.  I would be happy to get rid of it if I = knew.



+#ifndef HAVE_NS  /* PENDING: ensure font attrs = change goes through */

Better use "FIXME" instead of = "PENDING".

Are there other options = besides FIXME?  I use PENDING to flag something that is not = necessarily a bug or even needing change, but that needs to be = considered.  What about = TODO?



Index: = src/fringe.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS = file: /sources/emacs/emacs/src/fringe.c,v
retrieving revision = 1.52
diff -a -u -r1.52 fringe.c
--- src/fringe.c 14 May = 2008 07:49:32 -0000 1.52
+++ src/fringe.c 15 Jul = 2008 16:59:29 -0000
@@ -482,4 +482,4 @@
static Lisp_Object = *fringe_faces;
static int max_fringe_bitmaps;

-static int = max_used_fringe_bitmap =3D MAX_STANDARD_FRINGE_BITMAPS;
+int = max_used_fringe_bitmap =3D MAX_STANDARD_FRINGE_BITMAPS;


Why? = This is not in the ChangeLog?  Undo please if not = needed.

Used for memory allocation = handling in nsterm.m (ns_draw_fringe_bitmap(), from line 2163).  It = is in the ChangeLog:

* = fringe.c (max_used_fringe_bitmap): Make = public.



Index: = src/getloadavg.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3DRCS file: /sources/emacs/emacs/src/getloadavg.c,v
retrieving = revision 1.53
diff -a -u -r1.53 getloadavg.c
--- = src/getloadavg.c = 8 Jan 2008 20:44:33 -0000 1.53
+++ src/getloadavg.c 15 Jul = 2008 16:59:32 -0000

This file comes from gnulib, we try not to = change it here.  It should go there first.
Why wasn't this = needed until now?  This should not have anything to do with = NS...

I don't know, probably it = was never working.  I actually am not sure if it is now, but anyway = the original is using #ifdef NeXT to check whether mach.h is in a = subdirectory, but it seems all NeXT-derived systems put it in the same = place (e.g., my OS X 10.5.4 machine = here).

ACTUALLY, it looks like this file is not = even used anymore (grep thru Makefile.in), so probably it should just be = removed.



Index: = src/keyboard.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
R= CS file: /sources/emacs/emacs/src/keyboard.c,v
retrieving revision = 1.960
diff -a -u -r1.960 keyboard.c
--- src/keyboard.c 26 Jun = 2008 04:24:36 -0000 1.960
+++ src/keyboard.c 15 Jul = 2008 17:00:59 -0000
@@ -7305,3 +7312,7 @@
void
= handle_async_input ()
{
+#ifdef BSD4_1
+  extern int = select_alarmed;
+#endif
+

Please remove this or use HAVE_NS = instead, BSD4_1 is not defined in
the emacs tree anymore.  This = probably causes big problems with the
input if this code is = needed.

@@ -7317,6 +7328,9 @@
=       if (nread <=3D 0)
= break;

+#ifdef BSD4_1
+ =      select_alarmed =3D 1;  /* Force the = select emulator back to life */
+#endif

Same here.

@@ = -7335,3 +7349,6 @@
  signal (signo, = input_available_signal);
#endif /* USG */

+#ifdef BSD4_1
+ =  sigisheld (SIGIO);
+#endif

Same here.

@@ -7348,3 = +7366,6 @@
  handle_async_input ();
= #endif

+#ifdef BSD4_1
+  sigfree = ();
+#endif

And = here.

These all must be spurious = merge conflicts.  Removed.  Sorry about = this.



@@ -8044,7 +8070,12 @@
=      && SYMBOLP (XSYMBOL = (def)->function)
=      && ! NILP (Fget (def, = Qmenu_alias)))
   def =3D = XSYMBOL (def)->function;
+#ifdef HAVE_NS
+ =          /* prefer 'super' = bindings */
+ =  tem =3D Fwhere_is_internal (def, Qnil, Qsuper, Qt, = Qt);
+#else
 tem =3D Fwhere_is_internal = (def, Qnil, Qt, Qnil, Qt);
+#endif


Please run this by = Stefan, not sure if we want to have a platform do
something different = here.

Yes, I was working to make = this only happen when ns-extended-platform-support-mode is on, but it's = a bit tricky since this only gets called = once.



Index: = src/keymap.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS = file: /sources/emacs/emacs/src/keymap.c,v
retrieving revision = 1.374
diff -a -u -r1.374 keymap.c
--- src/keymap.c 5 Jun = 2008 05:44:11 -0000 1.374
+++ src/keymap.c 15 Jul = 2008 17:01:22 -0000
@@ -111,3 +111,6 @@

extern Lisp_Object = Voverriding_local_map;

+#ifdef HAVE_NS
+extern Lisp_Object = Qalt, Qcontrol, Qhyper, Qmeta, Qsuper;
+#endif

Please get the = changes in this file approved by Stefan, they look a bit = suspicious.

OK, being worked = on.



Index: = src/lisp.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS = file: /sources/emacs/emacs/src/lisp.h,v
retrieving revision = 1.631
diff -a -u -r1.631 lisp.h
--- src/lisp.h 11 Jul = 2008 14:20:06 -0000 1.631
+++ src/lisp.h 15 Jul = 2008 17:01:36 -0000
@@ -28,3 +28,7 @@
#define P_(proto) ()
= #endif

+#ifdef NS_IMPL_GNUSTEP
+/* This conflicts with = functions in the GNUstep libraries. */
+#define hash_remove = emacs_hash_remove
+#endif  /* NS_IMPL_GNUSTEP */

Sounds = odd, but if this is indeed true, better rename the function in
emacs = to avoid extra #ifdefs.

Yes, = hash_put, hash_remove, etc. are all used in those libs.  Only = hash_remove is declared so publicly within emacs = though.



Index: = src/terminfo.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
R= CS file: /sources/emacs/emacs/src/terminfo.c,v
retrieving revision = 1.24
diff -a -u -r1.24 terminfo.c
--- src/terminfo.c 14 May = 2008 07:49:52 -0000 1.24
+++ src/terminfo.c 15 Jul = 2008 17:03:07 -0000
@@ -24,4 +24,7 @@
   so that = we do not need to conditionalize the places in Emacs
=    that set them.  */

+/* Causes a conflict = on OS X 10.3 .*/
+#ifndef NS_IMPL_COCOA
char *UP, *BC, = PC;
+#endif

Does "emacs -nw" work after doing this?  How = come this wasn't a problem with the Carbon = port?

Yes, and I don't know. =  It's possible something is different in the includes brought in by = Carbon vs. = Cocoa.



Index: = src/xdisp.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS = file: /sources/emacs/emacs/src/xdisp.c,v
retrieving revision = 1.1233
diff -a -u -r1.1233 xdisp.c
--- src/xdisp.c 15 Jul = 2008 15:45:05 -0000 1.1233
+++ src/xdisp.c 15 Jul = 2008 17:05:45 -0000
@@ -22539,7 +22576,10 @@
/* Switch the = display of W's cursor on or off, according to the value
=    of ON.  */

-static void
+#ifndef = HAVE_NS
+static
+#endif
+void
update_window_cursor (w, = on)
     struct window *w;
=      int on;

Why? It doesn't seem to be = used outside this file.

nsterm.m = uses it for cursor blink.  Yes, I know, it should use the generic = mechanism for this, and that is a longstanding TODO.  Help with it = is welcome.  Once that's done this can be = removed.


thanks for the = comments,
Adrian

= --Apple-Mail-70--419950230--