Re: Patch to bookmark.el

Re: Patch to bookmark.el

[Karl Fogel]

[-- Attachment #1: Type: text/plain, Size: 4927 bytes --]

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.


[-- Attachment #2: Matthias Meulien's patch to modernize bookmark menu header display, with some revisions by Karl Fogel. --]
[-- Type: text/plain, Size: 3547 bytes --]

=== modified file 'lisp/bookmark.el'
--- lisp/bookmark.el	2011-11-27 04:43:11 +0000
+++ lisp/bookmark.el	2011-12-17 21:32:01 +0000
@@ -127,9 +127,15 @@
   :type 'boolean
   :group 'bookmark)
 
+(defcustom bookmark-bmenu-use-header-line t
+  "Non-nil means to use an immovable header line, as opposed to inline
+text at the top of the buffer."
+  :type 'boolean
+  :group 'bookmark)
 
-(defconst bookmark-bmenu-header-height 2
-  "Number of lines used for the *Bookmark List* header.")
+(defconst bookmark-bmenu-inline-header-height 2
+  "Number of lines used for the *Bookmark List* header
+\(only significant when `bookmark-bmenu-use-header-line' is nil\).")
 
 (defconst bookmark-bmenu-marks-width 2
   "Number of columns (chars) used for the *Bookmark List* marks column,
@@ -1543,7 +1549,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 +1575,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-inline-header-height))
     (if bookmark-bmenu-toggle-filenames
         (bookmark-bmenu-toggle-filenames t))))
 
@@ -1578,7 +1587,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 derived from `buff-menu.el'.
+    (setq header-line-format header)))
 
 (define-derived-mode bookmark-bmenu-mode special-mode "Bookmark Menu"
   "Major mode for editing a list of bookmarks.
@@ -1631,7 +1658,9 @@
     (setq bookmark-bmenu-toggle-filenames nil))
    (t
     (bookmark-bmenu-show-filenames)
-    (setq bookmark-bmenu-toggle-filenames t))))
+    (setq bookmark-bmenu-toggle-filenames t)))
+  (when bookmark-bmenu-use-header-line
+    (bookmark-bmenu-set-header)))
 
 
 (defun bookmark-bmenu-show-filenames (&optional force)
@@ -1696,9 +1725,11 @@
   "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-inline-header-height))
          (goto-char (point-min))
-         (forward-line bookmark-bmenu-header-height))
+         (forward-line bookmark-bmenu-inline-header-height))
         ((and (bolp) (eobp))
          (beginning-of-line 0))))
 

Re: Patch to bookmark.el

[Stefan Monnier]

> CC'ing the emacs-devel@ group.  Assuming it looks good, I'll commit the
> revised version soon.

Please hold the patch until the trunk is re-opened for non-bugfix
commits (some times "early" next year).


        Stefan

Re: Patch to bookmark.el

[Karl Fogel]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> CC'ing the emacs-devel@ group.  Assuming it looks good, I'll commit the
>> revised version soon.
>
>Please hold the patch until the trunk is re-opened for non-bugfix
>commits (some times "early" next year).

Sounds good.

Re: Patch to bookmark.el

[Glenn Morris]

I think this patch looks just big enough to warrant making a copyright
assignment/disclaimer. Is that something you are willing to do? The
process is straightforward.

Re: Patch to bookmark.el

[Matthias Meulien]

Dear Glenn,

> I think this patch looks just big enough to warrant making a copyright
> assignment/disclaimer. Is that something you are willing to do? 

Sure I am.
-- 
Matthias

Re: Patch to bookmark.el

[Glenn Morris]

Matthias Meulien wrote:

>> I think this patch looks just big enough to warrant making a copyright
>> assignment/disclaimer. Is that something you are willing to do? 
>
> Sure I am.

Great, thanks. I have sent you the form off-list.

Re: Patch to bookmark.el

[Karl Fogel]

[-- Attachment #1: Type: text/plain, Size: 82 bytes --]

Reposting the revised patch with a log message this time, to make review
easier:


[-- Attachment #2: Matthias Meulien's patch to modernize bookmark menu header display, revised and with log message by Karl Fogel. --]
[-- Type: text/plain, Size: 4318 bytes --]

[[[
Display the bookmark list header similarly to the buffer list header
(see `list-buffers'), where the default is now an immovable/immutable
header line.

Patch by: Matthias Meulien <orontee {_AT_} gmail.com>
          Karl Fogel <kfogel {_AT_} red-bean.com>

* lisp/bookmark.el
  (bookmark-bmenu-use-header-line): New variable.
  (bookmark-bmenu-inline-header-height): New name for
    `bookmark-bmenu-header-height', to avoid confusion with the code
     for the new immovable header.  All references changed.
  (bookmark-bmenu-set-header): New function.
  (bookmark-bmenu-list, bookmark-bmenu-toggle-filenames):
    Conditionalize header construction accordingly.
  (bookmark-bmenu-ensure-position): Conditionalize the skipping of the
    inline header height.
]]]

=== modified file 'lisp/bookmark.el'
--- lisp/bookmark.el	2011-11-27 04:43:11 +0000
+++ lisp/bookmark.el	2011-12-17 21:32:01 +0000
@@ -127,9 +127,15 @@
   :type 'boolean
   :group 'bookmark)
 
+(defcustom bookmark-bmenu-use-header-line t
+  "Non-nil means to use an immovable header line, as opposed to inline
+text at the top of the buffer."
+  :type 'boolean
+  :group 'bookmark)
 
-(defconst bookmark-bmenu-header-height 2
-  "Number of lines used for the *Bookmark List* header.")
+(defconst bookmark-bmenu-inline-header-height 2
+  "Number of lines used for the *Bookmark List* header
+\(only significant when `bookmark-bmenu-use-header-line' is nil\).")
 
 (defconst bookmark-bmenu-marks-width 2
   "Number of columns (chars) used for the *Bookmark List* marks column,
@@ -1543,7 +1549,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 +1575,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-inline-header-height))
     (if bookmark-bmenu-toggle-filenames
         (bookmark-bmenu-toggle-filenames t))))
 
@@ -1578,7 +1587,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 derived from `buff-menu.el'.
+    (setq header-line-format header)))
 
 (define-derived-mode bookmark-bmenu-mode special-mode "Bookmark Menu"
   "Major mode for editing a list of bookmarks.
@@ -1631,7 +1658,9 @@
     (setq bookmark-bmenu-toggle-filenames nil))
    (t
     (bookmark-bmenu-show-filenames)
-    (setq bookmark-bmenu-toggle-filenames t))))
+    (setq bookmark-bmenu-toggle-filenames t)))
+  (when bookmark-bmenu-use-header-line
+    (bookmark-bmenu-set-header)))
 
 
 (defun bookmark-bmenu-show-filenames (&optional force)
@@ -1696,9 +1725,11 @@
   "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-inline-header-height))
          (goto-char (point-min))
-         (forward-line bookmark-bmenu-header-height))
+         (forward-line bookmark-bmenu-inline-header-height))
         ((and (bolp) (eobp))
          (beginning-of-line 0))))
 

Re: Patch to bookmark.el

[Matthias Meulien]

Dear Karl,

I just tested the patch you sent to emacs-devel@gnu.org and realized it
is not based on the last version I sent to you: After applying your
patch, I still have a 

   (forward-line 2)

in function `bookmark-bmenu-show-filenames'. With this patch, file names
are not shown in the first two lines of the bookmark list (after calling
`bookmark-bmenu-toggle-file-names').

It should be replaced by something like:

   (when bookmark-bmenu-use-header-line
      (bookmark-bmenu-set-header)
     (forward-line bookmark-bmenu-header-height))

By the way, I have also a version of bookmark.el which adds dedicated
faces for buffer names and file paths like in the buffer list. If people
are interested I can send the corresponding patch to emacs-devel...

Thanks for reading,
-- 
Matthias

Re: Patch to bookmark.el

[Karl Fogel]

Matthias Meulien <orontee@gmail.com> writes:
>I just tested the patch you sent to emacs-devel@gnu.org and realized it
>is not based on the last version I sent to you: After applying your

Sorry -- I don't know what happend, but I only ever got one version from
you (as far as I know), which is the one my patch was based on.  But
anyway, see below...

>patch, I still have a 
>
>   (forward-line 2)
>
>in function `bookmark-bmenu-show-filenames'. With this patch, file names
>are not shown in the first two lines of the bookmark list (after calling
>`bookmark-bmenu-toggle-file-names').
>
>It should be replaced by something like:
>
>   (when bookmark-bmenu-use-header-line
>      (bookmark-bmenu-set-header)
>     (forward-line bookmark-bmenu-header-height))
>
>By the way, I have also a version of bookmark.el which adds dedicated
>faces for buffer names and file paths like in the buffer list. If people
>are interested I can send the corresponding patch to emacs-devel...

Sounds good.  For the sake of organization, let's do things this way:

For your original patch, can you please regenerate the entire patch
against the latest bookmark.el...

  http://bzr.savannah.gnu.org/lh/emacs/trunk/annotate/head:/lisp/bookmark.el
  (click the "download file" button)

...and post it here, incorporating tweaks from my revised patch as
appropriate, and including a log message similarly to the one I recently
posted?  Unified diff format ("diff -u") is best.  Then we can do review
right here.

And as for the dedicated-faces patch, yes, thanks, we'd love to see it
posted here too!  Also generated against the latest pristine bookmark.el
and with a log message.  But let's keep the two patches separate, to
make review simpler.

Best,
-Karl

Re: Patch to bookmark.el

[Matthias Meulien]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

> Sorry -- I don't know what happend, but I only ever got one version from
> you (as far as I know), which is the one my patch was based on.

Don't be sorry; I have many troubles with the ethernet card of my laptop
and won't be surprised if the mails I sent to you were not sent at
all...

Here are log files and patches. Note that header-line.patch is the
result of a diff against the latest pristine bookmark.el; But
dedicated-faces.patch was made after header-line.patch was applied to
bookmark.el. Tell me if you are expecting anything else.

Have a nice day,
-- 
Matthias

[-- Attachment #2: header-line.log --]
[-- Type: text/x-log, Size: 897 bytes --]

[[[
Display the bookmark list header similarly to the buffer list header
(see `list-buffers'), where the default is now an immovable/immutable
header line.

Patch by: Matthias Meulien <orontee {_AT_} gmail.com>
          Karl Fogel <kfogel {_AT_} red-bean.com>

* lisp/bookmark.el
  (bookmark-bmenu-use-header-line): New variable.
  (bookmark-bmenu-inline-header-height): New name for
    `bookmark-bmenu-header-height', to avoid confusion with the code
     for the new immovable header.  All references changed.
  (bookmark-bmenu-set-header): New function.
  (bookmark-bmenu-list, bookmark-bmenu-toggle-filenames):
    Conditionalize header construction accordingly.
  (bookmark-bmenu-ensure-position): Conditionalize the skipping of the
    inline header height.
  (bookmark-bmenu-show-filenames, bookmark-bmenu-hide-filenames): Conditionalize
    the skipping of the inline header height.
]]]

[-- Attachment #3: header-line.patch --]
[-- Type: text/x-patch, Size: 4726 bytes --]

diff --git a/bookmark.el b/bookmark.el
index 4d93e01..99816d2 100644
--- a/bookmark.el
+++ b/bookmark.el
@@ -127,9 +127,15 @@ recently set ones come first, oldest ones come last)."
   :type 'boolean
   :group 'bookmark)
 
+(defcustom bookmark-bmenu-use-header-line t
+  "Non-nil means to use an immovable header line, as opposed to inline
+text at the top of the buffer."
+  :type 'boolean
+  :group 'bookmark)
 
-(defconst bookmark-bmenu-header-height 2
-  "Number of lines used for the *Bookmark List* header.")
+(defconst bookmark-bmenu-inline-header-height 2
+  "Number of lines used for the *Bookmark List* header
+\(only significant when `bookmark-bmenu-use-header-line' is nil\).")
 
 (defconst bookmark-bmenu-marks-width 2
   "Number of columns (chars) used for the *Bookmark List* marks column,
@@ -1543,7 +1549,8 @@ deletion, or > if it is flagged for displaying."
       (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 +1575,10 @@ deletion, or > if it is flagged for displaying."
         (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-inline-header-height))
     (if bookmark-bmenu-toggle-filenames
         (bookmark-bmenu-toggle-filenames t))))
 
@@ -1578,7 +1587,25 @@ deletion, or > if it is flagged for displaying."
 ;;;###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 derived from `buff-menu.el'.
+    (setq header-line-format header)))
 
 (define-derived-mode bookmark-bmenu-mode special-mode "Bookmark Menu"
   "Major mode for editing a list of bookmarks.
@@ -1631,7 +1658,9 @@ Optional argument SHOW means show them unconditionally."
     (setq bookmark-bmenu-toggle-filenames nil))
    (t
     (bookmark-bmenu-show-filenames)
-    (setq bookmark-bmenu-toggle-filenames t))))
+    (setq bookmark-bmenu-toggle-filenames t)))
+  (when bookmark-bmenu-use-header-line
+    (bookmark-bmenu-set-header)))
 
 
 (defun bookmark-bmenu-show-filenames (&optional force)
@@ -1644,7 +1673,8 @@ mainly for debugging, and should not be necessary in normal use."
      (save-excursion
        (save-window-excursion
          (goto-char (point-min))
-         (forward-line 2)
+	 (if (not bookmark-bmenu-use-header-line)
+	     (forward-line bookmark-bmenu-inline-header-height))
          (setq bookmark-bmenu-hidden-bookmarks ())
          (let ((inhibit-read-only t))
            (while (< (point) (point-max))
@@ -1672,7 +1702,8 @@ mainly for debugging, and should not be necessary in normal use."
     (with-buffer-modified-unmodified
      (save-excursion
        (goto-char (point-min))
-       (forward-line 2)
+       (if (not bookmark-bmenu-use-header-line)
+	   (forward-line bookmark-bmenu-inline-header-height))
        (setq bookmark-bmenu-hidden-bookmarks
              (nreverse bookmark-bmenu-hidden-bookmarks))
        (let ((inhibit-read-only t))
@@ -1696,9 +1727,11 @@ mainly for debugging, and should not be necessary in normal use."
   "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-inline-header-height))
          (goto-char (point-min))
-         (forward-line bookmark-bmenu-header-height))
+         (forward-line bookmark-bmenu-inline-header-height))
         ((and (bolp) (eobp))
          (beginning-of-line 0))))
 

[-- Attachment #4: dedicated-faces.log --]
[-- Type: text/x-log, Size: 580 bytes --]

[[[
Define a face to highlight bookmark names in bookmark menu buffers,
where the default is a bold face similarly to buffer names in buffer
menu buffers.

Patch by: Matthias Meulien <orontee {_AT_} gmail.com>
          Karl Fogel <kfogel {_AT_} red-bean.com>

* lisp/bookmark.el
  (bookmark-menu-bookmark): New face to highlight bookmark names.
  (bookmark-insert-location): Removes dupplicated text property to conform
    to buffer list (see `list-buffers')
  (bookmark-bmenu-list, bookmark-bmenu-hide-filenames):
    Apply face `bookmark-menu-bookmark' to bookmark names.
]]]

[-- Attachment #5: dedicated-faces.patch --]
[-- Type: text/x-patch, Size: 2159 bytes --]

diff --git a/bookmark.el b/bookmark.el
index 99816d2..d27fd46 100644
--- a/bookmark.el
+++ b/bookmark.el
@@ -157,6 +157,10 @@ following in your `.emacs' file:
   :type 'boolean
   :group 'bookmark)
 
+(defface bookmark-menu-bookmark
+  '((t (:weight bold)))
+  "Face used to highlight bookmark names in bookmark menu buffers."
+  :group 'bookmark)
 
 (defcustom bookmark-menu-length 70
   "Maximum length of a bookmark name displayed on a popup menu."
@@ -1178,18 +1182,7 @@ Optional second arg NO-HISTORY means don't record this in the
 minibuffer history list `bookmark-history'."
   (interactive (list (bookmark-completing-read "Insert bookmark location")))
   (or no-history (bookmark-maybe-historicize-string bookmark-name))
-  (let ((start (point)))
-    (prog1
-	(insert (bookmark-location bookmark-name))
-      (if (display-mouse-p)
-	  (add-text-properties
-	   start
-	   (save-excursion (re-search-backward
-			    "[^ \t]")
-                           (1+ (point)))
-	   '(mouse-face highlight
-	     follow-link t
-	     help-echo "mouse-2: go to this bookmark in other window"))))))
+  (insert (bookmark-location bookmark-name)))
 
 ;;;###autoload
 (defalias 'bookmark-locate 'bookmark-insert-location)
@@ -1569,7 +1562,8 @@ deletion, or > if it is flagged for displaying."
         (when (display-mouse-p)
           (add-text-properties
            (+ bookmark-bmenu-marks-width start) end
-           '(mouse-face highlight
+           '(font-lock-face bookmark-menu-bookmark
+	     mouse-face highlight
              follow-link t
              help-echo "mouse-2: go to this bookmark in other window")))
         (insert "\n")))
@@ -1717,8 +1711,9 @@ mainly for debugging, and should not be necessary in normal use."
              (if (display-mouse-p)
                  (add-text-properties
                   start (point)
-                  '(mouse-face
-                    highlight follow-link t help-echo
+                  '(font-lock-face bookmark-menu-bookmark
+		    mouse-face highlight
+		    follow-link t help-echo
                     "mouse-2: go to this bookmark in other window"))))
            (forward-line 1)))))))
 

Re: Patch to bookmark.el

[Karl Fogel]

Matthias Meulien <orontee@gmail.com> writes:
>Here are log files and patches. Note that header-line.patch is the
>result of a diff against the latest pristine bookmark.el; But
>dedicated-faces.patch was made after header-line.patch was applied to
>bookmark.el. Tell me if you are expecting anything else.

That'll do fine, thank you.  I look forward to reviewing!

More soon,
-Karl

Re: Patch to bookmark.el

[Matthias Meulien]

On 2011/12/21, Karl Fogel wrote:

> (...) That'll do fine, thank you.  I look forward to reviewing!
>
> More soon,
> -Karl

Well, was there a problem with my patch? More than one year later the 
source repository does not include it... And the bookmarks buffer still 
look the old way.

I can update the patch if one explains what trouble there was with the 
previous one.
-- 
Matthias

Re: Patch to bookmark.el

[Karl Fogel]

Matthias Meulien <orontee@gmail.com> writes:
>> (...) That'll do fine, thank you.  I look forward to reviewing!
>>
>> More soon,
>> -Karl
>
>Well, was there a problem with my patch? More than one year later the
>source repository does not include it... And the bookmarks buffer
>still look the old way.
>
>I can update the patch if one explains what trouble there was with the
>previous one.

My bad -- they are applied now; sorry for dropping this ball.

The bookmark menu buffer looks better now, thank you!

-Karl