Re: |PATCH| describe-minor-mode and describe-minor-mode-from-indicator

["Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu>]

> +(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