doc-view support for bookmark.el

doc-view support for bookmark.el

[Tassilo Horn]

Hi all,

on emacs-sources there was a request for a bookmarking facility for
doc-view, so I added support for it in bookmark.el.

When I was doing that I found some strange code: bookmark-jump-noselect
opened the bookmarked file, set point at the correct position and
returned a cons (BUFFER . POSITION).  All calling functions did set the
position once again.  I suspect that was a relict of the past, so I
removed it and now bookmark-jump-noselect only returns the buffer.

Till now I didn't use bookmarks, so I'm not sure if this change will
break something.  So bookmark-users, please test it.  (For me it works
nicely, but I only tested simple setting and jumping.)

If nobody complains in some days or some people report that it works for
them, I'll install the change.

--8<---------------cut here---------------start------------->8---
2007-12-25  Tassilo Horn  <tassilo@member.fsf.org>

	* bookmark.el (bookmark-jump-noselect): Return only the bookmark
	buffer.
	(bookmark-insert, bookmark-bmenu-2-window)
	(bookmark-bmenu-other-window, bookmark-jump)
	(bookmark-jump-other-window, bookmark-bmenu-switch-other-window):
	Adapt to that.
	(bookmark-doc-view-p, bookmark-get-doc-view-page)
	(bookmark-set-doc-view-page): New functions.
	(bookmark-make-cell, bookmark-jump-noselect): Support
	doc-view-buffers.
--8<---------------cut here---------------end--------------->8---

Here's the patch:

--8<---------------cut here---------------start------------->8---
Index: lisp/bookmark.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/bookmark.el,v
retrieving revision 1.98
diff -u -r1.98 bookmark.el
--- lisp/bookmark.el	25 Sep 2007 10:43:39 -0000	1.98
+++ lisp/bookmark.el	25 Dec 2007 10:50:49 -0000
@@ -526,19 +526,22 @@
 INFO-NODE, so record this fact in the bookmark's entry."
   (let ((the-record
          `((filename . ,(bookmark-buffer-file-name))
-           (front-context-string
-            . ,(if (>= (- (point-max) (point)) bookmark-search-size)
-                   (buffer-substring-no-properties
-                    (point)
-                    (+ (point) bookmark-search-size))
-                   nil))
-           (rear-context-string
-            . ,(if (>= (- (point) (point-min)) bookmark-search-size)
-                   (buffer-substring-no-properties
-                    (point)
-                    (- (point) bookmark-search-size))
-                   nil))
-           (position . ,(point)))))
+	   ,(if (bookmark-doc-view-p)
+		;; For doc-view buffers only the page is of interest.
+		`(doc-view-page . ,doc-view-current-page)
+	      `(front-context-string
+		. ,(if (>= (- (point-max) (point)) bookmark-search-size)
+		       (buffer-substring-no-properties
+			(point)
+			(+ (point) bookmark-search-size))
+		     nil))
+	      `(rear-context-string
+		. ,(if (>= (- (point) (point-min)) bookmark-search-size)
+		       (buffer-substring-no-properties
+			(point)
+			(- (point) bookmark-search-size))
+		     nil))
+	      `(position . ,(point))))))
 
     ;; Now fill in the optional parts:
 
@@ -1068,10 +1071,9 @@
   (unless bookmark
     (error "No bookmark specified"))
   (bookmark-maybe-historicize-string bookmark)
-  (let ((cell (bookmark-jump-noselect bookmark)))
-    (and cell
-         (switch-to-buffer (car cell))
-         (goto-char (cdr cell))
+  (let ((buf (bookmark-jump-noselect bookmark)))
+    (and buf
+         (switch-to-buffer buf)
 	 (progn (run-hooks 'bookmark-after-jump-hook) t)
 	 (if bookmark-automatically-show-annotations
              ;; if there is an annotation for this bookmark,
@@ -1090,10 +1092,9 @@
          (list bkm) bkm)))
   (when bookmark
     (bookmark-maybe-historicize-string bookmark)
-    (let ((cell (bookmark-jump-noselect bookmark)))
-      (and cell
-           (switch-to-buffer-other-window (car cell))
-           (goto-char (cdr cell))
+    (let ((buf (bookmark-jump-noselect bookmark)))
+      (and buf
+           (switch-to-buffer-other-window buf)
            (if bookmark-automatically-show-annotations
                ;; if there is an annotation for this bookmark,
                ;; show it in a buffer.
@@ -1123,12 +1124,13 @@
 
 (defun bookmark-jump-noselect (str)
   ;; a leetle helper for bookmark-jump :-)
-  ;; returns (BUFFER . POINT)
+  ;; returns the BUFFER showing the bookmark
   (bookmark-maybe-load-default-file)
   (let* ((file (expand-file-name (bookmark-get-filename str)))
          (forward-str            (bookmark-get-front-context-string str))
          (behind-str             (bookmark-get-rear-context-string str))
          (place                  (bookmark-get-position str))
+	 (doc-view-page          (bookmark-get-doc-view-page str))
          (info-node              (bookmark-get-info-node str))
          (orig-file              file)
          )
@@ -1143,7 +1145,15 @@
 		    (Info-find-node file info-node)))
               ;; Else no Info.  Can do an ordinary find-file:
               (set-buffer (find-file-noselect file))
-              (goto-char place))
+	      (if (not doc-view-page)
+		  ;; ordinary text file
+		  (goto-char place)
+		;; a document that should be viewed with doc-view
+		(when (not (eq major-mode 'doc-view-mode))
+		  ;; d-v-m's not activated, so we're probably in ps-mode and
+		  ;; need to toggle display.
+		  (doc-view-toggle-display))
+		(doc-view-goto-page doc-view-page)))
 
             ;; Go searching forward first.  Then, if forward-str exists and
             ;; was found in the file, we can search backward for behind-str.
@@ -1158,7 +1168,7 @@
                     (goto-char (match-end 0))))
             ;; added by db
             (setq bookmark-current-bookmark str)
-            (cons (current-buffer) (point))))
+            (current-buffer)))
 
       ;; Else unable to find the marked file, so ask if user wants to
       ;; relocate the bookmark, else remind them to consider deletion.
@@ -1173,7 +1183,7 @@
             (bookmark-jump-noselect str))
         (message
          "Bookmark not relocated; consider removing it \(%s\)." str)
-        nil))))
+	nil))))
 
 
 ;;;###autoload
@@ -1275,7 +1285,7 @@
   (let ((orig-point (point))
         (str-to-insert
          (save-excursion
-           (set-buffer (car (bookmark-jump-noselect bookmark)))
+           (set-buffer (bookmark-jump-noselect bookmark))
            (buffer-string))))
     (insert str-to-insert)
     (push-mark)
@@ -1904,11 +1914,7 @@
             (pop-up-windows t))
         (delete-other-windows)
         (switch-to-buffer (other-buffer))
-	(let* ((pair (bookmark-jump-noselect bmrk))
-               (buff (car pair))
-               (pos  (cdr pair)))
-          (pop-to-buffer buff)
-          (goto-char pos))
+	(pop-to-buffer (bookmark-jump-noselect bmrk))
         (bury-buffer menu))))
 
 
@@ -1923,14 +1929,9 @@
   "Select this line's bookmark in other window, leaving bookmark menu visible."
   (interactive)
   (let ((bookmark (bookmark-bmenu-bookmark)))
-    (if (bookmark-bmenu-check-position)
-	(let* ((pair (bookmark-jump-noselect bookmark))
-               (buff (car pair))
-               (pos  (cdr pair)))
-	  (switch-to-buffer-other-window buff)
-          (goto-char pos)
-          (set-window-point (get-buffer-window buff) pos)
-	  (bookmark-show-annotation bookmark)))))
+    (when (bookmark-bmenu-check-position)
+      (switch-to-buffer-other-window (bookmark-jump-noselect bookmark))
+      (bookmark-show-annotation bookmark))))
 
 
 (defun bookmark-bmenu-switch-other-window ()
@@ -1941,18 +1942,9 @@
         (pop-up-windows t)
         same-window-buffer-names
         same-window-regexps)
-    (if (bookmark-bmenu-check-position)
-	(let* ((pair (bookmark-jump-noselect bookmark))
-               (buff (car pair))
-               (pos  (cdr pair)))
-	  (display-buffer buff)
-          (let ((o-buffer (current-buffer)))
-            ;; save-excursion won't do
-            (set-buffer buff)
-            (goto-char pos)
-            (set-window-point (get-buffer-window buff) pos)
-            (set-buffer o-buffer))
-	  (bookmark-show-annotation bookmark)))))
+    (when (bookmark-bmenu-check-position)
+      (display-buffer (bookmark-jump-noselect bookmark))
+      (bookmark-show-annotation bookmark))))
 
 (defun bookmark-bmenu-other-window-with-mouse (event)
   "Select bookmark at the mouse pointer in other window, leaving bookmark menu visible."
@@ -2107,6 +2099,27 @@
         (goto-char thispoint))))
 
 \f
+
+;;; Doc-view support functions
+
+(defun bookmark-doc-view-p ()
+  "Return non-nil if the current buffer in which the bookmark
+should be set uses `doc-view-mode'."
+  (eq major-mode 'doc-view-mode))
+
+(defun bookmark-get-doc-view-page (bookmark)
+  "Return the page \(i.e.: doc-view-page\) of BOOKMARK."
+  (cdr (assq 'doc-view-page (bookmark-get-bookmark-record bookmark))))
+
+(defun bookmark-set-doc-view-page (bookmark page)
+  "Set the page \(i.e.: doc-view-page\) of BOOKMARK to PAGE."
+  (let ((cell (assq 'doc-view-page (bookmark-get-bookmark-record bookmark))))
+    (if cell
+        (setcdr cell page)
+      (nconc (bookmark-get-bookmark-record bookmark)
+             (list (cons 'doc-view-page page))))))
+
+
 ;;; Menu bar stuff.  Prefix is "bookmark-menu".
 
 (defun bookmark-menu-popup-paned-menu (event name entries)
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

Re: doc-view support for bookmark.el

[Stefan Monnier]

> on emacs-sources there was a request for a bookmarking facility for
> doc-view, so I added support for it in bookmark.el.

Could we try and find a way to make the two packages independent:
bookmark.el should provide a way for doc-view.el to override the behavior
of bookmark commands in doc-view buffers.


        Stefan

Re: doc-view support for bookmark.el

[Tassilo Horn]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> on emacs-sources there was a request for a bookmarking facility for
>> doc-view, so I added support for it in bookmark.el.
>
> Could we try and find a way to make the two packages independent:
> bookmark.el should provide a way for doc-view.el to override the
> behavior of bookmark commands in doc-view buffers.

Sure, a general interface would be a good idea.  Do you propose an one?

A simple approach could be to add 2 new variables

  - bookmark-make-cell-function: A function that creates the record part
    of the bookmark (default: the current bookmark-make-cell after
    renaming it appropriately)

  - bookmark-jump-function: A function that jumps to the given bookmark
    location without selecting it. (default: the current
    bookmark-jump-noselect)

What do you think?

And what about the other changes?  (Return value of
bookmark-jump-noselect and the adaption of its callers.)  If it's ok
I'll check it in now and omit the doc-view stuff till we agree on
general interface.

Bye,
Tassilo

Re: doc-view support for bookmark.el

[Stefan Monnier]

>>> on emacs-sources there was a request for a bookmarking facility for
>>> doc-view, so I added support for it in bookmark.el.
>> 
>> Could we try and find a way to make the two packages independent:
>> bookmark.el should provide a way for doc-view.el to override the
>> behavior of bookmark commands in doc-view buffers.

> Sure, a general interface would be a good idea.  Do you propose an one?

> A simple approach could be to add 2 new variables

>   - bookmark-make-cell-function: A function that creates the record part
>     of the bookmark (default: the current bookmark-make-cell after
>     renaming it appropriately)

>   - bookmark-jump-function: A function that jumps to the given bookmark
>     location without selecting it. (default: the current
>     bookmark-jump-noselect)

> What do you think?

I don't have time to look into it right now, but I'd have expected that
the second one is not needed: doc-view's bookmark-make-cell-function
would create a bookmark entry which contains a callback to doc-view, so
that bookmark-jump would not need a bookmark-jump-function, or in other
words the bookmark-jump-function shouldn't be a global (buffer-local)
variable, but an (optional) entry in each bookmark.


        Stefan

Re: doc-view support for bookmark.el

[Tassilo Horn]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> Sure, a general interface would be a good idea.  Do you propose an
>> one?
>
>> A simple approach could be to add 2 new variables
>
>>   - bookmark-make-cell-function: A function that creates the record part
>>     of the bookmark (default: the current bookmark-make-cell after
>>     renaming it appropriately)
>
>>   - bookmark-jump-function: A function that jumps to the given bookmark
>>     location without selecting it. (default: the current
>>     bookmark-jump-noselect)
>
>> What do you think?
>
> I don't have time to look into it right now, but I'd have expected that
> the second one is not needed: doc-view's bookmark-make-cell-function
> would create a bookmark entry which contains a callback to doc-view, so
> that bookmark-jump would not need a bookmark-jump-function, or in other
> words the bookmark-jump-function shouldn't be a global (buffer-local)
> variable, but an (optional) entry in each bookmark.

That's a very good idea.  I'll try to implement it when I find some
time.

Bye,
Tassilo

Re: doc-view support for bookmark.el

[Tassilo Horn]

Hi Stefan,

here's the implementation of your idea and doc-view using it.

--8<---------------cut here---------------start------------->8---
2007-12-25  Tassilo Horn  <tassilo@member.fsf.org>

	* bookmark.el (bookmark-make-cell-function): New variable.
	(bookmark-make): Call bookmark-make-cell-function's function
	instead of bookmark-make-cell.
	(bookmark-get-handler): New function.
	(bookmark-jump, bookmark-jump-other-window, bookmark-insert)
	(bookmark-bmenu-2-window, bookmark-bmenu-other-window): Use the
	bookmark's handler function if it has one defined.
	(bookmark-make-cell-for-text-file): Renamed from
	bookmark-make-cell.

	* doc-view.el (doc-view-bookmark-make-cell)
	(doc-view-bookmark-jump): New functions.
	(doc-view-mode): Set bookmark-make-cell-function buffer-locally.
--8<---------------cut here---------------end--------------->8---

Here's the patch:

--8<---------------cut here---------------start------------->8---
Index: lisp/doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.26
diff -u -r1.26 doc-view.el
--- lisp/doc-view.el	6 Dec 2007 15:04:29 -0000	1.26
+++ lisp/doc-view.el	25 Dec 2007 21:51:49 -0000
@@ -958,6 +958,8 @@
     (set (make-local-variable 'cursor-type) nil)
     (use-local-map doc-view-mode-map)
     (set (make-local-variable 'after-revert-hook) 'doc-view-reconvert-doc)
+    (set (make-local-variable 'bookmark-make-cell-function)
+			      'doc-view-bookmark-make-cell)
     (setq mode-name "DocView"
 	  buffer-read-only t
 	  major-mode 'doc-view-mode)
@@ -996,4 +998,32 @@
 ;; End:
 
 ;; arch-tag: 5d6e5c5e-095f-489e-b4e4-1ca90a7d79be
+;;;; Bookmark integration
+
+(defun doc-view-bookmark-make-cell (annotation &rest args)
+  (let ((the-record
+         `((filename . ,(buffer-file-name))
+           (page     . ,doc-view-current-page)
+           (handler  . doc-view-bookmark-jump))))
+
+    ;; Take no chances with text properties
+    (set-text-properties 0 (length annotation) nil annotation)
+
+    (when annotation
+      (nconc the-record (list (cons 'annotation annotation))))
+
+    ;; Finally, return the completed record.
+    the-record))
+
+;;;###autoload
+(defun doc-view-bookmark-jump (bmk)
+  (save-window-excursion
+    (let ((filename (bookmark-get-filename bmk))
+	  (page (cdr (assq 'page (bookmark-get-bookmark-record bookmark)))))
+      (find-file filename)
+      (when (not (eq major-mode 'doc-view-mode))
+	(doc-view-toggle-display))
+      (doc-view-goto-page page)
+      (cons (current-buffer) 1))))
+
 ;;; doc-view.el ends here
Index: lisp/bookmark.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/bookmark.el,v
retrieving revision 1.98
diff -u -r1.98 bookmark.el
--- lisp/bookmark.el	25 Sep 2007 10:43:39 -0000	1.98
+++ lisp/bookmark.el	25 Dec 2007 21:51:50 -0000
@@ -443,6 +443,8 @@
   (message "%S" (assq 'info-node (bookmark-get-bookmark-record bookmark)))
   (sit-for 4))
 
+(defun bookmark-get-handler (bookmark)
+  (cdr (assq 'handler (bookmark-get-bookmark-record bookmark))))
 
 (defvar bookmark-history nil
   "The history list for bookmark functions.")
@@ -480,6 +482,22 @@
     (interactive-p)
     (setq bookmark-history (cons ,string bookmark-history))))
 
+(defvar bookmark-make-cell-function 'bookmark-make-cell-for-text-file
+  "A function that should be called to create the bookmark
+record.  Modes may set this variable buffer-locally to enable
+bookmarking of non-text files like images or pdf documents.
+
+The function will be called with two arguments: ANNOTATION and
+INFO-NODE.  See `bookmark-make-cell-for-text-file' for a
+description.
+
+The returned record may contain a special cons (handler
+. some-function) which sets the handler function that should be
+used to open this bookmark instead of `bookmark-jump-noselect'.
+It should return a cons (BUFFER . POINT) indicating buffer
+showing the bookmarked location and the value of point in that
+buffer.  Like `bookmark-jump-noselect' the buffer shouldn't be
+selected by the handler.")
 
 (defun bookmark-make (name &optional annotation overwrite info-node)
   "Make a bookmark named NAME.
@@ -498,7 +516,7 @@
         ;; already existing bookmark under that name and
         ;; no prefix arg means just overwrite old bookmark
         (setcdr (bookmark-get-bookmark stripped-name)
-                (list (bookmark-make-cell annotation info-node)))
+                (list (funcall bookmark-make-cell-function annotation info-node)))
 
       ;; otherwise just cons it onto the front (either the bookmark
       ;; doesn't exist already, or there is no prefix arg.  In either
@@ -507,7 +525,7 @@
       (setq bookmark-alist
             (cons
              (list stripped-name
-                   (bookmark-make-cell annotation info-node))
+                   (funcall bookmark-make-cell-function annotation info-node))
              bookmark-alist)))
 
     ;; Added by db
@@ -518,7 +536,7 @@
         (bookmark-save))))
 
 
-(defun bookmark-make-cell (annotation &optional info-node)
+(defun bookmark-make-cell-for-text-file (annotation &optional info-node)
   "Return the record part of a new bookmark, given ANNOTATION.
 Must be at the correct position in the buffer in which the bookmark is
 being set.  This might change someday.
@@ -1068,7 +1086,10 @@
   (unless bookmark
     (error "No bookmark specified"))
   (bookmark-maybe-historicize-string bookmark)
-  (let ((cell (bookmark-jump-noselect bookmark)))
+  (let* ((handler (bookmark-get-handler bookmark))
+	 (cell (if handler
+		   (funcall handler bookmark)
+		 (bookmark-jump-noselect bookmark))))
     (and cell
          (switch-to-buffer (car cell))
          (goto-char (cdr cell))
@@ -1090,7 +1111,10 @@
          (list bkm) bkm)))
   (when bookmark
     (bookmark-maybe-historicize-string bookmark)
-    (let ((cell (bookmark-jump-noselect bookmark)))
+    (let* ((handler (bookmark-get-handler bookmark))
+	   (cell (if handler
+		     (funcall handler bookmark)
+		   (bookmark-jump-noselect bookmark))))
       (and cell
            (switch-to-buffer-other-window (car cell))
            (goto-char (cdr cell))
@@ -1272,10 +1296,14 @@
   (interactive (list (bookmark-completing-read "Insert bookmark contents")))
   (bookmark-maybe-historicize-string bookmark)
   (bookmark-maybe-load-default-file)
-  (let ((orig-point (point))
+  (let* ((orig-point (point))
+	 (handler (bookmark-get-handler bookmark))
+	 (cell (if handler
+		   (funcall handler bookmark)
+		 (bookmark-jump-noselect bookmark)))
         (str-to-insert
          (save-excursion
-           (set-buffer (car (bookmark-jump-noselect bookmark)))
+           (set-buffer (car cell))
            (buffer-string))))
     (insert str-to-insert)
     (push-mark)
@@ -1904,7 +1932,10 @@
             (pop-up-windows t))
         (delete-other-windows)
         (switch-to-buffer (other-buffer))
-	(let* ((pair (bookmark-jump-noselect bmrk))
+	(let* ((handler (bookmark-get-handler bookmark))
+	       (pair (if handler
+			 (funcall handler bookmark)
+		       (bookmark-jump-noselect bookmark)))
                (buff (car pair))
                (pos  (cdr pair)))
           (pop-to-buffer buff)
@@ -1924,7 +1955,10 @@
   (interactive)
   (let ((bookmark (bookmark-bmenu-bookmark)))
     (if (bookmark-bmenu-check-position)
-	(let* ((pair (bookmark-jump-noselect bookmark))
+	(let* ((handler (bookmark-get-handler bookmark))
+	       (pair (if handler
+			 (funcall handler bookmark)
+		       (bookmark-jump-noselect bookmark)))
                (buff (car pair))
                (pos  (cdr pair)))
 	  (switch-to-buffer-other-window buff)
@@ -1942,7 +1976,10 @@
         same-window-buffer-names
         same-window-regexps)
     (if (bookmark-bmenu-check-position)
-	(let* ((pair (bookmark-jump-noselect bookmark))
+	(let* ((handler (bookmark-get-handler bookmark))
+	       (pair (if handler
+			 (funcall handler bookmark)
+		       (bookmark-jump-noselect bookmark)))
                (buff (car pair))
                (pos  (cdr pair)))
 	  (display-buffer buff)
--8<---------------cut here---------------end--------------->8---

Do you think it's ok and I should install it?

I've tested it with normal text files, a pdf and a ps file and it works
very nice.

Bye,
Tassilo

Re: doc-view support for bookmark.el

[Nick Roberts]

 > here's the implementation of your idea and doc-view using it.
 > 
 > --8<---------------cut here---------------start------------->8---
 > 2007-12-25  Tassilo Horn  <tassilo@member.fsf.org>
 > 
 > 	* bookmark.el (bookmark-make-cell-function): New variable.
 > 	(bookmark-make): Call bookmark-make-cell-function's function
 > 	instead of bookmark-make-cell.
 > 	(bookmark-get-handler): New function.
 > 	(bookmark-jump, bookmark-jump-other-window, bookmark-insert)
 > 	(bookmark-bmenu-2-window, bookmark-bmenu-other-window): Use the
 > 	bookmark's handler function if it has one defined.
 > 	(bookmark-make-cell-for-text-file): Renamed from
 > 	bookmark-make-cell.
 > 
 > 	* doc-view.el (doc-view-bookmark-make-cell)
 > 	(doc-view-bookmark-jump): New functions.
 > 	(doc-view-mode): Set bookmark-make-cell-function buffer-locally.
 > --8<---------------cut here---------------end--------------->8---
 > 
 > Here's the patch:
 > 
 > --8<---------------cut here---------------start------------->8---
 > Index: lisp/doc-view.el
 > ===================================================================
 > RCS file: /sources/emacs/emacs/lisp/doc-view.el,v


As an aside, if you attach your patch at the end of your e-mail, the diff
should apply cleanly without the need for "cut here" markers.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: doc-view support for bookmark.el

[Tassilo Horn]

Nick Roberts <nickrob@snap.net.nz> writes:

Hi Nick,

> As an aside, if you attach your patch at the end of your e-mail, the
> diff should apply cleanly without the need for "cut here" markers.

Ok, noted.  But nobody has to apply it for me, I can do that on my own.
I just wanted to get some feedback before installing it.

Bye,
Tassilo

Re: doc-view support for bookmark.el

[Stefan Monnier]

> +	  (if handler
> +		   (funcall handler bookmark)
> +		 (bookmark-jump-noselect bookmark))))

Aka (funcall (or handler 'bookmark-jump-noselect) bookmark)

> Do you think it's ok and I should install it?

Looks good from a cursory glance at it, although the repetition of all those
bookmark-get-handler + if + funcall is begging for some redundancy-reduction.


        Stefan

Re: doc-view support for bookmark.el

[Tassilo Horn]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> Do you think it's ok and I should install it?
>
> Looks good from a cursory glance at it, although the repetition of all
> those bookmark-get-handler + if + funcall is begging for some
> redundancy-reduction.

Yes, that came into my mind shortly after I've sent the patch.  I'll
extract that into it's own function and install it then.

Bye,
Tassilo

bookmark support for doc-view and image-mode (was: doc-view support for bookmark.el)

[Tassilo Horn]

Tassilo Horn <tassilo@member.fsf.org> writes:

>> Looks good from a cursory glance at it, although the repetition of
>> all those bookmark-get-handler + if + funcall is begging for some
>> redundancy-reduction.
>
> Yes, that came into my mind shortly after I've sent the patch.  I'll
> extract that into it's own function and install it then.

Done that, and I added support for bookmarking images (text or displayed
form) to image-mode.el.

Bye,
Tassilo

Re: doc-view support for bookmark.el

[Karl Fogel]

I like your new bookmark abstractions, Tassilo!  A few comments:

Tassilo Horn <tassilo@member.fsf.org> writes:
> --- lisp/bookmark.el	25 Sep 2007 10:43:39 -0000	1.98
> +++ lisp/bookmark.el	25 Dec 2007 21:51:50 -0000
> @@ -480,6 +482,22 @@
>      (interactive-p)
>      (setq bookmark-history (cons ,string bookmark-history))))
>  
> +(defvar bookmark-make-cell-function 'bookmark-make-cell-for-text-file
> +  "A function that should be called to create the bookmark
> +record.  Modes may set this variable buffer-locally to enable
> +bookmarking of non-text files like images or pdf documents.
> +
> +The function will be called with two arguments: ANNOTATION and
> +INFO-NODE.  See `bookmark-make-cell-for-text-file' for a
> +description.
> +
> +The returned record may contain a special cons (handler
> +. some-function) which sets the handler function that should be
> +used to open this bookmark instead of `bookmark-jump-noselect'.
> +It should return a cons (BUFFER . POINT) indicating buffer
> +showing the bookmarked location and the value of point in that
> +buffer.  Like `bookmark-jump-noselect' the buffer shouldn't be
> +selected by the handler.")

Okay, so "(BUFFER . POINT)" is now an exported interface, not just an
internal interface that we would be free to change inside bookmark.el.
Therefore, let's make it more extensible than a dotted pair.  The
cleanest thing would probably be an association list:

   '((buffer BUFFER) (point POINT))

That way, callers will be compatible even if we extend the type later:

   '((buffer BUFFER) (point POINT) ...)

And a comment regarding documentation:

Normally, a function type's documentation should all be in one place,
I think, not referring out to specific instances.  So normally, I
would suggest replacing this...

   The function will be called with two arguments: ANNOTATION and
   INFO-NODE.  See `bookmark-make-cell-for-text-file' for a
   description.

... with full documentation of the INFO-NODE argument.  But in this
case, the better thing might be to implement
`bookmark-make-cell-for-info-node' as a separate instance of
`bookmark-make-cell-function', adjusting everything else in the
obvious way.

Thoughts?

-Karl

RE: doc-view support for bookmark.el

[Drew Adams]

> Okay, so "(BUFFER . POINT)" is now an exported interface, not just an
> internal interface that we would be free to change inside bookmark.el.
> Therefore, let's make it more extensible than a dotted pair.

Why would "we would be free to change inside bookmark.el" before Tassilo's
change? There is nothing internal about `bookmark-jump-noselect'.

And, as I already pointed out, there already exists external code that calls
it and depends on the (BUFFER . POINT) interface.

Of course, nothing stops Emacs developers from changing anything, but please
don't assume that (BUFFER . POINT) is an "internal" representation that can
be changed with no external consequences.

Re: doc-view support for bookmark.el

[Karl Fogel]

"Drew Adams" <drew.adams@oracle.com> writes:
>> Okay, so "(BUFFER . POINT)" is now an exported interface, not just an
>> internal interface that we would be free to change inside bookmark.el.
>> Therefore, let's make it more extensible than a dotted pair.
>
> Why would "we would be free to change inside bookmark.el" before Tassilo's
> change? There is nothing internal about `bookmark-jump-noselect'.
>
> And, as I already pointed out, there already exists external code that calls
> it and depends on the (BUFFER . POINT) interface.
>
> Of course, nothing stops Emacs developers from changing anything, but please
> don't assume that (BUFFER . POINT) is an "internal" representation that can
> be changed with no external consequences.

Sorry, I missed where you mentioned that, but your point stands.

So, in that case, what I'm really advocating is: let's change it to
return a *better* representation, and update all the callers in Emacs.
(I doubt there are many callers outside the Emacs distribution, but
even if there are, it's still worth going through this pain once so we
have an extensible type from now on.)

-Karl

Re: doc-view support for bookmark.el

[Tassilo Horn]

Karl Fogel <kfogel@red-bean.com> writes:

Hi Karl,

> I like your new bookmark abstractions, Tassilo!

:-)

> A few comments:
>
> Tassilo Horn <tassilo@member.fsf.org> writes:
>> --- lisp/bookmark.el	25 Sep 2007 10:43:39 -0000	1.98
>> +++ lisp/bookmark.el	25 Dec 2007 21:51:50 -0000
>> @@ -480,6 +482,22 @@
>>      (interactive-p)
>>      (setq bookmark-history (cons ,string bookmark-history))))
>>  
>> +(defvar bookmark-make-cell-function 'bookmark-make-cell-for-text-file
>> +  "A function that should be called to create the bookmark
>> +record.  Modes may set this variable buffer-locally to enable
>> +bookmarking of non-text files like images or pdf documents.
>> +
>> +The function will be called with two arguments: ANNOTATION and
>> +INFO-NODE.  See `bookmark-make-cell-for-text-file' for a
>> +description.
>> +
>> +The returned record may contain a special cons (handler
>> +. some-function) which sets the handler function that should be
>> +used to open this bookmark instead of `bookmark-jump-noselect'.
>> +It should return a cons (BUFFER . POINT) indicating buffer
>> +showing the bookmarked location and the value of point in that
>> +buffer.  Like `bookmark-jump-noselect' the buffer shouldn't be
>> +selected by the handler.")
>
> Okay, so "(BUFFER . POINT)" is now an exported interface, not just an
> internal interface that we would be free to change inside bookmark.el.
> Therefore, let's make it more extensible than a dotted pair.  The
> cleanest thing would probably be an association list:
>
>    '((buffer BUFFER) (point POINT))
>
> That way, callers will be compatible even if we extend the type later:
>
>    '((buffer BUFFER) (point POINT) ...)

Yes, that seems to be a good idea.  And since it's a trivial change to
adapt current callers, I don't see any reason not to do it.  (Drew, I
hope we can still be friends. ;-))

> And a comment regarding documentation:
>
> Normally, a function type's documentation should all be in one place,
> I think, not referring out to specific instances.  So normally, I
> would suggest replacing this...
>
>    The function will be called with two arguments: ANNOTATION and
>    INFO-NODE.  See `bookmark-make-cell-for-text-file' for a
>    description.
>
> ... with full documentation of the INFO-NODE argument.  But in this
> case, the better thing might be to implement
> `bookmark-make-cell-for-info-node' as a separate instance of
> `bookmark-make-cell-function', adjusting everything else in the
> obvious way.

Definitely.  I didn't want to document INFO-NODE, because it hardly
makes any sense for anything but bookmarks in info docs.  Basically the
yet to be done `bookmark-make-cell-for-info-node' can use
`b-m-c-f-text-file' and install only another handler
(bookmark-jump-info), so that `bookmark-jump-noselect' can be split into
`bookmark-jump-text' and `bookmark-jump-info'.  Then the INFO-NODE arg
could be dropped.

I'm not sure when I find some time to work on that.  Xmas was a good
excuse not to work on my diploma thesis, but I mustn't put it off any
longer.  So if sombody wants to do it, go for it.

Bye,
Tassilo

RE: doc-view support for bookmark.el

[Drew Adams]

> Yes, that seems to be a good idea.  And since it's a trivial change to
> adapt current callers, I don't see any reason not to do it.  (Drew, I
> hope we can still be friends. ;-))

;-)

Sure, I have no problem using the new (better) representation. Just wanted
to point out that the representation is not only internal and there exists
at least one external use of it.

Re: doc-view support for bookmark.el

[Karl Fogel]

Tassilo Horn <tassilo@member.fsf.org> writes:
> Definitely.  I didn't want to document INFO-NODE, because it hardly
> makes any sense for anything but bookmarks in info docs.  Basically the
> yet to be done `bookmark-make-cell-for-info-node' can use
> `b-m-c-f-text-file' and install only another handler
> (bookmark-jump-info), so that `bookmark-jump-noselect' can be split into
> `bookmark-jump-text' and `bookmark-jump-info'.  Then the INFO-NODE arg
> could be dropped.
>
> I'm not sure when I find some time to work on that.  Xmas was a good
> excuse not to work on my diploma thesis, but I mustn't put it off any
> longer.  So if sombody wants to do it, go for it.

Okay.  I'll try to do it when I get a chance, but if someone wants to
beat me to it, be my guest! :-)

-Karl

Re: doc-view support for bookmark.el

[Kim F. Storm]

Karl Fogel <kfogel@red-bean.com> writes:

> cleanest thing would probably be an association list:
>
>    '((buffer BUFFER) (point POINT))
>

If you decide to change this, please use a plist, as in

   '(:buffer BUFFER :point POINT ...)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: doc-view support for bookmark.el

[Karl Fogel]

storm@cua.dk (Kim F. Storm) writes:
> Karl Fogel <kfogel@red-bean.com> writes:
>
>> cleanest thing would probably be an association list:
>>
>>    '((buffer BUFFER) (point POINT))
>>
>
> If you decide to change this, please use a plist, as in
>
>    '(:buffer BUFFER :point POINT ...)

*nod* ("ACK" :-) )

-Karl

Re: doc-view support for bookmark.el

[Stefan Monnier]

>> cleanest thing would probably be an association list:
>> 
>> '((buffer BUFFER) (point POINT))
>> 

> If you decide to change this, please use a plist, as in

>    '(:buffer BUFFER :point POINT ...)

Why?


        Stefan "who generally prefers alists"

Re: doc-view support for bookmark.el

[Kim F. Storm]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> cleanest thing would probably be an association list:
>>> 
>>> '((buffer BUFFER) (point POINT))
>>> 
>
>> If you decide to change this, please use a plist, as in
>
>>    '(:buffer BUFFER :point POINT ...)
>
> Why?

Alists certainly are more generally useful and versatile, but for this
kind of values, I find plists + keywords much simpler to use and
understand than alists.

E.g. it's trivial to make such a value:

     (list :buffer (current-buffer) :point (point))

However, if you decide an alist is better, shouldn't it be:

 '((buffer . BUFFER) (point . POINT))

using conses rather than lists as values ?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: doc-view support for bookmark.el

[Stefan Monnier]

>>>> cleanest thing would probably be an association list:
>>>> 
>>>> '((buffer BUFFER) (point POINT))
>>>> 
>> 
>>> If you decide to change this, please use a plist, as in
>> 
>>> '(:buffer BUFFER :point POINT ...)
>> 
>> Why?

> Alists certainly are more generally useful and versatile, but for this
> kind of values, I find plists + keywords much simpler to use and
> understand than alists.

> E.g. it's trivial to make such a value:

>      (list :buffer (current-buffer) :point (point))

> However, if you decide an alist is better, shouldn't it be:

>  '((buffer . BUFFER) (point . POINT))

> using conses rather than lists as values ?

Yes, but the lists here are generated by Lisp code, not hard-coded, so
the extra " . " and the extra parentheses don't matter.

I consider plists to be an unfortunate accident in Lisp's life.


        Stefan

Re: doc-view support for bookmark.el

[Kim F. Storm]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>>> cleanest thing would probably be an association list:
>>>>> 
>>>>> '((buffer BUFFER) (point POINT))

>> However, if you decide an alist is better, shouldn't it be:
>
>>  '((buffer . BUFFER) (point . POINT))
>
>> using conses rather than lists as values ?
>
> Yes, but the lists here are generated by Lisp code, not hard-coded, so
> the extra " . " and the extra parentheses don't matter.

Huh?

   (equal '((a . b) (c . d)) '((a b) (c d)))
   => nil


> I consider plists to be an unfortunate accident in Lisp's life.

Let's just agree to disagree :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: doc-view support for bookmark.el

[Stefan Monnier]

>>>>> cleanest thing would probably be an association list:
>>>>> 
>>>>> '((buffer BUFFER) (point POINT))

>>> However, if you decide an alist is better, shouldn't it be:
>> 
>>> '((buffer . BUFFER) (point . POINT))
>> 
>>> using conses rather than lists as values ?
>> 
>> Yes, but the lists here are generated by Lisp code, not hard-coded, so
>> the extra " . " and the extra parentheses don't matter.

> Huh?

>    (equal '((a . b) (c . d)) '((a b) (c d)))
>    => nil

No, I didn't mean that.  I meant that it doesn't matter because you only
write the code once, so the few extra chars (which may actually look
like "(cons" rather than " . ") don't matter.


        Stefan

RE: doc-view support for bookmark.el

[Drew Adams]

> When I was doing that I found some strange code: bookmark-jump-noselect
> opened the bookmarked file, set point at the correct position and
> returned a cons (BUFFER . POSITION).  All calling functions did set the
> position once again.  I suspect that was a relict of the past, so I
> removed it and now bookmark-jump-noselect only returns the buffer.

`bookmark-jump' expects the return value of `bookmark-jump-noselect' to be
what it has always been, a "cell" that provides both the buffer and the
position. I define my own `bookmark-jump-other-window' analogously, so it
has the same expectation:

(defun bookmark-jump-other-window (bookmark)
  "Jump to BOOKMARK (a point in some file) in another window.
See `bookmark-jump'."
  (interactive (list (bookmark-completing-read
                       "Jump to bookmark (in another window)"
                       bookmark-current-bookmark)))
  (when bookmark
    (bookmark-maybe-historicize-string bookmark)
    (let ((cell (bookmark-jump-noselect bookmark)))
      (and cell
           (switch-to-buffer-other-window (car cell))
           (goto-char (cdr cell))
           (if bookmark-automatically-show-annotations
               ;; if there is an annotation for this bookmark,
               ;; show it in a buffer.
               (bookmark-show-annotation bookmark))))))

> Till now I didn't use bookmarks, so I'm not sure if this change will
> break something.

See above.

Re: doc-view support for bookmark.el

[martin rudalics]

 > When I was doing that I found some strange code: bookmark-jump-noselect
 > opened the bookmarked file, set point at the correct position and
 > returned a cons (BUFFER . POSITION).  All calling functions did set the
 > position once again.  I suspect that was a relict of the past, so I
 > removed it and now bookmark-jump-noselect only returns the buffer.

(defun bookmark-jump-noselect (str)
    ...
         (save-excursion
             ...
             (cons (current-buffer) (point))))

The position might be in the same buffer where the excursion occurred,
hence this is needed.

Re: doc-view support for bookmark.el

[Tassilo Horn]

martin rudalics <rudalics@gmx.at> writes:

Hi Martin,

>> When I was doing that I found some strange code:
>> bookmark-jump-noselect opened the bookmarked file, set point at the
>> correct position and returned a cons (BUFFER . POSITION).  All
>> calling functions did set the position once again.  I suspect that
>> was a relict of the past, so I removed it and now
>> bookmark-jump-noselect only returns the buffer.
>
> (defun bookmark-jump-noselect (str)
>    ...
>         (save-excursion
>             ...
>             (cons (current-buffer) (point))))
>
> The position might be in the same buffer where the excursion occurred,
> hence this is needed.

Ok, I get it.  But it's at least a bit ugly and hard-wired to bookmarks
that actually save the value of point.  We should come up with a better
way that allows bookmarking images, pdfs and everything else emacs can
display.  At least for those two examples there's no need to save the
value of point.

Bye,
Tassilo