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 <address@hidden>
(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