[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: |PATCH| describe-minor-mode and describe-minor-mode-from-indicator
From: |
Stefan Monnier |
Subject: |
Re: |PATCH| describe-minor-mode and describe-minor-mode-from-indicator |
Date: |
Mon, 31 Mar 2003 10:51:01 -0500 |
> +(defun describe-minor-mode (minor-mode)
> + "Display documentation of a minor mode given as MINOR-MODE."
> + (interactive (list (intern (completing-read
> + "Minor mode: "
> + (delete nil (mapcar
> + (function (lambda (x)
`function' is not necessary, and it forces too much indendation here.
> + (if (eval (car x))
> + (symbol-name (car
> x)))))
I generally believe that `eval' should be avoided. This is especially
true here since you call `symbol-name' so you already assume that (car x)
is a symbol, so you could just call `symbol-value' instead of `eval'.
But note also that nothing guarantees you that (car x) is bound.
Finally, I think it's perfectly OK (if not preferable) to list all
the minor modes rather than just the currently active ones, so
I'd just use
(delq nil (mapcar (lambda (x) (symbol-name (car x))) minor-mode-alist))
> +(defun describe-minor-mode-from-indicator (indicator)
> + "Display documentation of a minor mode specified by INDICATOR."
> + (interactive (list
> + (completing-read
> + "Minor mode indicator: "
> + (delete nil
> + (mapcar
> + #'(lambda (x)
> + (if (eval (car x))
> + (let ((i (expand-minor-mode-indicator-object
> (cadr x))))
> + (if (and (< 0 (length i))
> + (string= " " (substring i 0 1)))
> + (substring i 1)
> + i))))
> + minor-mode-alist)))))
How about
(delq nil (mapcar (lambda (x)
(let ((i (format-mode-line x)))
(if (> (length i) 0)
(if (eq (aref i 0) ?\ )
(substring i 1) i))))
minor-mode-alist)) ?
> +(defun lookup-minor-mode-from-indicator (indicator)
> + "Return a minor mode symbol from its indicator on the modeline."
> + (if (and (< 0 (length indicator))
> + (not (string= " " (substring indicator 0 1))))
> + (setq indicator (concat " " indicator)))
I'd rather not assume that indicators start with a space.
> +cdr part of a `minor-mode-alist' element(indicator object) is the
> +indicator of minor mode that is in car part. Normally indicator
> +object is a string. However, in some case it is more compound object
> +like cons cell.
Actually, minor-mode-alist is a mode-line-spec.
So we can simply use `formal-mode-line' to interpret it.
> (defvar mode-line-minor-mode-keymap nil "\
> -Keymap to display on major and minor modes.")
> +Keymap to display on minor modes.")
> +
> +(let ((map (make-sparse-keymap)))
> + (define-key map [mode-line mouse-2] 'describe-mode)
> + (setq mode-line-major-mode-keymap map))
I recommend the use of
(defvar foo-map
(let ((map (...)
(define-key ...)
...
map))
I think that the coding cnvention also recommend it. Admittedly, a lot
of Emacs code doesn't use it :-(
In any case, it seems that your patch removes the "mouse-3 on major mode"
behavior that allowed to turn on minor modes by clicking mouse-3
on the major-mode name. I think this is important since it can
often happen that there is no active minor-mode on which to
click mouse-3.
Stefan