bug#20845: 25.0.50; bookmark.el, handling of fictitious `buffer' property

[Drew Adams <drew.adams@oracle.com>]

> > > I'm looking into Emacs Bug#20845 which asks about the purpose of the
> > > "bookmark" property in bookmark.el.
> >
> > I don't see any `bookmark` property, sorry.
> 
> That should be "buffer", sorry about that.

That typo at the beginning of the report was my fault.
I meant `buffer' property, not `bookmark' property.

> > Oh, I see how/where it's used and it's definitely still used:
> > Info-bookmark-jump uses it (i.e. not in the stored bookmark object, but
> > in a transient object passed to bookmark-default-handler).

Yes, thanks for pointing that out.

> > So maybe it should be documented in the docstring of
> > `bookmark-default-handler` for the benefit of other handlers.
> 
> I agree with this conclusion.  So not only do we keep it, but we document
> it.
> 
> Thanks for helping out Stefan, it seems we now have a way forward.

I agree, provided the doc of `bookmark-default-handler'
makes clear (1) that the value is assumed to be a
buffer, not a buffer name, and (2) that this is not a
"normal" bookmark property, that is, one whose value
can be saved.

It's OK for a jump function to pass a buffer object as
the property value to the default handler.  But it's
generally not OK for code to try to store a buffer
value for property `buffer'.  Such a bookmark works OK
in a live `bookmark-alist', but it isn't persistable.

That the default handler recognizes this property,
which cannot be persisted, is a bit aberrant.

It's normal for mode-setting, narrowing, etc. to be
coded in a jump function.  What's not so normal is
to communicate the buffer to the default bookmark
handler as a bookmark property.

It should be sufficient to communicate the buffer
_name_ as the property value.  And that's something
savable and retrievable - it's useful generally.

IOW, it's OK to have `buffer' bookmark property,
but I think its value should be a buffer name.

Something like this might be better than what we
have now (tested only mildly, using vanilla Emacs
26.2):

(defun Info-bookmark-make-record ()
  ""
  (let* ((file (and (stringp Info-current-file)
                    (file-name-sans-extension
                     (file-name-nondirectory Info-current-file))))
         (bookmark-name (if file
                            (concat "(" file ") " Info-current-node)
                          Info-current-node))
         (defaults (delq nil (list bookmark-name file Info-current-node))))
    `(,bookmark-name
      ,@(bookmark-make-record-default 'no-file)
      (filename . ,Info-current-file)
      (info-node . ,Info-current-node)
      (buffer . ,(buffer-name))         ; <=======================
      (handler . Info-bookmark-jump)
      (defaults . ,defaults))))

(defun Info-bookmark-jump (bmk)
  ""
  (let* ((file  (bookmark-prop-get bmk 'filename))
         (node  (bookmark-prop-get bmk 'info-node)))
    (Info-find-node file node)
    ;; Use `bookmark-default-handler' to move to location in node.
    (bookmark-default-handler
     (cons "" (bookmark-get-bookmark-record bmk))))) <===== no BUF

(defun bookmark-default-handler (bmk-record)
  ""
  (let ((file          (bookmark-get-filename bmk-record))
        ;; Get buffer from recorded buffer name. <================
        (buf           (get-buffer (bookmark-prop-get bmk-record 'buffer)))
        (forward-str   (bookmark-get-front-context-string bmk-record))
        (behind-str    (bookmark-get-rear-context-string bmk-record))
        (place         (bookmark-get-position bmk-record)))
    (set-buffer
     (cond
      ((and file (file-readable-p file) (not (buffer-live-p buf)))
       (find-file-noselect file))
      (buf)
      (t (signal 'bookmark-error-no-filename (list 'stringp file)))))
    (if place (goto-char place))
    (when (and forward-str (search-forward forward-str (point-max) t))
      (goto-char (match-beginning 0)))
    (when (and behind-str (search-backward behind-str (point-min) t))
      (goto-char (match-end 0)))
    nil))