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

[Dan Nicolaescu <dann@ics.uci.edu>]

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.