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

[Dan Nicolaescu]

Adrian Robert <address@hidden> writes:

  > On Jul 16, 2008, at 5:25 AM, Dan Nicolaescu wrote:
  > 
  > 
  >     Adrian Robert <address@hidden> 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 <address@hidden>
  >     +
  >     + * 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.