Re: ruler support in hexl mode

[Stefan Monnier <monnier@iro.umontreal.ca>]

> +(defcustom hexl-follow-line t
> +  "If non-nil then highlight the line address corresponding to point."
> +  :type 'boolean
> +  :group 'hexl)

Any reason why you do not simply use hl-line-mode ?
Or, better yet, let the user select hl-line-mode if he wants it?

> +(defcustom hexl-use-face-on-address-area t
> +  "If non-nil then put `hexl-address-area-face' on adderss area of hexl-mode buffer."
> +  :type 'face
> +  :group 'hexl)
> +(defcustom hexl-use-face-on-ascii-area t
> +  "If non-nil then put `hexl-ascii-area-face' on ascii area of hexl-mode buffer."
> +  :type 'face
> +  :group 'hexl)

You can probably just remove these variables and place the faces on the
`font-lock-face' property, so the user can use M-x global-font-lock-mode
to turn highlighting on/off.

> @@ -89,11 +118,20 @@
>  (defvar hexl-mode-old-isearch-search-fun-function)
>  (defvar hexl-mode-old-require-final-newline)
>  (defvar hexl-mode-old-syntax-table)
> +(defvar hexl-mode-old-header-line-format)
 
>  (defvar hexl-ascii-overlay nil
>    "Overlay used to highlight ASCII element corresponding to current point.")
>  (make-variable-buffer-local 'hexl-ascii-overlay)
 
> +(defvar hexl-line-overlay nil
> +  "Overlay used to highlight the address of line corresponding to current point.")
> +(make-variable-buffer-local 'hexl-line-overlay)
> +
> +(defconst hexl-mode-header-line-format
> +  '(:eval (hexl-mode-ruler))
> +  "`header-line-format' used in hexl mode.")
> +
>  ;; routines
 
>  (put 'hexl-mode 'mode-class 'special)
> @@ -245,8 +283,13 @@
>      (eldoc-remove-command "hexl-save-buffer" 
>  			  "hexl-current-address")
 
> -    (if hexl-follow-ascii (hexl-follow-ascii 1)))
> -  (run-hooks 'hexl-mode-hook))
> +    (make-variable-buffer-local 'hexl-mode-old-header-line-format)
> +    (setq hexl-mode-old-header-line-format header-line-format)

That should be
   (set (make-local-variable 'hexl-mode-old-header-line-format) header-line-format)
`make-variable-buffer-local' should almost never be called from inside
a function.

> +	(while (re-search-forward "^[0-9a-f]\\{8\\}:" nil t)

I'd use `+' rather than `\\{8\\}' because it's shorter, easier to read,
more efficient, and most importantly future-proof (for very large files).

> +	  (put-text-property (match-beginning 0) (match-end 0)
> +			     'face 'hexl-address-area-face)))
> +      (goto-char (point-min))
> +      (when hexl-use-face-on-ascii-area
> +	(while (re-search-forward " \\( .+$\\)" nil t)
> +	  (put-text-property (match-beginning 1) (match-end 1)
> +			     'face 'hexl-ascii-area-face))))

Why do you put the face property on the space before the actual "ascii" data?

> +;; This function is derived from `ruler-mode-ruler' in ruler-mode.el.
> +(defun hexl-mode-ruler ()
> +  "Return a string ruler for hexl mode."
> +  (when hexl-use-ruler
> +    (let* ((fullw (ruler-mode-full-window-width))
> +	   (w     (window-width))
> +	   (m     (window-margins))
> +	   (lsb   (ruler-mode-left-scroll-bar-cols))
> +	   (lf    (ruler-mode-left-fringe-cols))
> +	   (lm    (or (car m) 0))
> +	   (ruler (make-string fullw ?\ ))

We really need to move this out of ruler-mode into frame.el or some other
"global" file.  And to give it a clean interface so its implementation can
be improved later.

> +	   (o     (+ lsb lf lm))
> +	   (x o)
> +	   (highlight (mod (hexl-current-address) 16)))
> +      ;; "87654321"
> +      (do ((i 8 (1- i)))
> +	  ((= i 0))
> +	(aset ruler x (aref (number-to-string i) 0))
> +	(setq x (1+ x)))
> +      ;; "87654321  "
> +      (setq x (+ x 2))			; ": "
> +      ;; "87654321  0011 2233 4455 6677 8899 aabb ccdd eeff"
> +      (do* ((i 0 (1+ i))
> +	    (c (format "%x" i) (format "%x" i)))
> +	  ((= i 16))
> +	(aset ruler x (aref c 0))
> +	(setq x (1+ x))
> +	(aset ruler x (aref c 0))
> +	(setq x (1+ x))
> +	(if (= highlight i)
> +	    (put-text-property (- x 2) x 
> +			       'face 'highlight
> +			       ruler))
> +	(when (= (mod i 2) 1) 
> +	  (aset ruler x ?\ )
> +	  (setq x (1+ x))))
> +      ;; "87654321  0011 2233 4455 6677 8899 aabb ccdd eeff "
> +      (setq x (1+ x))			; " "
> +      ;; "87654321  0011 2233 4455 6677 8899 aabb ccdd eeff  0123456789abcdef"
> +      (do* ((i 0 (1+ i))
> +	    (c (format "%x" i) (format "%x" i)))
> +	  ((= i 16))
> +	(aset ruler x (aref c 0))
> +	(setq x (1+ x))
> +	(if (= highlight i)
> +	    (put-text-property (1- x) x 
> +			       'face 'highlight
> +			       ruler)))
> +      ruler)))

Isn't this always building the exact same string, except for the size
(which really does not need to depend on the window's width), the leading
space (to align it), and the highlighting of the current column?

Couldn't we just do (100% untested code, inspired from buff-menu.el):

  (let ((spaces (+ lsb lf lm))
        (s "87654321  0011 2233 4455 6677 8899 aabb ccdd eeff  0123456789abcdef")
        (pos 0))
    ;; Turn spaces in the header into stretch specs so they work
    ;; regardless of the header-line face.
    (while (string-match "[ \t]+" s pos)
      (setq pos (match-end 0))
      (put-text-property (match-beginning 0) pos 'display
                         ;; Assume fixed-size chars
                         `(space :align-to ,(+ spaces pos))
                         s))
    ;; Highlight the current column.
    (put-text-property (+ 10 (/ (* 5 highlight) 2))
                       (+ 12 (/ (* 5 highlight) 2))
                       'face 'highlight s)
    ;; Add the leading space.
    (concat (propertize (make-string (floor spaces) ? )
                        'display `(space :width ,spaces))
            s))

Note that the use of the display property also allows us to deal with
fractional sizes, although lsb and lf don't take advantage of it yet.
 
Also I wish we could put some more useful info up there, but admittedly,
I can't think of any.


        Stefan