From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Dan Nicolaescu Newsgroups: gmane.emacs.devel Subject: a review of the merge (Re: Emacs.app merged) Date: Wed, 16 Jul 2008 02:25:56 -0700 Message-ID: <200807160925.m6G9PuVj012462@sallyv1.ics.uci.edu> References: <1C66F1FC-BF82-4365-944D-ADCC4D1F435C@gmail.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1216200418 23815 80.91.229.12 (16 Jul 2008 09:26:58 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 16 Jul 2008 09:26:58 +0000 (UTC) Cc: emacs- devel To: Adrian Robert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Jul 16 11:27:43 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 1KJ3IN-00047p-S1 for ged-emacs-devel@m.gmane.org; Wed, 16 Jul 2008 11:27:36 +0200 Original-Received: from localhost ([127.0.0.1]:37254 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KJ3HV-00061m-6Z for ged-emacs-devel@m.gmane.org; Wed, 16 Jul 2008 05:26:41 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KJ3HM-0005yy-3U for emacs-devel@gnu.org; Wed, 16 Jul 2008 05:26:32 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KJ3HK-0005wC-E2 for emacs-devel@gnu.org; Wed, 16 Jul 2008 05:26:31 -0400 Original-Received: from [199.232.76.173] (port=41976 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KJ3HK-0005vi-58 for emacs-devel@gnu.org; Wed, 16 Jul 2008 05:26:30 -0400 Original-Received: from sallyv1.ics.uci.edu ([128.195.1.109]:41483) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.60) (envelope-from ) id 1KJ3HK-00066D-4N for emacs-devel@gnu.org; Wed, 16 Jul 2008 05:26:30 -0400 X-ICS-MailScanner-Watermark: 1216805159.18971@SBObyNG14+Fe5WWJSu8a3A Original-Received: from mothra.ics.uci.edu (mothra.ics.uci.edu [128.195.6.93]) by sallyv1.ics.uci.edu (8.13.7+Sun/8.13.7) with ESMTP id m6G9PuVj012462; Wed, 16 Jul 2008 02:25:57 -0700 (PDT) In-Reply-To: <1C66F1FC-BF82-4365-944D-ADCC4D1F435C@gmail.com> (Adrian Robert's message of "Tue, 15 Jul 2008 14:47:21 -0400") Original-Lines: 500 X-ICS-MailScanner: Found to be clean X-ICS-MailScanner-SpamCheck: not spam, SpamAssassin (score=-0.186, required 5, autolearn=disabled, ALL_TRUSTED -1.44, FM_MULTI_ODD2 1.10, TW_DX 0.08, TW_GT 0.08) X-ICS-MailScanner-From: dann@mothra.ics.uci.edu X-detected-kernel: by monty-python.gnu.org: Solaris 10 (beta) 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:100791 Archived-At: Adrian Robert writes: Congratulations for getting this done! > A patch containing the changes to existing files, as well as a listing > of the files added, is available at: > > 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 :-(. > 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. 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. +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. 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. +#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 ? 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. Index: lisp/frame.el =================================================================== RCS file: /sources/emacs/emacs/lisp/frame.el,v retrieving revision 1.272 diff -a -u -r1.272 frame.el --- lisp/frame.el 12 Jun 2008 03:56:16 -0000 1.272 +++ lisp/frame.el 15 Jul 2008 16:52:39 -0000 @@ -610,9 +610,16 @@ "Make a frame on X display DISPLAY. The optional second argument PARAMETERS specifies additional frame parameters." (interactive "sMake frame on display: ") - (or (string-match "\\`[^:]*:[0-9]+\\(\\.[0-9]+\\)?\\'" display) - (error "Invalid display, not HOST:SERVER or HOST:SERVER.SCREEN")) - (when (and (boundp 'x-initialized) (not x-initialized)) - (setq x-display-name display) - (x-initialize-window-system)) - (make-frame `((window-system . x) (display . ,display) . ,parameters))) + (if (featurep 'ns-windowing) + (progn + (when (and (boundp 'ns-initialized) (not ns-initialized)) + (setq ns-display-name display) + (ns-initialize-window-system)) + (make-frame `((window-system . ns) (display . ,display) . ,parameters))) + (progn + (or (string-match "\\`[^:]*:[0-9]+\\(\\.[0-9]+\\)?\\'" display) + (error "Invalid display, not HOST:SERVER or HOST:SERVER.SCREEN")) + (when (and (boundp 'x-initialized) (not x-initialized)) + (setq x-display-name display) + (x-initialize-window-system)) + (make-frame `((window-system . x) (display . ,display) . ,parameters))))) Is this necessary? Can NS make frames on another display? If not, then this should not be needed. 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. + (load "emacs-lisp/easy-mmode") Same as above, this is for adding platform specific behavior. 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? Index: src/Makefile.in =================================================================== RCS file: /sources/emacs/emacs/src/Makefile.in,v retrieving revision 1.408 diff -a -u -r1.408 Makefile.in --- src/Makefile.in 11 Jul 2008 11:20:21 -0000 1.408 +++ src/Makefile.in 15 Jul 2008 16:56:56 -0000 @@ -112,3 +112,5 @@ #endif #endif +/* Under GNUstep, putting libc on the link line causes problems. */ +#ifdef GNUSTEP Shouldn't this be NS_IMPL_GNUSTEP ? +#ifdef GNUSTEP Same here. @@ -935,3 +967,4 @@ temacs${EXEEXT}: $(LOCALCPP) $(STARTFILES) stamp-oldxmenu ${obj} ${otherobj} OBJECTS_MACHINE prefix-args${EXEEXT} echo "${obj} ${otherobj} " OBJECTS_MACHINE > buildobj.lst +#ifdef GNUSTEP Same here. + +#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. Index: src/font.c =================================================================== RCS file: /sources/emacs/emacs/src/font.c,v retrieving revision 1.75 diff -a -u -r1.75 font.c --- src/font.c 9 Jul 2008 13:24:09 -0000 1.75 +++ src/font.c 15 Jul 2008 16:58:47 -0000 +#ifdef HAVE_NS +#define DEFAULT_ENCODING Qiso10646_1 +#else +#define DEFAULT_ENCODING Qiso8859_1 +#endif Please get his approved by Handa-san, there has been a lot of work on fonts lately. Index: src/frame.c =================================================================== RCS file: /sources/emacs/emacs/src/frame.c,v retrieving revision 1.380 diff -a -u -r1.380 frame.c --- src/frame.c 7 Jul 2008 20:39:00 -0000 1.380 +++ src/frame.c 15 Jul 2008 16:59:19 -0000 @@ -203,4 +206,4 @@ Value is t for a termcap frame (a character-only terminal), `x' for an Emacs frame that is really an X window, `w32' for an Emacs frame that is a window on MS-Windows display, -`mac' for an Emacs frame on a Macintosh display, +`mac' for an Emacs frame on a Macintosh 8/9 X-Carbon display, Please fix this, we don't support Macintosh 8/9 anymore. +`ns' for an Emacs frame on a GNUstep or Macintosh Cocoa display, `pc' for a direct-write MS-DOS frame. See also `frame-live-p'. */) (object) @@ -224,6 +228,8 @@ return Qpc; case output_mac: return Qmac; + case output_ns: + return Qns; default: abort (); } @@ -880,3 +891,8 @@ 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? +#ifndef HAVE_NS /* PENDING: ensure font attrs change goes through */ Better use "FIXME" instead of "PENDING". 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. 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... 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. @@ -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. Index: src/keyboard.h =================================================================== RCS file: /sources/emacs/emacs/src/keyboard.h,v retrieving revision 1.85 diff -a -u -r1.85 keyboard.h --- src/keyboard.h 8 Jun 2008 08:59:47 -0000 1.85 +++ src/keyboard.h 15 Jul 2008 17:01:00 -0000 @@ -314,8 +314,7 @@ confined to an extended version of this with sections of code below using it unconditionally. */ #ifndef HAVE_NTGUI -#ifdef USE_GTK -/* gtk just uses utf-8. */ +#if defined (USE_GTK) || defined (HAVE_NS) # define ENCODE_MENU_STRING(str) ENCODE_UTF_8 (str) #elif defined HAVE_X_I18N #define ENCODE_MENU_STRING(str) ENCODE_SYSTEM (str) @@ -325,3 +324,8 @@ #else /* HAVE_NTGUI */ #define ENCODE_MENU_STRING(str) (str) #endif + +#if defined (HAVE_NS) || defined (HAVE_NTGUI) + +typedef void * XtPointer; +typedef unsigned char Boolean; This looks strange, please get it approved by one of the Windows maintainers as it affects that platform. 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. 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. 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? Index: src/w32gui.h =================================================================== RCS file: /sources/emacs/emacs/src/w32gui.h,v retrieving revision 1.34 diff -a -u -r1.34 w32gui.h --- src/w32gui.h 26 Jun 2008 10:48:28 -0000 1.34 +++ src/w32gui.h 15 Jul 2008 17:03:07 -0000 @@ -21,5 +21,5 @@ #define EMACS_W32GUI_H #include +#include "w32bdf.h" -/* Emulate widget_value from ../lwlib/lwlib.h, modified for Windows. */ This looks very suspicious, why touch the Windows code? 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.