Matthias Meulien <orontee@gmail.com> writes:
>I am sending you a patch to `bookmark.el'.
>
>The idea is to display the bookmark list and the buffer list (
>`list-buffers') in a consistent way: The default in the buffer list is
>now to use the immutable header line. 
>
>Do you think it could be integrated to the trunk?

Hi, Matthias.  Your patch's direction was reversed, I think.  I've
inverted it, and converted it to unified diff format ("diff -u"),
against the latest version of bookmark.el which can be found here:

  http://bzr.savannah.gnu.org/lh/emacs/trunk/annotate/head:/lisp/bookmark.el

Below I'll annotate your patch with some comments, and at the end will
attach a revised patch for review by you and by anyone else -- I'm
CC'ing the emacs-devel@ group.  Assuming it looks good, I'll commit the
revised version soon.

  === modified file 'lisp/bookmark.el'
  --- lisp/bookmark.el	2011-11-27 04:43:11 +0000
  +++ lisp/bookmark.el	2011-12-17 21:01:27 +0000
  @@ -127,6 +127,10 @@
     :type 'boolean
     :group 'bookmark)
   
  +(defcustom bookmark-bmenu-use-header-line t
  +  "Non-nil means to use an immovable header-line."
  +  :type 'boolean
  +  :group 'bookmark)

This is modeled on `Buffer-menu-use-header-line' in buff-menu.el, right?
I think the doc string should say something about how this immovable
header would be used instead of the plain text header.  

The doc string for `bookmark-bmenu-header-height' should be updated to
mention which type of header that variable refers to.
   
   (defconst bookmark-bmenu-header-height 2
     "Number of lines used for the *Bookmark List* header.")
  @@ -1543,7 +1547,8 @@
         (set-buffer buf)))
     (let ((inhibit-read-only t))
       (erase-buffer)
  -    (insert "% Bookmark\n- --------\n")
  +    (if (not bookmark-bmenu-use-header-line)
  +      (insert "% Bookmark\n- --------\n"))
       (add-text-properties (point-min) (point)
   			 '(font-lock-face bookmark-menu-heading))
       (dolist (full-record (bookmark-maybe-sort-alist))
  @@ -1568,8 +1573,10 @@
           (insert "\n")))
       (set-buffer-modified-p (not (= bookmark-alist-modification-count 0)))
       (goto-char (point-min))
  -    (forward-line 2)
       (bookmark-bmenu-mode)
  +    (if bookmark-bmenu-use-header-line
  +	(bookmark-bmenu-set-header)
  +      (forward-line bookmark-bmenu-header-height))
       (if bookmark-bmenu-toggle-filenames
           (bookmark-bmenu-toggle-filenames t))))

Looks good.
   
  @@ -1578,7 +1585,25 @@
   ;;;###autoload
   (defalias 'edit-bookmarks 'bookmark-bmenu-list)
   
  -
  +(defun bookmark-bmenu-set-header ()
  +  "Sets the immutable header line."
  +  (let ((header (concat "%% " "Bookmark")))
  +    (when bookmark-bmenu-toggle-filenames 
  +      (setq header (concat header 
  +			   (make-string (- bookmark-bmenu-file-column 
  +					   (- (length header) 3))  ?\s)
  +			   "File")))
  +    (let ((pos 0))
  +      (while (string-match "[ \t\n]+" header pos)
  +	(setq pos (match-end 0))
  +	(put-text-property (match-beginning 0) pos 'display
  +			   (list 'space :align-to (- pos 1))
  +			   header)))
  +    (put-text-property 0 2 'face 'fixed-pitch header)
  +    (setq header (concat (propertize " " 'display '(space :align-to 0))
  +			 header))
  +    ;; Code stollen to `buff-menu.el' --Matthias
  +    (setq header-line-format header)))

I adjusted the comment to say "Code derived from `buff-menu.el'."
   
   (define-derived-mode bookmark-bmenu-mode special-mode "Bookmark Menu"
     "Major mode for editing a list of bookmarks.
  @@ -1631,7 +1656,8 @@
       (setq bookmark-bmenu-toggle-filenames nil))
      (t
       (bookmark-bmenu-show-filenames)
  -    (setq bookmark-bmenu-toggle-filenames t))))
  +    (setq bookmark-bmenu-toggle-filenames t)))
  +  (bookmark-bmenu-set-header))
   
(This is in `bookmark-bmenu-toggle-filenames'.)  There's no conditional
around the call to `bookmark-bmenu-set-header', so what would happen if
`bookmark-bmenu-use-header-line' were nil?  I can tell you the answer,
because I just tested it -- we get *both* headers :-).

I've wrapped it in `(when bookmark-bmenu-use-header-line ...)' to solve
the problem.
   
   (defun bookmark-bmenu-show-filenames (&optional force)
  @@ -1696,7 +1722,8 @@
     "If point is not on a bookmark line, move it to one.
   If before the first bookmark line, move to the first; if after the
   last full line, move to the last full line.  The return value is undefined."
  -  (cond ((< (count-lines (point-min) (point)) bookmark-bmenu-header-height)
  +  (cond ((and (not bookmark-bmenu-use-header-line)
  +	      (< (count-lines (point-min) (point)) bookmark-bmenu-header-height))
            (goto-char (point-min))
            (forward-line bookmark-bmenu-header-height))
           ((and (bolp) (eobp))
  
Looks good, except for the long line, which reformatted to stay under 80
columns.

Revised patch attached, in unidiff format.