Emacs.app merged

Emacs.app merged

[Adrian Robert]

Hello,

The Cocoa/GNUstep port (Emacs.app) has been checked into CVS trunk.  A  
tag "before-merge-emacs-app-to-trunk" was made just before.

"configure" may need to be regenerated -- I did not have the  
sufficiently recent version of autoconf here, but it seemed to work  
anyway.

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

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

There is (still) a project at sourceforge associated with this port:

http://emacs-app.sf.net

I'm undecided whether to continue this -- it may be helpful as a  
developer tool, but I don't want users confused about where bugs are  
to be reported, etc..  A final binary release will be made there when  
23.1 comes out.

Adrian

Re: Emacs.app merged

[İsmail Dönmez]

On Tue, Jul 15, 2008 at 9:47 PM, Adrian Robert
<adrian.b.robert@gmail.com> wrote:
> Hello,
>
> The Cocoa/GNUstep port (Emacs.app) has been checked into CVS trunk.  A tag
> "before-merge-emacs-app-to-trunk" was made just before.
>
> "configure" may need to be regenerated -- I did not have the sufficiently
> recent version of autoconf here, but it seemed to work anyway.
>
> 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
>
> The newly-added 'nextstep' directory contains additional information.
>  FOR-RELEASE in that directory contains a list of TODOs before release of
> 23.1.
>
> There is (still) a project at sourceforge associated with this port:
>
> http://emacs-app.sf.net
>
> I'm undecided whether to continue this -- it may be helpful as a developer
> tool, but I don't want users confused about where bugs are to be reported,
> etc..  A final binary release will be made there when 23.1 comes out.

Yay! Thanks to everyone involved.

Regards,
ismail

-- 
I do object-oriented programming - if the customer objects, I do more
programming.

Re: Emacs.app merged

[Chong Yidong]

Thanks for your work, Adrian.

Now, since this is done, is there any reason to keep the Carbon support
code around?

Re: Emacs.app merged

[Thomas Christensen]

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

> "configure" may need to be regenerated -- I did not have the
> sufficiently recent version of autoconf here, but it seemed to work
> anyway.

Me likes.

I cannot compile it though.  I get this:

Loading emacs-lisp/easymenu.el (source)...
Loading emacs-lisp/easy-mmode.el (source)...
(require cl) while preparing to dump
make[1]: *** [emacs] Error 255
make: *** [src] Error 2
*** Compilation failed. ***
Please examine the above output to determine what went wrong,
edit the configure options in this script (\'compile\') to fix it, and rerun.


		Thomas

Re: Emacs.app merged

[Cezar Halmagean]

On 2008-07-15 15:32:47 -0700, Thomas Christensen 
<thomasc@thomaschristensen.org> said:

> Adrian Robert <adrian.b.robert@gmail.com> writes:
> 
>> "configure" may need to be regenerated -- I did not have the
>> sufficiently recent version of autoconf here, but it seemed to work
>> anyway.
> 
> Me likes.
> 
> I cannot compile it though.  I get this:
> 
> Loading emacs-lisp/easymenu.el (source)...
> Loading emacs-lisp/easy-mmode.el (source)...
> (require cl) while preparing to dump
> make[1]: *** [emacs] Error 255
> make: *** [src] Error 2
> *** Compilation failed. ***
> Please examine the above output to determine what went wrong,
> edit the configure options in this script (\'compile\') to fix it, and rerun.
> 
> 
> 		Thomas

I get exactly the same thing (OS X 10.5.4)

Cezar

a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

Adrian Robert <adrian.b.robert@gmail.com> 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 <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.

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 <windows.h>
 
+#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.

Re: a review of the merge (Re: Emacs.app merged)

[Jason Rumney]

Dan Nicolaescu wrote:

> 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

> +#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.

I think it is OK, those definitions were formerly in w32gui.h when it
was needed on windows only.

> 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 <windows.h>
>  
> +#include "w32bdf.h"
>  
> -/* Emulate widget_value from ../lwlib/lwlib.h, modified for Windows.  */
> 
> This looks very suspicious, why touch the Windows code?

It looks like the merge encountered conflicts here, with the code that
was moved to keyboard.h (above) being in the vicinity of some code that
was removed on 26 June when I tidied up the font backend code. The
conflict was resolved in the wrong way, reintroducing the old code.
As long as that only happened here (and you seem to be checking the
patch thoroughly, so hopefully any further occurences will be caught),
it should be simple to fix this.

Re: a review of the merge (Re: Emacs.app merged)

[Adrian Robert]

On Jul 16, 2008, at 6:00 AM, Jason Rumney wrote:

>> Index: src/w32gui.h
>> ...
>> +#include "w32bdf.h"
>>
>> -/* Emulate widget_value from ../lwlib/lwlib.h, modified for  
>> Windows.  */
>>
>> This looks very suspicious, why touch the Windows code?
>
> It looks like the merge encountered conflicts here, with the code that
> was moved to keyboard.h (above) being in the vicinity of some code  
> that
> was removed on 26 June when I tidied up the font backend code. The
> conflict was resolved in the wrong way, reintroducing the old code.
> As long as that only happened here (and you seem to be checking the
> patch thoroughly, so hopefully any further occurences will be caught),
> it should be simple to fix this.

Sorry about this.  The way I did the merge was:

1) Merge from trunk in bzr on 7/14
2) Do a CVS checkout around the same time
3) Copy needed files from bzr tree to CVS tree on 7/15
4) Do a CVS update
5) Tag, and do a commit

I resolved all the conflicts reported to me in steps 1,4 (not many) --  
though I don't remember one in w32gui.

The diff I posted and/or the CVS tag "before-merge-emacs-app-to-trunk"  
can hopefully help confirm / deny existence of additional spurious  
changes, if any.

Re: a review of the merge (Re: Emacs.app merged)

[Stefan Monnier]

> Sorry about this.  The way I did the merge was:

> 1) Merge from trunk in bzr on 7/14
> 2) Do a CVS checkout around the same time
> 3) Copy needed files from bzr tree to CVS tree on 7/15

*Never* copy files like this.  Instead, extract a patch from one tree
and apply it to the other.  Otherwise you risk losing changes.
This is true of any work with branches and things like that: nothing
specific to Emacs at all.


        Stefan

Re: a review of the merge (Re: Emacs.app merged)

[Stefan Monnier]

Thank you Dan for double checking.
Adrian, please try and address those issues.

> +#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?

Actually, shouldn't this even check MAC_OSX instead?

> 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.

I believe GNUstep can.

> +(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.

How do other platforms do it?  Shoujld we have a lisp/ns-fns.el where we
can put those things?

> @@ -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.

I'm looking at it, indeed.

> 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.

I think the intention is OK, but it needs to be made generic.  This is
linked to the above where_is_internal call (and the current "menus are
slow" problem).


        Stefan

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

  > > +(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.
  > 
  > How do other platforms do it? 

I think they define x-handle-* functions in term/*-win.el

Not sure why Emacs.app chose not to define x-handle-* functions, but
call them ns-handle-*

We probably need some common file for these functions (and the humongous
x-colors list) to avoid all the duplication that is happening now.

  >  Shoujld we have a lisp/ns-fns.el where we can put those things?

Not sure if anything is needed, maybe just appending to
command-line-x-option-alist would work.

Re: a review of the merge (Re: Emacs.app merged)

[Adrian Robert]

On Jul 16, 2008, at 5:23 PM, Dan Nicolaescu wrote:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
> We probably need some common file for these functions (and the  
> humongous
> x-colors list) to avoid all the duplication that is happening now.

If the x-colors list were put in a common file, with RGB specs, then  
each non-X GUI could share it at the cost of a few lines to iterate  
through the list --e.g.:

lisp var has a list of char *name, unsigned char r,g,b

macfns.c:
   colormap_t *mac_color_map = malloc(length-of-list);
   foreach-list-element mac_color_map[i] = { RGB_TO_ULONG(r,g,b),  
name };

w32fns.c:
colormap_t *w32_color_map = malloc(length-of-list);
foreach-list-element w32_color_map[i] = { name, PALETTERGB(r,g,b) };

nsterm.m:
NSColorList *cl = [[NSColorList alloc] init];
foreach-list-element [cl setColor:
         [NSColor colorWithCalibratedRed:r green: g blue:b alpha: 1.0]
     forKey: [NSString stringWithUTF8String: name]];

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

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

  > On Jul 16, 2008, at 5:23 PM, Dan Nicolaescu wrote:
  > 
  > > Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
  > >
  > > We probably need some common file for these functions (and the
  > > humongous
  > > x-colors list) to avoid all the duplication that is happening now.
  > 
  > If the x-colors list were put in a common file, with RGB specs, then
  > each non-X GUI could share it at the cost of a few lines to iterate
  > through the list --e.g.:
  > 
  > lisp var has a list of char *name, unsigned char r,g,b
  > 
  > macfns.c:
  >   colormap_t *mac_color_map = malloc(length-of-list);
  >   foreach-list-element mac_color_map[i] = { RGB_TO_ULONG(r,g,b), name
  > };
  > 
  > w32fns.c:
  > colormap_t *w32_color_map = malloc(length-of-list);
  > foreach-list-element w32_color_map[i] = { name, PALETTERGB(r,g,b) };
  > 
  > nsterm.m:
  > NSColorList *cl = [[NSColorList alloc] init];
  > foreach-list-element [cl setColor:
  >         [NSColor colorWithCalibratedRed:r green: g blue:b alpha: 1.0]
  >     forKey: [NSString stringWithUTF8String: name]];

Let's go one step at a time: please make the nsterm.m code use something
like this.  After having some working code it would be easy to move the
big color array definition into some sort of a common file.

Re: a review of the merge (Re: Emacs.app merged)

[Adrian Robert]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]


On Jul 20, 2008, at 7:56 AM, Dan Nicolaescu wrote:

> Adrian Robert <adrian.b.robert@gmail.com> writes:
>
>> On Jul 16, 2008, at 5:23 PM, Dan Nicolaescu wrote:
>>
>>> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>>>
>>> We probably need some common file for these functions (and the
>>> humongous
>>> x-colors list) to avoid all the duplication that is happening now.
>>
>> If the x-colors list were put in a common file, with RGB specs, then
>> each non-X GUI could share it at the cost of a few lines to iterate
>> through the list --e.g.:
>>
>> lisp var has a list of char *name, unsigned char r,g,b
>>
>> macfns.c:
>>  colormap_t *mac_color_map = malloc(length-of-list);
>>  foreach-list-element mac_color_map[i] = { RGB_TO_ULONG(r,g,b), name
>> };
>>
>> w32fns.c:
>> colormap_t *w32_color_map = malloc(length-of-list);
>> foreach-list-element w32_color_map[i] = { name, PALETTERGB(r,g,b) };
>>
>> nsterm.m:
>> NSColorList *cl = [[NSColorList alloc] init];
>> foreach-list-element [cl setColor:
>>        [NSColor colorWithCalibratedRed:r green: g blue:b alpha: 1.0]
>>    forKey: [NSString stringWithUTF8String: name]];
>
> Let's go one step at a time: please make the nsterm.m code use  
> something
> like this.  After having some working code it would be easy to move  
> the
> big color array definition into some sort of a common file.

OK.  Here is a patch that moves w32_load_color_file from w32fns.c to  
x_load_color_file in xfaces.c, and uses it in nsterm.m.  xfaces made  
the most sense because there are already cross-platform color handling  
functions in there.


[-- Attachment #2: rgbTxt.patch --]
[-- Type: application/octet-stream, Size: 6504 bytes --]

Index: w32fns.c
===================================================================
RCS file: /sources/emacs/emacs/src/w32fns.c,v
retrieving revision 1.343
diff -u -r1.343 w32fns.c
--- w32fns.c	15 Jul 2008 15:45:05 -0000	1.343
+++ w32fns.c	28 Jul 2008 13:20:30 -0000
@@ -502,53 +502,6 @@
   return (oldrgb);
 }
 
-DEFUN ("w32-load-color-file", Fw32_load_color_file,
-       Sw32_load_color_file, 1, 1, 0,
-       doc: /* Create an alist of color entries from an external file.
-Assign this value to `w32-color-map' to replace the existing color map.
-
-The file should define one named RGB color per line like so:
-  R G B   name
-where R,G,B are numbers between 0 and 255 and name is an arbitrary string.  */)
-    (filename)
-    Lisp_Object filename;
-{
-  FILE *fp;
-  Lisp_Object cmap = Qnil;
-  Lisp_Object abspath;
-
-  CHECK_STRING (filename);
-  abspath = Fexpand_file_name (filename, Qnil);
-
-  fp = fopen (SDATA (filename), "rt");
-  if (fp)
-    {
-      char buf[512];
-      int red, green, blue;
-      int num;
-
-      BLOCK_INPUT;
-
-      while (fgets (buf, sizeof (buf), fp) != NULL) {
-	if (sscanf (buf, "%u %u %u %n", &red, &green, &blue, &num) == 3)
-	  {
-	    char *name = buf + num;
-	    num = strlen (name) - 1;
-	    if (name[num] == '\n')
-	      name[num] = 0;
-	    cmap = Fcons (Fcons (build_string (name),
-				 make_number (RGB (red, green, blue))),
-			  cmap);
-	  }
-      }
-      fclose (fp);
-
-      UNBLOCK_INPUT;
-    }
-
-  return cmap;
-}
-
 /* The default colors for the w32 color map */
 typedef struct colormap_t
 {
@@ -7236,7 +7189,6 @@
 
   defsubr (&Sw32_define_rgb_color);
   defsubr (&Sw32_default_color_map);
-  defsubr (&Sw32_load_color_file);
   defsubr (&Sw32_send_sys_command);
   defsubr (&Sw32_shell_execute);
   defsubr (&Sw32_register_hot_key);
Index: xfaces.c
===================================================================
RCS file: /sources/emacs/emacs/src/xfaces.c,v
retrieving revision 1.407
diff -u -r1.407 xfaces.c
--- xfaces.c	27 Jul 2008 18:24:48 -0000	1.407
+++ xfaces.c	28 Jul 2008 13:20:42 -0000
@@ -6574,6 +6574,54 @@
 }
 
 \f
+
+DEFUN ("x-load-color-file", Fx_load_color_file,
+       Sx_load_color_file, 1, 1, 0,
+       doc: /* Create an alist of color entries from an external file.
+
+The file should define one named RGB color per line like so:
+  R G B   name
+where R,G,B are numbers between 0 and 255 and name is an arbitrary string.  */)
+    (filename)
+    Lisp_Object filename;
+{
+  FILE *fp;
+  Lisp_Object cmap = Qnil;
+  Lisp_Object abspath;
+
+  CHECK_STRING (filename);
+  abspath = Fexpand_file_name (filename, Qnil);
+
+  fp = fopen (SDATA (filename), "rt");
+  if (fp)
+    {
+      char buf[512];
+      int red, green, blue;
+      int num;
+
+      BLOCK_INPUT;
+
+      while (fgets (buf, sizeof (buf), fp) != NULL) {
+	if (sscanf (buf, "%u %u %u %n", &red, &green, &blue, &num) == 3)
+	  {
+	    char *name = buf + num;
+	    num = strlen (name) - 1;
+	    if (name[num] == '\n')
+	      name[num] = 0;
+	    cmap = Fcons (Fcons (build_string (name),
+                                make_number ((red << 16) | (green << 8) | blue)),
+			  cmap);
+	  }
+      }
+      fclose (fp);
+
+      UNBLOCK_INPUT;
+    }
+
+  return cmap;
+}
+
+\f
 /***********************************************************************
 				Tests
  ***********************************************************************/
@@ -6829,6 +6877,7 @@
 #endif
   defsubr (&Scolor_gray_p);
   defsubr (&Scolor_supported_p);
+  defsubr (&Sx_load_color_file);
   defsubr (&Sface_attribute_relative_p);
   defsubr (&Smerge_face_attribute);
   defsubr (&Sinternal_get_lisp_face_attribute);
Index: nsterm.m
===================================================================
RCS file: /sources/emacs/emacs/src/nsterm.m,v
retrieving revision 1.17
diff -u -r1.17 nsterm.m
@@ -3822,37 +3822,37 @@
     ns_selection_color = NS_SELECTION_COLOR_DEFAULT;
 
   {
-    id cl;
-    Lisp_Object tem, tem1;
-    extern Lisp_Object Vsource_directory;
-
-    cl = [NSColorList colorListNamed: @"Emacs"];
+    NSColorList *cl = [NSColorList colorListNamed: @"Emacs"];
 
     if ( cl == nil )
       {
-        /* first try data_dir, then invocation-dir
-           and finally source-directory/etc */
-        tem1 = tem
-	  = Fexpand_file_name (build_string ("Emacs.clr"), Vdata_directory);
-        if (NILP (Ffile_exists_p (tem)))
+        Lisp_Object color_file, color_map, color;
+        int r,g,b;
+        unsigned long c;
+        char *name;
+
+        color_file = Fexpand_file_name (build_string ("rgb.txt"),
+                         Fsymbol_value (intern ("data-directory")));
+        if (NILP (Ffile_readable_p (color_file)))
+          fatal ("Could not find %s.\n", SDATA (color_file));
+
+        color_map = Fx_load_color_file (color_file);
+        if (NILP (color_map))
+          fatal ("Could not read %s.\n", SDATA (color_file));
+
+        cl = [[NSColorList alloc] initWithName: @"Emacs"];
+        for ( ; CONSP (color_map); color_map = XCDR (color_map))
           {
-            tem = Fexpand_file_name (build_string ("Emacs.clr"),
-                                     Vinvocation_directory);
-            if (NILP (Ffile_exists_p (tem)))
-              {
-                Lisp_Object newdir
-		  = Fexpand_file_name (build_string ("etc/"),
-				       Vsource_directory);
-                tem = Fexpand_file_name (build_string ("Emacs.clr"),
-                                         newdir);
-              }
+            color = XCAR (color_map);
+            name = SDATA (XCAR (color));
+            c = XINT (XCDR (color));
+            [cl setColor:
+                  [NSColor colorWithCalibratedRed: RED_FROM_ULONG (c) / 255.0
+                                            green: GREEN_FROM_ULONG (c) / 255.0
+                                             blue: BLUE_FROM_ULONG (c) / 255.0
+                                            alpha: 1.0]
+                  forKey: [NSString stringWithUTF8String: name]];
           }
-
-        cl = [[NSColorList alloc]
-               initWithName: @"Emacs"
-                   fromFile: [NSString stringWithCString: SDATA (tem)]];
-        if (cl ==nil)
-          fatal ("Could not find %s.\n", SDATA (tem1));
         [cl writeToFile: nil];
       }
   }

[-- Attachment #3: Type: text/plain, Size: 3 bytes --]

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

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

  > On Jul 20, 2008, at 7:56 AM, Dan Nicolaescu wrote:
  > 
  > > Adrian Robert <adrian.b.robert@gmail.com> writes:
  > >
  > >> On Jul 16, 2008, at 5:23 PM, Dan Nicolaescu wrote:
  > >>
  > >>> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
  > >>>
  > >>> We probably need some common file for these functions (and the
  > >>> humongous
  > >>> x-colors list) to avoid all the duplication that is happening now.
  > >>
  > >> If the x-colors list were put in a common file, with RGB specs, then
  > >> each non-X GUI could share it at the cost of a few lines to iterate
  > >> through the list --e.g.:
  > >>
  > >> lisp var has a list of char *name, unsigned char r,g,b
  > >>
  > >> macfns.c:
  > >>  colormap_t *mac_color_map = malloc(length-of-list);
  > >>  foreach-list-element mac_color_map[i] = { RGB_TO_ULONG(r,g,b), name
  > >> };
  > >>
  > >> w32fns.c:
  > >> colormap_t *w32_color_map = malloc(length-of-list);
  > >> foreach-list-element w32_color_map[i] = { name, PALETTERGB(r,g,b) };
  > >>
  > >> nsterm.m:
  > >> NSColorList *cl = [[NSColorList alloc] init];
  > >> foreach-list-element [cl setColor:
  > >>        [NSColor colorWithCalibratedRed:r green: g blue:b alpha: 1.0]
  > >>    forKey: [NSString stringWithUTF8String: name]];
  > >
  > > Let's go one step at a time: please make the nsterm.m code use
  > > something
  > > like this.  After having some working code it would be easy to move
  > > the
  > > big color array definition into some sort of a common file.
  > 
  > OK.  Here is a patch that moves w32_load_color_file from w32fns.c to
  > x_load_color_file in xfaces.c, and uses it in nsterm.m.  xfaces made
  > the most sense because there are already cross-platform color handling
  > functions in there.

The X11 code does not need this, so it should at least be properly
protected by #ifdefs, and maybe have a name.  w32-load-color-file does
not seem to be used outside w32fns.c, so maybe it does not even need to
get exported.  All these should probably be discussed with the w32
maintainers. 

Re: a review of the merge (Re: Emacs.app merged)

[Adrian Robert]

On Jul 28, 2008, at 3:00 PM, Dan Nicolaescu wrote:

> Adrian Robert <adrian.b.robert@gmail.com> writes:
>
>> On Jul 20, 2008, at 7:56 AM, Dan Nicolaescu wrote:
>>
>>> Adrian Robert <adrian.b.robert@gmail.com> writes:
>>>
>>>> On Jul 16, 2008, at 5:23 PM, Dan Nicolaescu wrote:
>>>>
>>>>> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>>>>>
>>>>> We probably need some common file for these functions (and the
>>>>> humongous
>>>>> x-colors list) to avoid all the duplication that is happening now.
>>>>
>>>> If the x-colors list were put in a common file, with RGB specs,  
>>>> then
>>>> each non-X GUI could share it at the cost of a few lines to iterate
>>>> through the list --e.g.:
>>>>
>>>> lisp var has a list of char *name, unsigned char r,g,b
>>>>
>>>> macfns.c:
>>>> colormap_t *mac_color_map = malloc(length-of-list);
>>>> foreach-list-element mac_color_map[i] = { RGB_TO_ULONG(r,g,b), name
>>>> };
>>>>
>>>> w32fns.c:
>>>> colormap_t *w32_color_map = malloc(length-of-list);
>>>> foreach-list-element w32_color_map[i] = { name,  
>>>> PALETTERGB(r,g,b) };
>>>>
>>>> nsterm.m:
>>>> NSColorList *cl = [[NSColorList alloc] init];
>>>> foreach-list-element [cl setColor:
>>>>       [NSColor colorWithCalibratedRed:r green: g blue:b alpha: 1.0]
>>>>   forKey: [NSString stringWithUTF8String: name]];
>>>
>>> Let's go one step at a time: please make the nsterm.m code use
>>> something
>>> like this.  After having some working code it would be easy to move
>>> the
>>> big color array definition into some sort of a common file.
>>
>> OK.  Here is a patch that moves w32_load_color_file from w32fns.c to
>> x_load_color_file in xfaces.c, and uses it in nsterm.m.  xfaces made
>> the most sense because there are already cross-platform color  
>> handling
>> functions in there.
>
> The X11 code does not need this, so it should at least be properly
> protected by #ifdefs, and maybe have a name.  w32-load-color-file does
> not seem to be used outside w32fns.c, so maybe it does not even need  
> to
> get exported.  All these should probably be discussed with the w32
> maintainers.

Hi Jason,

Do you have any objection to moving/renaming w32-load-color-file to  
xfaces.c so the NS port can use it?  Also, does it need to be exposed  
to lisp (syms_of...)?  It's not called from anywhere in the emacs tree  
lisp anyway.

Finally, how necessary is the hard-coded set of xcolors definitions in  
w32fns.c?  It's not OK just to abort if this file is not found in data- 
directory?

Re: a review of the merge (Re: Emacs.app merged)

[Jason Rumney]

Adrian Robert wrote:

> Hi Jason,
> 
> Do you have any objection to moving/renaming w32-load-color-file to
> xfaces.c so the NS port can use it?  Also, does it need to be exposed to
> lisp (syms_of...)?  It's not called from anywhere in the emacs tree lisp
> anyway.

No objection to renaming, I guess the export to lisp is so users can
override the color names with their own rgb.txt file, though I suspect
the number of users who do so is pretty close to zero.

> Finally, how necessary is the hard-coded set of xcolors definitions in
> w32fns.c?  It's not OK just to abort if this file is not found in
> data-directory?

I think Emacs should not abort (on any platform) if it does not find
data files. Emacs 22 starts up on Windows if given -nw without ANY other
files, without -nw it requires lisp/term/w32-win.el, but in the trunk
that is dumped, so theoretically it should be possible to do basic
editing with just an executable (currently it is producing an "Invalid
code(s)" error).

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

Jason Rumney <jasonr@gnu.org> writes:

  > Adrian Robert wrote:
  > 
  > > Hi Jason,
  > > 
  > > Do you have any objection to moving/renaming w32-load-color-file to
  > > xfaces.c so the NS port can use it?  Also, does it need to be exposed to
  > > lisp (syms_of...)?  It's not called from anywhere in the emacs tree lisp
  > > anyway.
  > 
  > No objection to renaming, I guess the export to lisp is so users can
  > override the color names with their own rgb.txt file, though I suspect
  > the number of users who do so is pretty close to zero.

If the number is close to zero, then should we even consider giving such
an option?  We have too many knobs in emacs anyway, why add one that we
know won't be used?  And IMHO overriding color names is not something we
want to allow...

Re: a review of the merge (Re: Emacs.app merged)

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@ics.uci.edu>
> Date: Fri, 01 Aug 2008 05:55:54 -0700
> Cc: Adrian Robert <adrian.b.robert@gmail.com>,
> 	emacs- devel <emacs-devel@gnu.org>
> 
> IMHO overriding color names is not something we want to allow...

Why not?  Emacs uses the RGB values, not the names of the colors.

Re: a review of the merge (Re: Emacs.app merged)

[Jason Rumney]

Dan Nicolaescu wrote:

> If the number is close to zero, then should we even consider giving such
> an option?  We have too many knobs in emacs anyway, why add one that we
> know won't be used?

This isn't a new option. That function has been in w32fns.c since at
least 1996.

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

Jason Rumney <jasonr@gnu.org> writes:

  > Dan Nicolaescu wrote:
  > 
  > > If the number is close to zero, then should we even consider giving such
  > > an option?  We have too many knobs in emacs anyway, why add one that we
  > > know won't be used?
  > 
  > This isn't a new option. That function has been in w32fns.c since at
  > least 1996.

It would be new for NS.
And it had a different name in w32...

Now IMHO: I'd say that the w32 function can be removed because it is not
very useful, and the data in etc/rgb.txt be put in a static struct in a
C file and use it from there.

Re: a review of the merge (Re: Emacs.app merged)

[Adrian Robert]

On Aug 1, 2008, at 10:23 AM, Dan Nicolaescu wrote:

> Now IMHO: I'd say that the w32 function can be removed because it is  
> not
> very useful, and the data in etc/rgb.txt be put in a static struct  
> in a
> C file and use it from there.

Having it in rgb.txt makes it easy to upgrade when X changes this file  
(which it does from time to time).  What about best of both worlds,  
some sort of compile-time awk operation that generates a C file from  
etc/rgb.txt?

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

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

  > On Aug 1, 2008, at 10:23 AM, Dan Nicolaescu wrote:
  > 
  > > Now IMHO: I'd say that the w32 function can be removed because it is
  > > not
  > > very useful, and the data in etc/rgb.txt be put in a static struct
  > > in a
  > > C file and use it from there.
  > 
  > Having it in rgb.txt makes it easy to upgrade when X changes this file
  > (which it does from time to time).  

It does?  Is it often enough to worry about it?

  > What about best of both worlds, some sort of compile-time awk
  > operation that generates a C file from etc/rgb.txt?

It would be better if the C file was checked in the repository (like we
do for example with the "configure" script).
And it would be even better if we didn't have the etc/rgb.txt file at
all, (it get needlessly installed on GNU/Linux for example), just the
script that processes the standard X11 rgb.txt and produces the C file.

Re: a review of the merge (Re: Emacs.app merged)

[Adrian Robert]

[-- Attachment #1: Type: text/plain, Size: 15239 bytes --]


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
> ===================================================================
> 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


[-- Attachment #2: Type: text/html, Size: 23193 bytes --]

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

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

  > 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.

With a good editor :-), clutter is not an issue.

  >     +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.

IMHO that would be better, there's no need to have extra code for a
parser that way.

  >     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.  

AFAIK RMS was against doing things like this, even when not on by
default.
Even if it was to stay (and IMHO it should not), it would be better if
this code would be in a different place, not in ns-win.el, and autoload
in ns-win.el should be enough.

  > 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).

Keymap manipulation is not too bad, easymenu is more complex because it
needs to be cross platform..

  >     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?

NS should be following whatever X, w32 and mac are doing.  That makes
maintaining the code easier.

For example there are quite a few functions/variables that X, w32 and
mac call x-BLAH.  But ns calls them ns-BLAH.  Please rename them to
x-BLAH.

  >     +#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?

Dependencies do not hurt if they are in the generic code, better keep
the Makefile simpler (it is way too complex as it is)

  >     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.

IMHO this needs to be debugged and understood better.  When does it happen?

  > 
  >     +#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?

XXX, TODO and FIXME are the ones that are in use in the tree.  Anything
that has been in use for a while should be fine.

  >     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:

Sorry, power of habit: M-x rgrep ch used to be enough, now there are .m
files in the tree..


  >     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.

If you don't have a getloadavg.o in your tree, then it's safe to undo
these changes...

  >     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.

It looks like hash_remove is only used in fns.c, so better rename it
there, and it can even be made static (BTW nsgui.h also seems to have
this #define).

  >     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.

Is OS X 10.3 something we still want to support?
If yes, then maybe better rename those variables.

FOR-RELEASE [was Re: a review of the merge (Re: Emacs.app merged)]

[Glenn Morris]

Dan Nicolaescu wrote:

>>> 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.
>
> With a good editor :-), clutter is not an issue.

Or we could start using the bugtracker instead of FOR-RELEASE. Things
can be assigned to people or packages, discussion can happen, there's
a more permanent record, etc. Just a suggestion.

Re: a review of the merge (Re: Emacs.app merged)

[Glenn Morris]

Dan Nicolaescu wrote:

>   > ACTUALLY, it looks like this file is not even used anymore (grep thru
>   > Makefile.in), so probably it should just be removed.
>
> If you don't have a getloadavg.o in your tree, then it's safe to undo
> these changes...

AC_FUNC_GETLOADAVG in configure.in uses it. Perhaps that should go.

Re: a review of the merge (Re: Emacs.app merged)

[Adrian Robert]

On Jul 16, 2008, at 11:24 PM, Dan Nicolaescu wrote:

> Adrian Robert <adrian.b.robert@gmail.com> writes:
>
>> On Jul 16, 2008, at 5:25 AM, Dan Nicolaescu wrote:
>>
>>
>>    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?
>
> NS should be following whatever X, w32 and mac are doing.  That makes
> maintaining the code easier.
>
> For example there are quite a few functions/variables that X, w32 and
> mac call x-BLAH.  But ns calls them ns-BLAH.  Please rename them to
> x-BLAH.

Sorry if I was unclear, there are two points here.  First, NS has  
followed X, but mac and w32 do their own (different) thing for arg  
handling.  Things should be consistent, but don't know which ones  
should be changed.

The second point is about naming.  I feel that ports that used the  
"x-" prefix for their own functions did the wrong thing in general,  
and make things more confusing.  Now there is no way to know, from a  
function name, whether it is implemented generic window system code,  
non-X port-specific code, or X windows-specific code.  It also makes  
grepping and using automated tools like find-tag difficult to use with  
emacs source.

However using x- for everything makes other things easier (no need for  
aliasing, etc.), and that is the way things have gone.  But in this  
case, there would be a naming conflict, unless adding some 'if'  
structure to set command-line-x-option-alist, etc. differently  
depending on platform (which, is it known at this stage)?



>>    +#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?
>
> Dependencies do not hurt if they are in the generic code, better keep
> the Makefile simpler (it is way too complex as it is)

OK, so remove both the HAVE_NS and HAVE_CARBON ifdefs for their source  
dependencies?



>>    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.
>
> IMHO this needs to be debugged and understood better.  When does it  
> happen?

The case being hit was, when a frame was closed, no other frame would  
get focus.



>>    +#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?
>
> XXX, TODO and FIXME are the ones that are in use in the tree.   
> Anything
> that has been in use for a while should be fine.

OK, changed.



>>    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.
>
> If you don't have a getloadavg.o in your tree, then it's safe to undo
> these changes...

I don't, but neither does anybody else.  ;) How about just removing  
the .c file?



>>    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.
>
> It looks like hash_remove is only used in fns.c, so better rename it
> there, and it can even be made static (BTW nsgui.h also seems to have
> this #define).

Will do.  At one point it was used in >1 c file (else I would have  
done the static thing) but can't find ChangeLog when that changed.   
Maybe I was hallucinating again...  ;)



>>    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.
>
> Is OS X 10.3 something we still want to support?
> If yes, then maybe better rename those variables.

As far as Emacs.app, no reason not to support 10.3 for now, but  
eventually some code can be removed if the decision is to drop it.  In  
truth, I haven't tested it in a while.  I don't understand enough  
about the use of these variables and their relation to the same ones  
in ncurses includes to want to mess any further with these variables  
though.

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

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

  > On Jul 16, 2008, at 11:24 PM, Dan Nicolaescu wrote:
  > 
  > > Adrian Robert <adrian.b.robert@gmail.com> writes:
  > >
  > >> On Jul 16, 2008, at 5:25 AM, Dan Nicolaescu wrote:
  > >>
  > >>
  > >>    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?
  > >
  > > NS should be following whatever X, w32 and mac are doing.  That makes
  > > maintaining the code easier.
  > >
  > > For example there are quite a few functions/variables that X, w32 and
  > > mac call x-BLAH.  But ns calls them ns-BLAH.  Please rename them to
  > > x-BLAH.
  > 
  > Sorry if I was unclear, there are two points here.  First, NS has
  > followed X, but mac and w32 do their own (different) thing for arg
  > handling.  Things should be consistent, but don't know which ones
  > should be changed.
  > 
  > The second point is about naming.  I feel that ports that used the
  > "x-" prefix for their own functions did the wrong thing in general,
  > and make things more confusing.  

That decision was made probably more than 10 years ago, there's little
point to reopen this issue now without a good reason.  The fact that a
platform that does not follow existing conventions just makes the code
harder to maintain, and maintanability is one of the main concerns for
emacs.

Same argument goes for getting rid of etc/Emacs.clr.

  > >>    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.
  > >
  > > IMHO this needs to be debugged and understood better.  When does it
  > > happen?
  > 
  > The case being hit was, when a frame was closed, no other frame would
  > get focus.

As Stefan said, it would be better if this problem was understood, and
the first step would be to have a proper description. 

  > >>    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.
  > >
  > > If you don't have a getloadavg.o in your tree, then it's safe to undo
  > > these changes...
  > 
  > I don't, but neither does anybody else.  ;) How about just removing
  > the .c file?

I don't think that would be safe, it is still used by configure for
systems that do have a getloadvg function.  I don't know if we still
support such systems, but I'd welcome it being removed...

But this is a different discussion, the fact is the code in getloadvg
should not be modified in the emacs tree, and more, it should not be
modified if we don't know for sure it needs to be modified.

  > >>    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.
  > >
  > > Is OS X 10.3 something we still want to support?
  > > If yes, then maybe better rename those variables.
  > 
  > As far as Emacs.app, no reason not to support 10.3 for now, but
  > eventually some code can be removed if the decision is to drop it.  In
  > truth, I haven't tested it in a while.  I don't understand enough
  > about the use of these variables and their relation to the same ones
  > in ncurses includes to want to mess any further with these variables
  > though.

So basically we are adding code to support a system that we don't even
know that it even works, nor do we know for sure what the problem is.
The termcap/terminfo code is too complicated as it is, better not add
yet more complications like this.  So I'd say, better take this out, we
are still not very close to a release.  If problems occur by then, they
can be fixed with a proper understanding of what is going on and being
able to test the results.

Re: a review of the merge (Re: Emacs.app merged)

[Stefan Monnier]

>> 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.

Actually, I think it can be placed into admin/FOR-RELEASE.  Thanks to
the outline mode, anybody bothered by this Emacs.app section can fold it
away from view ;-)

> 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.

I have no preference, except that merging them into a single file sounds
like a good idea (to the extent possible, obviously: I haven't looked
at it at all).

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

Can't similar "uncompletable colors" be specified in X11 (with format
"#RRGGBB" or somesuch)?  Maybe your change should be applied to
more platforms?

In any case, we need a comment in that code to explain why
`require-match' is set or not, and when.

> 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).

Maybe a separate file would be a good choice.  Better yet: make it work
(as much as possible) for non-NS platforms as well, in case someone used
to those modifications wants to use them under w32, X11, ...

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

The dependencies seem harmless for other platforms, so you can just
remove the #ifdef.  Any removal of CPP tricks in src/Makefile.in is
good, since it gets us one step closer to the point where we can get rid
of the cpp processing step.

>> 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.

Please explain as precisely as you can what is the problem you're trying
to solve with the above code.

> 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?

I use FIXME for those as well.

> 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.

With the new where-is-preferred-modifier, this should now be easier.

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

Actually, I see hash_remove in lisp.h, but it seems not to be used
outside of fns.c so it should probably be made static.


        Stefan

Re: a review of the merge (Re: Emacs.app merged)

[David De La Harpe Golden]

Stefan Monnier wrote:

>> This is so users can enter colors in numeric format, such as ARGBD0FFFFFF.
>> The NS port interprets such formats to allow alpha  specification.
> 
> Can't similar "uncompletable colors" be specified in X11 (with format
> "#RRGGBB" or somesuch)? 

Yes, but  because XParseColor() itself supports #RRGGBB syntax (it's
actually "discouraged", the "recommended" syntax is
"rgb:RR/GG/BB", which also works in x11 emacs, because emacs just calls
XParseColor(), see XParseColor man page)

But while X11 IIUC now supports argb visuals*, XParseColor() does not
support alpha component specification.  This might just be an oversight
by X.org people, or it might be because alpha is just not allowed for in
the existing XColor struct (I'm unclear on whether that struct could
be safely extended by the xlib maintainers).

* so emacs on modern X11 should also be able to do useful (well, mostly
eye-candy) things with alpha values, far finer-grained than
just specifying the overall window transparency for a compositing
manager to pick up (as a recent patch did), so it would certainly be
useful to have support for alpha in emacs color specs for X11 too, but
there'd need to be quite a lot of changes to the rendering path for
maximum coolness (e.g. handwavily, face realisation or thereabouts doing
alpha-compositing rather than mere overriding of color properties so
that region highlighting could be a pretty tinted overlay rather than
just obliterating some parts of existing highlighting it's overlaying.)

> Maybe your change should be applied to
> more platforms?
>

Not sure I like "ARGB11223344" syntax in particular, never seen it
anywhere before, though I guess it doesn't matter much if it's
emacs-internal.  Might be worth asking someone X.org-developer-y what
the X11 syntax should be and whether XParseColor() could/should be
extended to support it.  I'd guess they'd favour  argb:AA/RR/GG/BB
(note that XParseColor already supports
#RRRRGGGGBBBB so #AAARRRGGGBBB is a nonrunner due to ambiguity)

Re: a review of the merge (Re: Emacs.app merged)

[Dan Nicolaescu]

Here are some comments on ns-win.el.  Some things are not hard to fix,
but not many people have access to this platform, so it's hard to change
things without being able to do some minimal testing... 

(defun ns-submit-bug-report ()

No need for this anymore, use the built in function.


(defun ns-handle-switch (switch &optional numeric)
(defun ns-handle-numeric-switch (switch)
(defun ns-handle-iconic (switch)
(defun ns-handle-name-switch (switch)
(defvar ns-display-name nil

Please rename these, replacing ns- with x-

;; nsterm.m.
(defvar ns-input-file)

(defun ns-ignore-0-arg (switch)
Remove it, just use `ignore' instead.

(defun ns-handle-args (args)
Rename to x-handle-args


;; Map certain keypad keys into ASCII characters
;; that people usually expect.
(define-key function-key-map [backspace] [127])
(define-key function-key-map [delete] [127])
[snip]

These should go in x-setup-function-keys, see what x-win.el does.


(load "ns-carbon-compat")
Just inline it here, if it's necessary.

;; alt-up/down scrolling a la Stuart.app
;; only activated if ns-extended-platform-support is on
(defun up-one () (interactive) (scroll-up 1))
(defun down-one () (interactive) (scroll-down 1))
(defun left-one () (interactive) (scroll-left 1))
(defun right-one () (interactive) (scroll-right 1))

I thought Yidong already said that it's better not to add these.


(define-minor-mode ns-extended-platform-support-mode
This was mentioned in another message.

(defun mouse-extend-region (event) 
Not sure if this belongs here, I'd ask Stefan/Yidong.  But if it
does, please add an ns- to the name.

(define-key global-map [menu-bar] (make-sparse-keymap "menu-bar"))
[snip]
The menu business was discussed in another message.

(defun info-ns-emacs ()
This is is not very useful, and is not even justified by being
similar to other programs on the platform.

(defun menu-bar-select-frame (&optional frame)
(defun menu-bar-update-frames ()
These have nothing to do with ns- 


(defun force-menu-bar-update-buffers ()
  ;; This is a hack to get around fact that we already checked
  ;; frame-or-buffer-changed-p and reset it, so menu-bar-update-buffers
  ;; does not pick up any change.
  (menu-bar-update-buffers t))

(add-hook 'menu-bar-update-fab-hook 'menu-bar-update-frames)
(add-hook 'menu-bar-update-fab-hook 'force-menu-bar-update-buffers)

(defun menu-bar-update-frames-and-buffers ()
  (if (frame-or-buffer-changed-p)
      (run-hooks 'menu-bar-update-fab-hook)))

(setq menu-bar-update-hook
      (delq 'menu-bar-update-buffers menu-bar-update-hook))
(add-hook 'menu-bar-update-hook 'menu-bar-update-frames-and-buffers)

Don't think any of those are right

(menu-bar-update-frames-and-buffers)
(precompute-menubar-bindings)
This is file is loaded when dumping emacs, no reason to do this here.


;; ns-arrange functions contributed
;; by Eberhard Mandler <mandler@dbag.ulm.DaimlerBenz.COM>
(defun ns-arrange-all-frames ()
(defun ns-arrange-visible-frames ()
(defun ns-arrange-frames ( vis)
These have nothing to do with ns-, they should be added to emacs
on their own merit, following the normal procedures.



(defun get-lisp-resource (arg1 arg2)
(defun ns-save-preferences ()
(fset 'menu-bar-options-save-orig (symbol-function 'menu-bar-options-save))
(defun ns-save-options ()

Not sure if these are acceptable here, please ask Stefan/Yidong.

(setq-default mode-line-frame-identification '("  "))

It's doubtful that this works correctly with both terminal and ns frames.

;; You say tomAYto, I say tomAHto..
(defvaralias 'ns-option-modifier 'ns-alternate-modifier)
Please remove.

(defun ns-do-hide-emacs ()
(defun ns-do-hide-others ()
(defun ns-next-frame ()
(defun ns-prev-frame ()

;; If no position specified, make new frame offset by 25 from current.
(add-hook 'before-make-frame-hook
          (lambda ()
            (let ((left (cdr (assq 'left (frame-parameters))))
                  (top (cdr (assq 'top (frame-parameters)))))
              (if (consp left) (setq left (cadr left)))
              (if (consp top) (setq top (cadr top)))
              (cond
               ((or (assq 'top parameters) (assq 'left parameters)))
               ((or (not left) (not top)))
               (t
                (setq parameters (cons (cons 'left (+ left 25))
                                       (cons (cons 'top (+ top 25))
                                             parameters))))))))

There's nothing ns- specific about these.



;; frame will be focused anyway, so select it
(add-hook 'after-make-frame-functions 'select-frame)

This does not seem right.  Other platforms don't need it.

(defun ns-toggle-toolbar (&optional frame)
Not ns- specific

;; Redefine from frame.el.
(define-minor-mode blink-cursor-mode
Please don't do this, change blink-cursor-mode if it's missing something.


(defun ns-yes-or-no-p (prompt)

This should not be needed, yes-or-no-p has enough knobs to
implement this behavior.


(defalias 'x-list-fonts 'ns-list-fonts)
Better rename ns-list-fonts to x-list-fonts, and remove this.

(setq scalable-fonts-allowed t)
Is this still needed?


(defun ns-respond-to-change-font ()

Is this needed?

;; Conditional on new-fontset so bootstrapping works on non-GUI compiles.
(if (fboundp 'new-fontset)
    (progn
      ;; Setup the default fontset.
      (setup-default-fontset)
      ;; Create the standard fontset.
      (create-fontset-from-fontset-spec ns-standard-fontset-spec t)))

Not sure if this is needed, you might want to get all this font
business reviewed by someone that knows how fonts are supposed
to work after the unicode-2 merge.

(defalias 'x-select-text 'ns-select-text)
(defalias 'x-cut-buffer-or-selection-value 'ns-pasteboard-value)
(defalias 'x-disown-selection-internal 'ns-disown-selection-internal)
(defalias 'x-get-selection-internal 'ns-get-selection-internal)
(defalias 'x-own-selection-internal 'ns-own-selection-internal)

Just rename the functions instead of using defalias.

(set-face-background 'region "ns_selection_color")
Better not mess up with face foregrounds here.



(defun ns-scroll-bar-move (event)
(defun ns-handle-scroll-bar-event (event)
Are these different from what other platforms do about scroll bars?
(No idea here)


(defvar colors x-colors
Is this used?

(defalias 'x-defined-colors 'ns-defined-colors)
(defalias 'xw-defined-colors 'ns-defined-colors)
(defalias 'x-display-mm-width 'ns-display-mm-width)
(defalias 'x-display-mm-height 'ns-display-mm-height)
(defalias 'x-display-backing-store 'ns-display-backing-store)
(defalias 'x-display-save-under 'ns-display-save-under)
(defalias 'x-display-visual-class 'ns-display-visual-class)
(defalias 'x-display-screens 'ns-display-screens)
(defalias 'x-focus-frame 'ns-focus-frame)

(setq frame-title-format t
      icon-title-format t)
Not sure if we want to change the defaults for these...  Better check with Stefan/Yidong.

(setq browse-url-browser-function 'browse-url-generic)
(setq browse-url-generic-program

browse-url seems to deal with 'darwin already, if it's not enough,
this belongs in browse-url.el

 

Re: Emacs.app merged

[Stefan Monnier]

Hi again, some more things:

- The support for GNUstep and Cocoa needs to be mentioned in etc/NEWS
- lisp/term/ns-win.el should not redefine blink-cursor-mode.
  If at all possible, Emacs.app's cursor blinking code should use the
  same code used on all other platforms.
  If it's not possible, the reason should be documented, and then
  blink-cursor-mode needs to be changed to set ns-cursor-blink-mode
  when appropriate.
- I've installed changes to keymap.c which add
  a `where-is-preferred-modifier'.  It's not being thoroughly tested, so
  if it doesn't do what you want, please yell.  Its default is currently
  nil, so you may want to change that somewhere.


        Stefan

Re: Emacs.app merged

[Adrian Robert]

On Jul 16, 2008, at 3:26 PM, Stefan Monnier wrote:

> Hi again, some more things:
>
> - The support for GNUstep and Cocoa needs to be mentioned in etc/NEWS

Will add soon (once I finish configure/build changes).


> - lisp/term/ns-win.el should not redefine blink-cursor-mode.
>  If at all possible, Emacs.app's cursor blinking code should use the
>  same code used on all other platforms.

I don't see any reason the common code cannot be used, but I've never  
gotten around to trying.  If anyone wants to help on this one, that  
would be great.


> - I've installed changes to keymap.c which add
>  a `where-is-preferred-modifier'.  It's not being thoroughly tested,  
> so
>  if it doesn't do what you want, please yell.  Its default is  
> currently
>  nil, so you may want to change that somewhere.

Excellent, I will try...

some missing code? (was: Re: Emacs.app merged)

[Dan Nicolaescu]

I noticed that this function in frame.el does not have a check for ns,
shouldn't it?

(defun face-set-after-frame-default (frame &optional parameters)
  "Initialize the frame-local faces of FRAME.
Calculate the face definitions using the face specs, custom theme
settings, X resources, and `face-new-frame-defaults'.
Finally, apply any relevant face attributes found amongst the
frame parameters in PARAMETERS and `default-frame-alist'."
  (dolist (face (nreverse (face-list)))
    (condition-case ()
    (progn
      ;; Initialize faces from face spec and custom theme.
        (face-spec-recalc face frame)
          ;; X resouces for the default face are applied during
            ;; x-create-frame.
              (and (not (eq face 'default))
                     (memq (window-system frame) '(x w32))   
                                                 ^^^^^^^^^
                            (make-face-x-resource-internal face frame))


"x-create-frame" does not do a copy_alist at the beginning like the
other ports to.  On Windows not having that resulted in some strange
behavior.  It might be a good idea to sync that function with the X
version again.


What is the #ifdef HAVE_NS code in frame.c:x_get_frame used for?

                Lisp_Object lower;
                            lower = Fdowncase (tem);
                                  if (!strcmp (SDATA (lower), "on")
#ifdef HAVE_NS
                    || !strcmp(SDATA(lower), "yes")
#endif
                    || !strcmp (SDATA (lower), "true"))
                         return Qt;
                                else if (!strcmp (SDATA (lower), "off")
#ifdef HAVE_NS
                      || !strcmp(SDATA(lower), "no")
#endif
                      || !strcmp (SDATA (lower), "false"))
                           return Qnil;
                                  else
                            return Fintern (tem, Qnil);

Is "yes" and "no" something that some other program on the system can
write? Or something that the user would write?  If the latter, then
wouldn't it be better to just teach the users to use the values all
other systems uses and avoid complications in code and docs?

Please take a look.

Thanks

        --dan       

observations for ns*.m files (Re: Emacs.app merged)

[Dan Nicolaescu]

Here are some observations for the code in the syms_of_* ns*.m files. 
Can you please address these?

void
syms_of_nsfns ()
{
  int i;

  Qns_frame_parameter = intern ("ns-frame-parameter");
  		      	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This seems unused, better get rid of it.

  Qbuffered = intern ("bufferd");

Did this ever work given the typo?  Is it something that people would
care enough about to have an option?


  Qfontsize = intern ("fontsize");
  staticpro (&Qfontsize);

This seems a generic facility, should it be here?  Can't the generic
way of specifying fonts work on this platform too?


  DEFVAR_LISP ("ns-icon-type-alist", &Vns_icon_type_alist,
               doc: /* Alist of elements (REGEXP . IMAGE) for images of icons associated to frames.
If the title of a frame matches REGEXP, then IMAGE.tiff is
selected as the image of the icon representing the frame when it's
miniaturized.  If an element is t, then Emacs tries to select an icon
based on the filetype of the visited file.

The images have to be installed in a folder called English.lproj in the
Emacs folder.  You have to restart Emacs after installing new icons.

Example: Install an icon Gnus.tiff and execute the following code

  (setq ns-icon-type-alist
        (append ns-icon-type-alist
                '((\"^\\\\*\\\\(Group\\\\*$\\\\|Summary \\\\|Article\\\\*$\\\\)\"
[snip]

This looks like a generic thing, better not make it platform
specific.  Is there a problem with the normal way of specifying icons?


void
syms_of_nsselect (void)
{

/* 23: { */
  DEFVAR_LISP ("selection-coding-system", &Vselection_coding_system,
         doc: /* Coding system for communicating with other programs.
When sending or receiving text via cut_buffer, selection, and clipboard,
the text is encoded or decoded by this coding system.
The default value is determined by the system script code.  */);
  Vselection_coding_system = Qnil;

  DEFVAR_LISP ("next-selection-coding-system", &Vnext_selection_coding_system,
         doc: /* Coding system for the next communication with other programs.
Usually, `selection-coding-system' is used for communicating with
other programs.  But, if this variable is set, it is used for the
next communication only.  After the communication, this variable is
set to nil.  */);
  Vnext_selection_coding_system = Qnil;

These were removed from the X code:

2008-02-01  Kenichi Handa  <handa@m17n.org>

	    * xselect.c (Vselection_coding_system)
	    (Vnext_selection_coding_system): Delete them.

are they still needed for ns?


void
syms_of_nsterm ()
{
  DEFVAR_LISP ("ns-command-modifier", &ns_command_modifier,
               "This variable describes the behavior of the command key.\n\
Set to control, meta, alt, super, or hyper means it is taken to be that key.");

  DEFVAR_LISP ("ns-control-modifier", &ns_control_modifier,
               "This variable describes the behavior of the control key.\n\
Set to control, meta, alt, super, or hyper means it is taken to be that key.");

These 2 look identical, are they both needed?  Are they needed at all
after the recent modifier changes?


  DEFVAR_LISP ("ns-cursor-blink-mode", &ns_cursor_blink_mode,
               "Internal variable -- use M-x blink-cursor-mode or preferences\n\
panel to control this setting.");

Is this needed?  Can't the standard blink-cursor-mode be used?

  DEFVAR_LISP ("ns-cursor-blink-rate", &ns_cursor_blink_rate,
               "Rate at which the Emacs cursor blinks (in seconds).\n\
Set to nil to disable blinking.");

Can't blink-cursor-rate be used?
(IMHO blink-cursor-mode should be off by default anyway, but that's a
completely different topic).


  DEFVAR_LISP ("ns-expand-space", &ns_expand_space,
               "Amount by which spacing between lines is expanded (positive)\n\
or shrunk (negative).  Zero (the default) means standard line height.\n\
(This variable should only be read, never set.)");

This is generic, better not make it ns specific.  Can this can be done
in platform specific code without changes to redisplay?  It seems a
bit strange anyway.



  DEFVAR_LISP ("ns-antialias-text", &ns_antialias_text,
               "Non-nil (the default) means to render text antialiased. Only has an effect on OS X Panther and above.");


Can the generic functionality be used instead of this?

  DEFVAR_LISP ("ns-use-system-highlight-color",
               &ns_use_system_highlight_color,
               "Whether to use the system default (on OS X only) for the highlight color.  Nil means to use standard emacs (prior to version 21) 'grey'.");

Is this the region color or something else?

Thanks

        --dan

Re: observations for ns*.m files (Re: Emacs.app merged)

[Adrian Robert]

[-- Attachment #1: Type: text/plain, Size: 6054 bytes --]

On Jul 27, 2008, at 4:12 PM, Dan Nicolaescu wrote:

> I noticed that this function in frame.el does not have a check for ns,
> shouldn't it?
>
> (defun face-set-after-frame-default (frame &optional parameters)
>  "Initialize the frame-local faces of FRAME.

Yes, thanks, added.  Note, most of these window-system checks  
throughout the lisp code simply check for every window system (x, w32,  
ns, and formerly, mac).  It might be good at some point to change all  
these to simply check whether a window system is being used.


> "x-create-frame" does not do a copy_alist at the beginning like the
> other ports to.  On Windows not having that resulted in some strange
> behavior.  It might be a good idea to sync that function with the X
> version again.

OK.


> What is the #ifdef HAVE_NS code in frame.c:x_get_frame used for?
> ...
>                                 if (!strcmp (SDATA (lower), "on")
> #ifdef HAVE_NS
>                    || !strcmp(SDATA(lower), "yes")
> #endif
>                    || !strcmp (SDATA (lower), "true"))
> ...
> Is "yes" and "no" something that some other program on the system can
> write? Or something that the user would write?  If the latter, then
> wouldn't it be better to just teach the users to use the values all
> other systems uses and avoid complications in code and docs?

"YES" and "NO" are the standard boolean values used in the NS defaults  
system.  A property list editor or a user could write them.  As far as  
teaching users, I would hope that the purpose of application software  
is to adapt computers to users rather than the other way around!  ;)


On Jul 27, 2008, at 6:18 PM, Dan Nicolaescu wrote:

> void
> syms_of_nsfns ()
>  ...
>  Qns_frame_parameter = intern ("ns-frame-parameter");
>  		      	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This seems unused, better get rid of it.

Done.



>  Qbuffered = intern ("bufferd");
>
> Did this ever work given the typo?  Is it something that people would
> care enough about to have an option?

I think it's just something I copied from an earlier version of x- 
create-frame in some other term.  I've replaced it with alpha.



>  Qfontsize = intern ("fontsize");
>  staticpro (&Qfontsize);
>
> This seems a generic facility, should it be here?  Can't the generic
> way of specifying fonts work on this platform too?

I'm not sure if there is a generic facility.  On X, font size is part  
of the XLFD strings.  These are not used except where absolutely  
necessary on NS, and I would like to get rid of these cases.



>  DEFVAR_LISP ("ns-icon-type-alist", &Vns_icon_type_alist,
>               doc: /* Alist of elements (REGEXP . IMAGE) for images  
> of icons associated to frames.
> ...
> This looks like a generic thing, better not make it platform
> specific.  Is there a problem with the normal way of specifying icons?

What is the normal way?  I think this facility in the NS port probably  
dates from before emacs itself had this feature, so certainly we  
should try to use the generic code now.



> void
> syms_of_nsselect (void)
> {
>
> /* 23: { */
>  DEFVAR_LISP ("selection-coding-system", &Vselection_coding_system,
> ...
> DEFVAR_LISP ("next-selection-coding-system",  
> &Vnext_selection_coding_system,
>
> These were removed from the X code:
>
> 2008-02-01  Kenichi Handa  <handa@m17n.org>
>
> 	    * xselect.c (Vselection_coding_system)
> 	    (Vnext_selection_coding_system): Delete them.
>
> are they still needed for ns?

These were not actually used in the NS code, so I've now dropped  
them.  However note they are still present in W32 and DOS.



> void
> syms_of_nsterm ()
> {
>  DEFVAR_LISP ("ns-command-modifier", &ns_command_modifier,
>               "This variable describes the behavior of the command  
> key.\n\
> Set to control, meta, alt, super, or hyper means it is taken to be  
> that key.");
>
>  DEFVAR_LISP ("ns-control-modifier", &ns_control_modifier,
>               "This variable describes the behavior of the control  
> key.\n\
> Set to control, meta, alt, super, or hyper means it is taken to be  
> that key.");
>
> These 2 look identical, are they both needed?  Are they needed at all
> after the recent modifier changes?

On Mac keyboards, Command and Control are separate keys, so both are  
needed.  What recent modifier changes are you referring to?



>  DEFVAR_LISP ("ns-cursor-blink-mode", &ns_cursor_blink_mode,
>               "Internal variable -- use M-x blink-cursor-mode or  
> preferences\n\
> panel to control this setting.");
>
> Is this needed?  Can't the standard blink-cursor-mode be used?

That's the goal, but it's not trivial (http://thread.gmane.org/gmane.emacs.devel/101190/focus=101588 
).  Hopefully eventually.



>  DEFVAR_LISP ("ns-expand-space", &ns_expand_space,
>               "Amount by which spacing between lines is expanded  
> (positive)\n\
> or shrunk (negative).  Zero (the default) means standard line height. 
> \n\
> (This variable should only be read, never set.)");
>
> This is generic, better not make it ns specific.  Can this can be done
> in platform specific code without changes to redisplay?  It seems a
> bit strange anyway.

This might be the same as the lineSpacing frame parameter.  Can  
someone say whether it is?  ns-expand-space is as documented above, a  
way for the user to customize how close together or spread apart lines  
are vertically.  If so, we can hook up this code to that parameter.



>  DEFVAR_LISP ("ns-antialias-text", &ns_antialias_text,
>               "Non-nil (the default) means to render text  
> antialiased. Only has an effect on OS X Panther and above.");
>
> Can the generic functionality be used instead of this?

Which generic functionality?



>  DEFVAR_LISP ("ns-use-system-highlight-color",
>               &ns_use_system_highlight_color,
>               "Whether to use the system default (on OS X only) for  
> the highlight color.  Nil means to use standard emacs (prior to  
> version 21) 'grey'.");
>
> Is this the region color or something else?

Yes, the 'region' face.  I'll update the doc.



[-- Attachment #2: Type: text/html, Size: 9454 bytes --]

Re: observations for ns*.m files (Re: Emacs.app merged)

[Dan Nicolaescu]

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

  > On Jul 27, 2008, at 4:12 PM, Dan Nicolaescu wrote:
  > 
  > OK.
  > 
  > 
  > 
  >     What is the #ifdef HAVE_NS code in frame.c:x_get_frame used for?
  >     ...
  > 
  >                                     if (!strcmp (SDATA (lower), "on")
  >     #ifdef HAVE_NS
  >                        || !strcmp(SDATA(lower), "yes")
  >     #endif
  >                        || !strcmp (SDATA (lower), "true"))
  >     ...
  >     Is "yes" and "no" something that some other program on the system can
  >     write? Or something that the user would write?  If the latter, then
  >     wouldn't it be better to just teach the users to use the values all
  >     other systems uses and avoid complications in code and docs?
  > 
  > 
  > "YES" and "NO" are the standard boolean values used in the NS defaults system.
  >  A property list editor or a user could write them.  
  > As far as teaching users,
  > I would hope that the purpose of application software is to adapt computers to
  > users rather than the other way around!  ;)

I was referring to the case where the users were taught to write
"yes"/"no" in order to use this stuff in emacs, they can be taught to
use the cross-platform standard values instead.

  >      Qfontsize = intern ("fontsize");
  >      staticpro (&Qfontsize);
  >    
  >     This seems a generic facility, should it be here?  Can't the generic
  >     way of specifying fonts work on this platform too?
  > 
  > 
  > I'm not sure if there is a generic facility.  On X, font size is part of the
  > XLFD strings.  These are not used except where absolutely necessary on NS, and
  > I would like to get rid of these cases.

At least talk to the Windows people because they probably have the same
problem.  This is something very basic, it would be a pity to have
different solutions for different platform.

  > 
  >      DEFVAR_LISP ("ns-icon-type-alist", &Vns_icon_type_alist,
  >                   doc: /* Alist of elements (REGEXP . IMAGE) for images of
  >     icons associated to frames.
  >     ...
  > 
  >     This looks like a generic thing, better not make it platform
  >     specific.  Is there a problem with the normal way of specifying icons?
  > 
  > 
  > What is the normal way?  I think this facility in the NS port probably dates
  > from before emacs itself had this feature, so certainly we should try to use
  > the generic code now.

You can set a icon as a frame parameter.

  >     void
  >     syms_of_nsterm ()
  >     {
  >      DEFVAR_LISP ("ns-command-modifier", &ns_command_modifier,
  >                   "This variable describes the behavior of the command key.\n\
  >     Set to control, meta, alt, super, or hyper means it is taken to be that
  >     key.");
  >    
  >      DEFVAR_LISP ("ns-control-modifier", &ns_control_modifier,
  >                   "This variable describes the behavior of the control key.\n\
  >     Set to control, meta, alt, super, or hyper means it is taken to be that
  >     key.");
  >    
  >     These 2 look identical, are they both needed?  Are they needed at all
  >     after the recent modifier changes?
  > 
  > 
  > On Mac keyboards, Command and Control are separate keys, so both are needed.

So you can change the meaning of Control?  That sounds strange.  Maybe
Meta/Alt are sometime confused, but Control should be pretty well
established.  Do users actually want this?

  >  What recent modifier changes are you referring to?

where-is-preferred-modifier

  >      DEFVAR_LISP ("ns-antialias-text", &ns_antialias_text,
  >                   "Non-nil (the default) means to render text antialiased. Only
  >     has an effect on OS X Panther and above.");
  >    
  >     Can the generic functionality be used instead of this?
  > 
  > 
  > Which generic functionality?

There's support for antialiasing in X, I know no details about it as I
don't use it.

One more thing:

DEFUN ("ns-set-alpha", Fns_set_alpha, Sns_set_alpha, 2, 2, 0,
       doc: /* Return a color equivalent to COLOR with alpha setting
       ALPHA.
The argument ALPHA should be a number between 0 and 1, where 0 is full
transparency and 1 is opaque.  */)

Not sure about the details, but can this functionality be replaced by
an implementation of x_set_frame_alpha?

Re: observations for ns*.m files (Re: Emacs.app merged)

[Stefan Monnier]

> One more thing:

> DEFUN ("ns-set-alpha", Fns_set_alpha, Sns_set_alpha, 2, 2, 0,
>        doc: /* Return a color equivalent to COLOR with alpha setting
>        ALPHA.
> The argument ALPHA should be a number between 0 and 1, where 0 is full
> transparency and 1 is opaque.  */)

> Not sure about the details, but can this functionality be replaced by
> an implementation of x_set_frame_alpha?

No, it can't.  `ns-set-alpha' uses the alpha channels for colors, which
is much more refined than the crude x_set_frame_alpha.


        Stefan

Re: observations for ns*.m files (Re: Emacs.app merged)

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> No, it can't.  `ns-set-alpha' uses the alpha channels for colors, which
> is much more refined than the crude x_set_frame_alpha.

It would certainly be cool to update the emacs core code to keep track
of alpha as well -- since emacs general color usage is fairly simple,
for many cases it could probably be handled internally even for backends
that don't support an alpha channel...

-Miles

-- 
XML is like violence.  If it doesn't solve your problem, you're not
using enough of it.

Re: observations for ns*.m files (Re: Emacs.app merged)

[Jason Rumney]

Dan Nicolaescu wrote:
> Adrian Robert <adrian.b.robert@gmail.com> writes:

>   > I'm not sure if there is a generic facility.  On X, font size is part of the
>   > XLFD strings.  These are not used except where absolutely necessary on NS, and
>   > I would like to get rid of these cases.
> 
> At least talk to the Windows people because they probably have the same
> problem.  This is something very basic, it would be a pity to have
> different solutions for different platform.

Emacs 23 accepts 3 different formats for specifying fonts. In addition
to XLFD, there are the fontconfig ("Monaco-10", "Monaco-10:bold"...) and
GTK ("Monaco 10", "Monaco Bold 10") formats. If the point size is left
out, a default of I think 12 is assumed.

Re: observations for ns*.m files (Re: Emacs.app merged)

[Adrian Robert]

On Jul 28, 2008, at 3:15 AM, Jason Rumney wrote:

> Emacs 23 accepts 3 different formats for specifying fonts. In addition
> to XLFD, there are the fontconfig ("Monaco-10", "Monaco-10:bold"...)  
> and
> GTK ("Monaco 10", "Monaco Bold 10") formats. If the point size is left
> out, a default of I think 12 is assumed.

Thanks.  The font backend system handles size and font name  
separately.  I will have to study the code to see where the parsing  
takes place that separates off the size attribute from the name.

The nice thing about the fontSize frame parameter is it ties into this  
aspect of the backend, and allows the user to easily control font size  
separately from the font.

Also, how does one specify multiple attributes in the new formats; are  
these correct?

Monaco-10:bold,italic
Monaco Bold Italic 10

Re: observations for ns*.m files (Re: Emacs.app merged)

[Chong Yidong]

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

> On Jul 28, 2008, at 3:15 AM, Jason Rumney wrote:
>
>> Emacs 23 accepts 3 different formats for specifying fonts. In addition
>> to XLFD, there are the fontconfig ("Monaco-10", "Monaco-10:bold"...)
>> and
>> GTK ("Monaco 10", "Monaco Bold 10") formats. If the point size is left
>> out, a default of I think 12 is assumed.
>
> Thanks.  The font backend system handles size and font name
> separately.  I will have to study the code to see where the parsing
> takes place that separates off the size attribute from the name.

See font_parse_xlfd and font_parse_fcname (and their comments) in
font.c.

Re: observations for ns*.m files (Re: Emacs.app merged)

[Jason Rumney]

Adrian Robert wrote:

> Also, how does one specify multiple attributes in the new formats; are
> these correct?
> 
> Monaco-10:bold,italic
> Monaco Bold Italic 10

The latter is correct, and is about as advanced as GTK font specs can
get. In fontconfig format it would be:

Monaco-10:bold:italic

Or using a more generic notation which can be extended to fit your needs
(such as specifying whether to antialias):

Monaco:size=10:weight=bold:slant=italic

Re: observations for ns*.m files (Re: Emacs.app merged)

[Adrian Robert]

On Jul 27, 2008, at 10:58 PM, Dan Nicolaescu wrote:

>>     DEFVAR_LISP ("ns-icon-type-alist", &Vns_icon_type_alist,
>>                  doc: /* Alist of elements (REGEXP . IMAGE) for  
>> images of
>>    icons associated to frames.
>>    ...
>>
>>    This looks like a generic thing, better not make it platform
>>    specific.  Is there a problem with the normal way of specifying  
>> icons?
>>
>>
>> What is the normal way?  I think this facility in the NS port  
>> probably dates
>> from before emacs itself had this feature, so certainly we should  
>> try to use
>> the generic code now.
>
> You can set a icon as a frame parameter.

This is a different approach.  ns-icon-type-alist allows all frames w/ 
titles matching a regexp to get a certain icon.  I'm not sure how  
widely used this is, but the code is there, it's working, there have  
been no complaints about it, I'd rather leave it.  Someone took the  
effort to write it at some point, and I assume it has been found useful.



>>    void
>>    syms_of_nsterm ()
>>    {
>>     DEFVAR_LISP ("ns-command-modifier", &ns_command_modifier,
>>                  "This variable describes the behavior of the  
>> command key.\n\
>>    Set to control, meta, alt, super, or hyper means it is taken to  
>> be that
>>    key.");
>>
>>     DEFVAR_LISP ("ns-control-modifier", &ns_control_modifier,
>>                  "This variable describes the behavior of the  
>> control key.\n\
>>    Set to control, meta, alt, super, or hyper means it is taken to  
>> be that
>>    key.");
>>
>>    These 2 look identical, are they both needed?  Are they needed  
>> at all
>>    after the recent modifier changes?
>>
>>
>> On Mac keyboards, Command and Control are separate keys, so both  
>> are needed.
>
> So you can change the meaning of Control?  That sounds strange.  Maybe
> Meta/Alt are sometime confused, but Control should be pretty well
> established.  Do users actually want this?

I'm not sure, they definitely want to map modifiers for alt and  
command keys.  It's more consistent to handle all modifiers the same  
way, and keeps the code simpler.  Furthermore, the control panel  
interface on OS X where modifiers can be remapped also shows all keys,  
so this is consistent with that.


>> What recent modifier changes are you referring to?
>
> where-is-preferred-modifier

OK, no that relates to a different issue.



>>     DEFVAR_LISP ("ns-antialias-text", &ns_antialias_text,
>>                  "Non-nil (the default) means to render text  
>> antialiased. Only
>>    has an effect on OS X Panther and above.");
>>
>>    Can the generic functionality be used instead of this?
>>
>>
>> Which generic functionality?
>
> There's support for antialiasing in X, I know no details about it as I
> don't use it.

Maybe there's a case for x-antialias-text now, applicable to all  
platforms, with caveat that it's ignored where not supported.



> One more thing:
>
> DEFUN ("ns-set-alpha", Fns_set_alpha, Sns_set_alpha, 2, 2, 0,
>       doc: /* Return a color equivalent to COLOR with alpha setting
>       ALPHA.
> The argument ALPHA should be a number between 0 and 1, where 0 is full
> transparency and 1 is opaque.  */)
>
> Not sure about the details, but can this functionality be replaced by
> an implementation of x_set_frame_alpha?

This is a utility function.  There's also ns-set-background-alpha.  I  
would like to replace it with the new alpha frame parameter, but the  
meaning is different.  For x_set_frame_alpha, the alpha of the entire  
window is intended (including the text).  ns-set-background-alpha  
changes only the background, which usually results in more readable  
text.  But I'm not sure that any other platform can support this right  
now.  Maybe drop ns-set-background-alpha and make a customized  
variable ns-frame-alpha-affects-only-background?

Re: observations for ns*.m files (Re: Emacs.app merged)

[Dan Nicolaescu]

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

  > On Jul 27, 2008, at 10:58 PM, Dan Nicolaescu wrote:
  > 
  > >>     DEFVAR_LISP ("ns-icon-type-alist", &Vns_icon_type_alist,
  > >>                  doc: /* Alist of elements (REGEXP . IMAGE) for
  > >> images of
  > >>    icons associated to frames.
  > >>    ...
  > >>
  > >>    This looks like a generic thing, better not make it platform
  > >>    specific.  Is there a problem with the normal way of specifying
  > >> icons?
  > >>
  > >>
  > >> What is the normal way?  I think this facility in the NS port
  > >> probably dates
  > >> from before emacs itself had this feature, so certainly we should
  > >> try to use
  > >> the generic code now.
  > >
  > > You can set a icon as a frame parameter.
  > 
  > This is a different approach.  ns-icon-type-alist allows all frames w/
  > titles matching a regexp to get a certain icon.  I'm not sure how
  > widely used this is, but the code is there, it's working, there have
  > been no complaints about it, I'd rather leave it.  

The problem is that this is generic functionality, not ns specific, so
it should not be in ns specific files.  The thing is if something makes
it into a released version of emacs it will have to be supported until
the end of time.  If some functionality gets implemented differently
later in the cross platform part of emacs, it will just add complexity
to the code.  And it will make the life of elisp writers more
complicated: they'd have to write (if (featurep 'ns) (foo) (bar)) or
(if (boundp 'ns-icon-type-alist) (foo) (bar)).  That is unclean and
better avoid it.

  > Someone took the effort to write it at some point, and I assume it
  > has been found useful.

Again, please don't try to force generic functionality in through
platform specific code.

  > >>    void
  > >>    syms_of_nsterm ()
  > >>    {
  > >>     DEFVAR_LISP ("ns-command-modifier", &ns_command_modifier,
  > >>                  "This variable describes the behavior of the
  > >> command key.\n\
  > >>    Set to control, meta, alt, super, or hyper means it is taken to
  > >> be that
  > >>    key.");
  > >>
  > >>     DEFVAR_LISP ("ns-control-modifier", &ns_control_modifier,
  > >>                  "This variable describes the behavior of the
  > >> control key.\n\
  > >>    Set to control, meta, alt, super, or hyper means it is taken to
  > >> be that
  > >>    key.");
  > >>
  > >>    These 2 look identical, are they both needed?  Are they needed
  > >> at all
  > >>    after the recent modifier changes?
  > >>
  > >>
  > >> On Mac keyboards, Command and Control are separate keys, so both
  > >> are needed.
  > >
  > > So you can change the meaning of Control?  That sounds strange.  Maybe
  > > Meta/Alt are sometime confused, but Control should be pretty well
  > > established.  Do users actually want this?
  > 
  > I'm not sure, they definitely want to map modifiers for alt and
  > command keys.  It's more consistent to handle all modifiers the same
  > way, and keeps the code simpler.  Furthermore, the control panel
  > interface on OS X where modifiers can be remapped also shows all keys,
  > so this is consistent with that.

IMVHO this is just an extra knob that almost nobody would use.  Did the
Carbon port have something similar?

  > >>     DEFVAR_LISP ("ns-antialias-text", &ns_antialias_text,
  > >>                  "Non-nil (the default) means to render text
  > >> antialiased. Only
  > >>    has an effect on OS X Panther and above.");
  > >>
  > >>    Can the generic functionality be used instead of this?
  > >>
  > >>
  > >> Which generic functionality?
  > >
  > > There's support for antialiasing in X, I know no details about it as I
  > > don't use it.
  > 
  > Maybe there's a case for x-antialias-text now, applicable to all
  > platforms, with caveat that it's ignored where not supported.

Probably, please talk to Handa-san about that.

  > > One more thing:
  > >
  > > DEFUN ("ns-set-alpha", Fns_set_alpha, Sns_set_alpha, 2, 2, 0,
  > >       doc: /* Return a color equivalent to COLOR with alpha setting
  > >       ALPHA.
  > > The argument ALPHA should be a number between 0 and 1, where 0 is full
  > > transparency and 1 is opaque.  */)
  > >
  > > Not sure about the details, but can this functionality be replaced by
  > > an implementation of x_set_frame_alpha?
  > 
  > This is a utility function.  There's also ns-set-background-alpha.  I
  > would like to replace it with the new alpha frame parameter, but the
  > meaning is different.  For x_set_frame_alpha, the alpha of the entire
  > window is intended (including the text).  ns-set-background-alpha
  > changes only the background, which usually results in more readable
  > text.  But I'm not sure that any other platform can support this right
  > now.  Maybe drop ns-set-background-alpha and make a customized
  > variable ns-frame-alpha-affects-only-background?

If this is a better interface to this functionality, then IMHO it would
be better make it available under a generic name, and then the other
platforms can catch up and use this name.  Stefan and Jason had opinions
about this in other replies, so you might want to talk to them about the
details.

build system observations (was Re: Emacs.app merged)

[Dan Nicolaescu]

Here are some comments on the build system changes for NS.

configure.in:
#ifdef HAVE_NS
# ifdef NS_IMPL_GNUSTEP
/* See also .m.o rule in Makefile.in */
#  define C_SWITCH_X_SYSTEM -MMD -MP -D_REENTRANT -fPIC -fno-strict-aliasing
                            ^^^^^^^^^
                            Does anything use the dependency info produced here?  If not, better not produce it.
#  define GNU_OBJC_CFLAGS -fgnu-runtime -Wno-import -fconstant-string-class=NSConstantString -DGNUSTEP_BASE_LIBRARY=1 -DGNU_GUI_LIBRARY=1 -DGNU_RUNTIME=1 -DGSWARN -DGSDIAGNOSE
      This is a new flag, better use autoconf substitions instead of the preprocessor.


# ifdef C_SWITCH_SYSTEM
# undef C_SWITCH_SYSTEM
# endif

This sequence has not effect, it can be removed.


lib-src/Makefile.in:

#if defined(NS_IMPL_COCOA)
/* Build these programs as universal binaries. */
CFLAGS := $(CFLAGS) -universal

  Is this needed?  It does not seem that the emacs binary itself is built in the same way.
  There are many ways of adding stuff to CFLAGS, it's cleaner to not add yet another #ifdef for this.

/* Add mac-fix-env for OS X systems running NS version. */
INSTALLABLES = etags${EXEEXT} ctags${EXEEXT} emacsclient${EXEEXT} b2m${EXEEXT} ebrowse${EXEEXT} mac-fix-env${EXEEXT}
  This can be done more elegantly with and autoconf substitution like this:

INSTALLABLES = etags${EXEEXT} ctags${EXEEXT} emacsclient${EXEEXT} b2m${EXEEXT} ebrowse${EXEEXT} @LIB_SRC_EXTRA_INSTALLABLES@

  and define LIB_SRC_EXTRA_INSTALLABLES with the right value in the NS_IMPL_COCOA case.


.m.o:
#ifdef NS_IMPL_GNUSTEP
       $(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) -fgnu-runtime -Wno-import -fconstant-string-class=NSConstantString $<
#else
        $(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) $<
#endif

  Why the #ifdef, can't the GNU_OBJC_CFLAGS be used instead?


src/Makefile.in:

/* XXX: not working under NS currently due to path shenanigans.. */
#ifndef HAVE_NS
        -./emacs -q -batch -f list-load-path-shadows
#endif

    This just hides problems and complicates the Makefile.  Better take the #ifndef out.


#ifdef NS_IMPL_GNUSTEP
       rm -f *.d
#endif
   If dependency info is not produced with C_SWITCH_X_SYSTEM, then there's no need to remove it here.



#ifdef HAVE_NS
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 sound.o: nsgui.h
     This can be solved by adding an autoconf variable NSGUI_H and make all
     these .o file depend on $(NSGUI_H) which is nsgui.h for NS and nothing everywhere else.
     After that the above #ifdef can probably be removed.

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)

    All these can go in the alphabetical list of dependencies. 

Hope this helps.

Re: build system observations (was Re: Emacs.app merged)

[Adrian Robert]

On Aug 5, 2008, at 1:13 AM, Dan Nicolaescu wrote:

> Here are some comments on the build system changes for NS.

Thanks..  I'll address what I'm able to below.



> configure.in:
> #ifdef HAVE_NS
> # ifdef NS_IMPL_GNUSTEP
> /* See also .m.o rule in Makefile.in */
> #  define C_SWITCH_X_SYSTEM -MMD -MP -D_REENTRANT -fPIC -fno-strict- 
> aliasing
>                            ^^^^^^^^^
>                            Does anything use the dependency info  
> produced here?  If not, better not produce it.

OK, I removed -MMD -MP (probably came from early attempts to replicate  
gnustep-make behavior), but I cannot test the effect -- can anyone  
compiling under GNUstep let me know if it causes problems?



> #  define GNU_OBJC_CFLAGS -fgnu-runtime -Wno-import -fconstant- 
> string-class=NSConstantString -DGNUSTEP_BASE_LIBRARY=1 - 
> DGNU_GUI_LIBRARY=1 -DGNU_RUNTIME=1 -DGSWARN -DGSDIAGNOSE
>      This is a new flag, better use autoconf substitions instead of  
> the preprocessor.

I'm not sure how to do this, but have no objection to someone else  
making the change if it simplifies things.



> # ifdef C_SWITCH_SYSTEM
> # undef C_SWITCH_SYSTEM
> # endif
>
> This sequence has not effect, it can be removed.

This actually comes BEFORE the above definitions, and is to prevent  
redefinition errors.  Some files under src/s define this, but they do  
so for X-Windows purposes.  The NS port using GNUstep can be built on  
these systems and therefore needs to change the switch.



> lib-src/Makefile.in:
>
> #if defined(NS_IMPL_COCOA)
> /* Build these programs as universal binaries. */
> CFLAGS := $(CFLAGS) -universal
>
>  Is this needed?  It does not seem that the emacs binary itself is  
> built in the same way.
>  There are many ways of adding stuff to CFLAGS, it's cleaner to not  
> add yet another #ifdef for this.

I took it out.  Anyone attempting to build a binary distribution will  
need to add -universal to CFLAGS, but only in lib-src.  What is the  
easiest way to do this?



> /* Add mac-fix-env for OS X systems running NS version. */
> INSTALLABLES = etags${EXEEXT} ctags${EXEEXT} emacsclient${EXEEXT} b2m 
> ${EXEEXT} ebrowse${EXEEXT} mac-fix-env${EXEEXT}
>  This can be done more elegantly with and autoconf substitution like  
> this:
>
> INSTALLABLES = etags${EXEEXT} ctags${EXEEXT} emacsclient${EXEEXT} b2m 
> ${EXEEXT} ebrowse${EXEEXT} @LIB_SRC_EXTRA_INSTALLABLES@
>
>  and define LIB_SRC_EXTRA_INSTALLABLES with the right value in the  
> NS_IMPL_COCOA case.

This sounds good (maybe LIB_SRC_ARCHDEP_INSTALLABLES) but I don't  
trust my autoconf skills enough to attempt this change.


> .m.o:
> #ifdef NS_IMPL_GNUSTEP
>       $(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) -fgnu-runtime -Wno-import - 
> fconstant-string-class=NSConstantString $<
> #else
>        $(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) $<
> #endif

I got rid of it, unneeded since only .m is compiled on Cocoa.



> src/Makefile.in:
>
> /* XXX: not working under NS currently due to path shenanigans.. */
> #ifndef HAVE_NS
>        -./emacs -q -batch -f list-load-path-shadows
> #endif
>
>    This just hides problems and complicates the Makefile.  Better  
> take the #ifndef out.

I'd welcome help on this one.  I guess the Carbon port didn't need it,  
but despite NS using identical "path shenanigans" to the Carbon port,  
it didn't work there.  Not sure what the problem is.



> #ifdef NS_IMPL_GNUSTEP
>       rm -f *.d
> #endif
>   If dependency info is not produced with C_SWITCH_X_SYSTEM, then  
> there's no need to remove it here.

OK, done.




> #ifdef HAVE_NS
> 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  
> sound.o: nsgui.h
>     This can be solved by adding an autoconf variable NSGUI_H and  
> make all
>     these .o file depend on $(NSGUI_H) which is nsgui.h for NS and  
> nothing everywhere else.
>     After that the above #ifdef can probably be removed.

This sounds more complicated than what is there now.  If the goal is  
to get rid of the Makefile.c system shouldn't this be done all at once  
and for all GUIs?  (Most of the #ifdef complexity in that file is not  
due to NS.)



> 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)
>
>    All these can go in the alphabetical list of dependencies.

Moved.  Note the following files are placed out of order at the end of  
the list for some reason:

gtkutil.o
dbusbind.o
hftctl.o
sound.o
atimer.o

In the "Lisp proper" section,

lread.o is out of order.

Under "Text properties support", the files are in reverse alphabetical.

Re: build system observations

[Dan Nicolaescu]

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

  > On Aug 5, 2008, at 1:13 AM, Dan Nicolaescu wrote:

  > > #  define GNU_OBJC_CFLAGS -fgnu-runtime -Wno-import -fconstant-
  > > string-class=NSConstantString -DGNUSTEP_BASE_LIBRARY=1 -
  > > DGNU_GUI_LIBRARY=1 -DGNU_RUNTIME=1 -DGSWARN -DGSDIAGNOSE
  > >      This is a new flag, better use autoconf substitions instead of
  > > the preprocessor.
  > 
  > I'm not sure how to do this, but have no objection to someone else
  > making the change if it simplifies things.

In configure.in:
GNU_OBJC_CFLAGS=...
AC_SUBST(GNU_OBJC_CFLAGS)

and use @GNU_OBJC_CFLAGS@ in Makefile.in

  > 
  > > # ifdef C_SWITCH_SYSTEM
  > > # undef C_SWITCH_SYSTEM
  > > # endif
  > >
  > > This sequence has not effect, it can be removed.
  > 
  > This actually comes BEFORE the above definitions, and is to prevent
  > redefinition errors.  Some files under src/s define this, but they do
  > so for X-Windows purposes.  The NS port using GNUstep can be built on
  > these systems and therefore needs to change the switch.

Please look at how src/config.h is generated, it will have /* # undef C_SWITCH_SYSTEM */
so I think this never did anything.

  > > lib-src/Makefile.in:
  > >
  > > #if defined(NS_IMPL_COCOA)
  > > /* Build these programs as universal binaries. */
  > > CFLAGS := $(CFLAGS) -universal
  > >
  > >  Is this needed?  It does not seem that the emacs binary itself is
  > > built in the same way.
  > >  There are many ways of adding stuff to CFLAGS, it's cleaner to not
  > > add yet another #ifdef for this.
  > 
  > I took it out.  Anyone attempting to build a binary distribution will
  > need to add -universal to CFLAGS, but only in lib-src.  What is the
  > easiest way to do this?

Add another flag in configure.in?

  > > /* Add mac-fix-env for OS X systems running NS version. */
  > > INSTALLABLES = etags${EXEEXT} ctags${EXEEXT} emacsclient${EXEEXT}
  > > b2m
  > > ${EXEEXT} ebrowse${EXEEXT} mac-fix-env${EXEEXT}
  > >  This can be done more elegantly with and autoconf substitution like
  > > this:
  > >
  > > INSTALLABLES = etags${EXEEXT} ctags${EXEEXT} emacsclient${EXEEXT}
  > > b2m
  > > ${EXEEXT} ebrowse${EXEEXT} @LIB_SRC_EXTRA_INSTALLABLES@
  > >
  > >  and define LIB_SRC_EXTRA_INSTALLABLES with the right value in the
  > > NS_IMPL_COCOA case.
  > 
  > This sounds good (maybe LIB_SRC_ARCHDEP_INSTALLABLES) but I don't

ARCHDEP implies something architecture specific, but that does not seem
to be the case here.

  > trust my autoconf skills enough to attempt this change.

Well, I can do the change, but I can't test it. Unless someone wants
to send me a mac..

  > > src/Makefile.in:
  > >
  > > /* XXX: not working under NS currently due to path shenanigans.. */
  > > #ifndef HAVE_NS
  > >        -./emacs -q -batch -f list-load-path-shadows
  > > #endif
  > >
  > >    This just hides problems and complicates the Makefile.  Better
  > > take the #ifndef out.
  > 
  > I'd welcome help on this one.  I guess the Carbon port didn't need it,
  > but despite NS using identical "path shenanigans" to the Carbon port,
  > it didn't work there.  Not sure what the problem is.

First take it out, and then when people see the problem, someone might
help.  It's just a warning, so it should not harm anyone.

  > > #ifdef HAVE_NS
  > > 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
  > > sound.o: nsgui.h
  > >     This can be solved by adding an autoconf variable NSGUI_H and
  > > make all
  > >     these .o file depend on $(NSGUI_H) which is nsgui.h for NS and
  > > nothing everywhere else.
  > >     After that the above #ifdef can probably be removed.
  > 
  > This sounds more complicated than what is there now.  If the goal is
  > to get rid of the Makefile.c system shouldn't this be done all at once
  > and for all GUIs?  (Most of the #ifdef complexity in that file is not
  > due to NS.)

It's better not add to the complexity and do things the right way from
the start instead of placing the burden on someone else in the future.

The way you did it now, it will force rebuilds for people not using NS
when they update and nsgui.h changes.  I personally won't mind, but is
that the case with everyone else?

  > Moved.  Note the following files are placed out of order at the end of
  > the list for some reason:
  > 
  > gtkutil.o
  > dbusbind.o
  > hftctl.o
  > sound.o
  > atimer.o
  > 
  > In the "Lisp proper" section,
  > 
  > lread.o is out of order.
  > 
  > Under "Text properties support", the files are in reverse alphabetical.

Oh, well...


Thanks!

        --dan

Re: build system observations

[Adrian Robert]

>>> #  define GNU_OBJC_CFLAGS -fgnu-runtime -Wno-import -fconstant-
>>> string-class=NSConstantString -DGNUSTEP_BASE_LIBRARY=1 -
>>> DGNU_GUI_LIBRARY=1 -DGNU_RUNTIME=1 -DGSWARN -DGSDIAGNOSE
>>>     This is a new flag, better use autoconf substitions instead of
>>> the preprocessor.
>>
>> I'm not sure how to do this, but have no objection to someone else
>> making the change if it simplifies things.
>
> In configure.in:
> GNU_OBJC_CFLAGS=...
> AC_SUBST(GNU_OBJC_CFLAGS)
>
> and use @GNU_OBJC_CFLAGS@ in Makefile.in

OK.  Looking more closely there seems to be no pattern to when to use  
@...@ or #define.  In configure.in there is:

# ifdef NS_IMPL_GNUSTEP
#  define C_SWITCH_X_SYSTEM -D_REENTRANT -fPIC -fno-strict-aliasing
#  define GNU_OBJC_CFLAGS -fgnu-runtime -Wno-import ...
...

Since C_SWITCH_X_SYSTEM is a #define, it seems simpler to keep them  
both the same, instead of adding several lines in various places to do  
autoconf substitution.  Getting rid of the Makefile.c system should be  
an explicit project, done all at once.



>>> # ifdef C_SWITCH_SYSTEM
>>> # undef C_SWITCH_SYSTEM
>>> # endif
>>>
>>> This sequence has not effect, it can be removed.
>>
>> This actually comes BEFORE the above definitions, and is to prevent
>> redefinition errors.  Some files under src/s define this, but they do
>> so for X-Windows purposes.  The NS port using GNUstep can be built on
>> these systems and therefore needs to change the switch.
>
> Please look at how src/config.h is generated, it will have /* #  
> undef C_SWITCH_SYSTEM */
> so I think this never did anything.

OK, I see your point.  Removed.



>> I'd welcome help on this one.  I guess the Carbon port didn't need  
>> it,
>> but despite NS using identical "path shenanigans" to the Carbon port,
>> it didn't work there.  Not sure what the problem is.
>
> First take it out, and then when people see the problem, someone might
> help.  It's just a warning, so it should not harm anyone.

Out.

Re: build system observations

[Stefan Monnier]

> substitution.  Getting rid of the Makefile.c system should be an explicit
> project, done all at once.

It's an explicit goal.  Maybe we'll finish it all at once at some point,
but in the mean time, what it means is: don't add CPP directives, and
when changing something, try to remove them.  It can also be
done gradually.


        Stefan