[PATCH] Bugs in lisp/hl-line.el.

[Lute Kamstra]

Dear People,

Looking at the ChageLogs, I gather that hl-line-mode was originally a
global minor mode and that it was rewritten to pair of minor modes,
hl-line-mode and global-hl-line-mode (one local and one global) by
means of the macros define-minor-mode and
easy-mmode-define-global-mode.

It seems that this transition introduced the following three bugs.

1. The following documentation seem obsolete as the hl-line-mode
   variable is always buffer local (by means of
   make-variable-buffer-local).

,----[ hl-line.el; Commentary ]
| ;; You could make variable `hl-line-mode' buffer-local to avoid
| ;; highlighting specific buffers, when the global mode is used.
`----

2. The function hl-line-mode uses the global variables
   pre-command-hook and post-command-hook.  

,----[ hl-line.el; Code ]
| ;;;###autoload
| (define-minor-mode hl-line-mode
|   "Minor mode to highlight the line about point in the current window.
| With ARG, turn Hl-Line mode on if ARG is positive, off otherwise.
| Uses functions `hl-line-unhighlight' and `hl-line-highlight' on
| `pre-command-hook' and `post-command-hook'."
|   nil nil nil
|   (if hl-line-mode
|       (progn
|       (add-hook 'pre-command-hook #'hl-line-unhighlight)
|       (add-hook 'post-command-hook #'hl-line-highlight))
|     (hl-line-unhighlight)
|     (remove-hook 'pre-command-hook #'hl-line-unhighlight)
|     (remove-hook 'post-command-hook #'hl-line-highlight)))
`----

  Since hl-line-mode is supposed to be buffer local, this is a bug.
  Turning off hl-line-mode in one buffer would effectively disable
  line highlighting in all buffers.  The (un)highlighting functions
  should be in the buffer local command hooks.

3. The function used to highlight a line checks the value of the
   hl-line-mode variable.  

,----[ hl-line.el; Code ]
| (defun hl-line-highlight ()
|   "Active the Hl-Line overlay on the current line in the current window.
| \(Unless it's a minibuffer window.)"
|   (when hl-line-mode                  ; Could be made buffer-local.
|     (unless (window-minibuffer-p (selected-window)) ; silly in minibuffer
|       (unless hl-line-overlay
|       (setq hl-line-overlay (make-overlay 1 1)) ; to be moved
|       (overlay-put hl-line-overlay 'face hl-line-face))
|       (overlay-put hl-line-overlay 'window (selected-window))
|       (move-overlay hl-line-overlay
|                   (line-beginning-position) (1+ (line-end-position))
|                   (current-buffer)))))
`----

  This is strange.  Again, the hl-line-mode variable is always buffer
  local.  Furthermore, hl-line-highlight can expect hl-line-mode to be
  t, since it will not be executed if hl-line-mode is nil.  That is,
  if the value of hl-line-mode variable is changed only by means of
  the command hl-line-mode.  This is documented already.

,----[ C-h v hl-line-mode RET ]
| hl-line-mode's value is nil
| 
| Documentation:
| Non-nil if Hl-Line mode is enabled.
| Use the command `hl-line-mode' to change this variable.
| 
| Defined in `hl-line'.
`----

  I think this test for hl-line-mode can be safely removed.

In addition to these three bugs, I think the following is also a bug.

The command global-hl-line-mode is defined as follows.

,----[ hl-line.el; Code ]
| (easy-mmode-define-global-mode
|  global-hl-line-mode hl-line-mode hl-line-mode
|  :group 'hl-line)
`----

The third argument to the easy-mmode-define-global-mode macro is
supposed to be the function that turns the mode on in every buffer
(documented as such).  However, here it is hl-line-mode, which toggles
the mode.  As a result turning global-hl-line-mode on results in the
toggling of hl-line-mode in all buffers, while turning
global-hl-line-mode off results in turning hl-line-mode off in all
buffers.  This is counter intuitive to me.  I think it's better to let
global-hl-line-mode turn hl-line-mode on unconditionally.

Here is a patch that implements my proposed changes along with a
ChageLog entry.  I've already signed the necessary copyright papers.

Kind regards,

  Lute.


2003-05-03  Lute Kamstra  <address@hidden>

        * hl-line.el: Removed an erroneous comment.
        (hl-line-mode): Use buffer local hooks.
        (turn-on-hl-line): New function.
        (global-hl-line-mode): Use `turn-on-hl-line'.
        (hl-line-highlight): Don't check the `hl-line-mode' variable.


*** lisp/hl-line.el.~1.18.~     Wed Mar 27 11:22:34 2002
--- lisp/hl-line.el     Sat May  3 21:44:18 2003
***************
*** 46,54 ****
  ;; `hl-line-highlight', on `post-command-hook', activates it again
  ;; across the window width.
  
- ;; You could make variable `hl-line-mode' buffer-local to avoid
- ;; highlighting specific buffers, when the global mode is used.
- 
  ;;; Code:
  
  (defgroup hl-line nil
--- 46,51 ----
***************
*** 72,100 ****
    nil nil nil
    (if hl-line-mode
        (progn
!       (add-hook 'pre-command-hook #'hl-line-unhighlight)
!       (add-hook 'post-command-hook #'hl-line-highlight))
      (hl-line-unhighlight)
!     (remove-hook 'pre-command-hook #'hl-line-unhighlight)
!     (remove-hook 'post-command-hook #'hl-line-highlight)))
  
  ;;;###autoload
  (easy-mmode-define-global-mode
!  global-hl-line-mode hl-line-mode hl-line-mode
   :group 'hl-line)
  
  (defun hl-line-highlight ()
    "Active the Hl-Line overlay on the current line in the current window.
  \(Unless it's a minibuffer window.)"
!   (when hl-line-mode                  ; Could be made buffer-local.
!     (unless (window-minibuffer-p (selected-window)) ; silly in minibuffer
!       (unless hl-line-overlay
!       (setq hl-line-overlay (make-overlay 1 1)) ; to be moved
!       (overlay-put hl-line-overlay 'face hl-line-face))
!       (overlay-put hl-line-overlay 'window (selected-window))
!       (move-overlay hl-line-overlay
!                   (line-beginning-position) (1+ (line-end-position))
!                   (current-buffer)))))
  
  (defun hl-line-unhighlight ()
    "Deactivate the Hl-Line overlay on the current line in the current window."
--- 69,100 ----
    nil nil nil
    (if hl-line-mode
        (progn
!       (add-hook 'pre-command-hook #'hl-line-unhighlight nil t)
!       (add-hook 'post-command-hook #'hl-line-highlight nil t))
      (hl-line-unhighlight)
!     (remove-hook 'pre-command-hook #'hl-line-unhighlight t)
!     (remove-hook 'post-command-hook #'hl-line-highlight t)))
! 
! (defun turn-on-hl-line ()
!   "Unconditionally turn on Hl-Line mode."
!   (hl-line-mode 1))
  
  ;;;###autoload
  (easy-mmode-define-global-mode
!  global-hl-line-mode hl-line-mode turn-on-hl-line
   :group 'hl-line)
  
  (defun hl-line-highlight ()
    "Active the Hl-Line overlay on the current line in the current window.
  \(Unless it's a minibuffer window.)"
!   (unless (window-minibuffer-p (selected-window)) ; silly in minibuffer
!     (unless hl-line-overlay
!       (setq hl-line-overlay (make-overlay 1 1)) ; to be moved
!       (overlay-put hl-line-overlay 'face hl-line-face))
!     (overlay-put hl-line-overlay 'window (selected-window))
!     (move-overlay hl-line-overlay
!                   (line-beginning-position) (1+ (line-end-position))
!                   (current-buffer))))
  
  (defun hl-line-unhighlight ()
    "Deactivate the Hl-Line overlay on the current line in the current window."

-- 
Lute Kamstra  <address@hidden>
CWI  department PNA4
Room M233  phone (+31) 20 592 4214
[Echelon material: JPL Vickie Weaver terrorism]