[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#20845: 25.0.50; bookmark.el, handling of fictitious `buffer' propert
From: |
Drew Adams |
Subject: |
bug#20845: 25.0.50; bookmark.el, handling of fictitious `buffer' property |
Date: |
Fri, 5 Jul 2019 10:16:18 -0700 (PDT) |
> > > 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))