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