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

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

[Drew Adams]

Function `bookmark-default-handler' picks up and handles the property
(field) named `bookmark', if present.

I see nowhere else where this property is used, and nowhere where it is
set.  And it is not documented.

IOW, there is no notion or existence (AFAICT) of any bookmarks that
record a `buffer' property.  And yet, the *default* handler handles
this unknown property - in two ways:

* If the file is readable and is not visited then it visits the file
  using `find-file-noselect'.  Why?

* Otherwise, if the buffer named by property `buffer' exists then the
  default handler does nothing (no-op).  The (non-English) comment here
  is "See if buffer BUF have been created.", which tells us nothing
  about why it is handled by doing nothing.

This partial "handling" of the undocumented and never-set property
`buffer' was apparently introduced in Emacs 23.1.  As it never did
anything, it was seemingly never noticed.

This property should either be properly documented (in the doc string of
`bookmark-alist'), as to its use and meaning, or its default "handling"
should be removed, as misguided.  I'd suggest that the unclear handling
should just be removed.

If there is some existing code distributed with Emacs that actually uses
this property (and I can find none) then a bug can be filed for it.

A grep of the Emacs sources for `bookmark-prop-set' shows no occurrences
of that function outside bookmark.el, and that function is never used to
set property `bookmark'.  I have also checked all occurrences of
`bookmark-make-record' in the Emacs Lisp sources, and found none that
set or use property `bookmark'.

AFAICT, this property does not exist, and its incomplete "handling"
should be removed.

On the other hand, if this handling was just added to bookmark.el by
someone only as support for some 3rd-party code, then it doesn't belong
here anyway - that 3rd-party code can advise `bookmark-default-handler'
or simply define its own, non-default, handlers.


In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-10-20 on LEG570
Bzr revision: 118168 rgm@gnu.org-20141020195941-icp42t8ttcnud09g
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking=yes,glyphs CPPFLAGS=-DGLYPH_DEBUG=1'

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

[Stefan Kangas]

Hi Stefan,

I'm looking into Emacs Bug#20845 which asks about the purpose of the
"bookmark" property in bookmark.el.  I'm confused as to what its
purpose is, as is Drew in the bug report quoted below.

Drew Adams <drew.adams@oracle.com> writes:

> Function `bookmark-default-handler' picks up and handles the property
> (field) named `bookmark', if present.
>
> I see nowhere else where this property is used, and nowhere where it is
> set.  And it is not documented.
>
> IOW, there is no notion or existence (AFAICT) of any bookmarks that
> record a `buffer' property.  And yet, the *default* handler handles
> this unknown property - in two ways:
>
> * If the file is readable and is not visited then it visits the file
>   using `find-file-noselect'.  Why?
>
> * Otherwise, if the buffer named by property `buffer' exists then the
>   default handler does nothing (no-op).  The (non-English) comment here
>   is "See if buffer BUF have been created.", which tells us nothing
>   about why it is handled by doing nothing.
>
> This partial "handling" of the undocumented and never-set property
> `buffer' was apparently introduced in Emacs 23.1.  As it never did
> anything, it was seemingly never noticed.
>
> This property should either be properly documented (in the doc string of
> `bookmark-alist'), as to its use and meaning, or its default "handling"
> should be removed, as misguided.  I'd suggest that the unclear handling
> should just be removed.
>
> If there is some existing code distributed with Emacs that actually uses
> this property (and I can find none) then a bug can be filed for it.
>
> A grep of the Emacs sources for `bookmark-prop-set' shows no occurrences
> of that function outside bookmark.el, and that function is never used to
> set property `bookmark'.  I have also checked all occurrences of
> `bookmark-make-record' in the Emacs Lisp sources, and found none that
> set or use property `bookmark'.
>
> AFAICT, this property does not exist, and its incomplete "handling"
> should be removed.
>
> On the other hand, if this handling was just added to bookmark.el by
> someone only as support for some 3rd-party code, then it doesn't belong
> here anyway - that 3rd-party code can advise `bookmark-default-handler'
> or simply define its own, non-default, handlers.

This handling was added in this commit:

    commit dbf8402bc76a775284905f09399b4d88ee0c03e5
    Author: Stefan Monnier <monnier@iro.umontreal.ca>
    Date:   Wed Feb 10 15:02:54 2010 -0500

        (bookmark-handle-bookmark): Catch the right error.
        (bookmark-default-handler): Accept new bookmark field `buffer'.

This was authored by Thierry Volpiatto according to ChangeLog:

    +2010-02-10  Thierry Volpiatto  <thierry.volpiatto@gmail.com>

But trying to find out why this was committed made me even more
confused.  I could find Stefan Monnier arguing against this change on
two different occasions:

    2010-01-25 - https://debbugs.gnu.org/cgi/bugreport.cgi?bug=5476#14
    2010-01-26 - https://debbugs.gnu.org/cgi/bugreport.cgi?bug=5476#20

Yet this was committed on 2010-02-10.  I was unable to find any
discussion that would explain what had changed.

I understand this was a long time ago, but if you still remember it,
do you think you could help shed some light into the purpose of this
code?

The suggestion given by Drew above is to remove it.  Do you have an
opinion about that?

Thanks in advance!

Best regards,
Stefan Kangas

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

[Stefan Monnier]

> 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.

>> Function `bookmark-default-handler' picks up and handles the property
>> (field) named `bookmark', if present.

Where?

>> I see nowhere else where this property is used, and nowhere where it is
>> set.  And it is not documented.

Sounds like the description of leftover code.  Usually it's either
because of an oversight or for backward compatibility purposes.

>> IOW, there is no notion or existence (AFAICT) of any bookmarks that
>> record a `buffer' property.

Which property are we talking about, `buffer` or `bookmark`?

>> * If the file is readable and is not visited then it visits the file
>>   using `find-file-noselect'.  Why?

I'll assume this question was the result of a temporary confusion on
Drew's part, since I think it's pretty obvious why we'd want to visit
the file here.

>> * Otherwise, if the buffer named by property `buffer' exists then the
>>   default handler does nothing (no-op).

No: it selects that buffer as the current buffer.

> This handling was added in this commit:
>
>     commit dbf8402bc76a775284905f09399b4d88ee0c03e5
>     Author: Stefan Monnier <monnier@iro.umontreal.ca>
>     Date:   Wed Feb 10 15:02:54 2010 -0500
>
>         (bookmark-handle-bookmark): Catch the right error.
>         (bookmark-default-handler): Accept new bookmark field `buffer'.

And it had been earlier removed by:

    commit 13901bcbc4926630bdb2127301af0cdf7bcc50f7
    Author: Karl Fogel <kfogel@red-bean.com>
    Date:   Mon Oct 5 01:35:34 2009 +0000

so my commit re-instated support for it.

> But trying to find out why this was committed made me even more
> confused.  I could find Stefan Monnier arguing against this change on
> two different occasions:
>
>     2010-01-25 - https://debbugs.gnu.org/cgi/bugreport.cgi?bug=5476#14
>     2010-01-26 - https://debbugs.gnu.org/cgi/bugreport.cgi?bug=5476#20

Good digging, thanks.

> I understand this was a long time ago, but if you still remember it,
> do you think you could help shed some light into the purpose of this
> code?

I can't remember why I accepted to install it, but these discussions
seem to indicate that the purpose for the patch was to preserve
compatibility with old bookmarks that don't use their own
bookmark handler.

> The suggestion given by Drew above is to remove it.
> Do you have an opinion about that?

Not really, no.  But such "old-style" bookmarks might still be used by
third-party packages and might still appear in users's
~/.emacs.d/bookmarks, so if we want to remove support for it, maybe we
should try and do it gradually.

[...taking a second look...]

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).
So maybe it should be documented in the docstring of
`bookmark-default-handler` for the benefit of other handlers.


        Stefan

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

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> > 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.

> > The suggestion given by Drew above is to remove it.
> > Do you have an opinion about that?
>
> Not really, no.  But such "old-style" bookmarks might still be used by
> third-party packages and might still appear in users's
> ~/.emacs.d/bookmarks, so if we want to remove support for it, maybe we
> should try and do it gradually.
>
> [...taking a second look...]
>
> 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).
> 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.

Best regards,
Stefan Kangas

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

[Drew Adams]

> > > 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))

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

[Noam Postavsky]

Drew Adams <drew.adams@oracle.com> writes:

> 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.

Would that potentially be a problem if the buffer is renamed?

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

[Drew Adams]

> > 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.
> 
> Would that potentially be a problem if the buffer is renamed?

Yes. But it's not much different from the problem
that arises if a file name is changed.

And no different from the case where a buffer
(not a buffer name) no longer exists (which can
happen even if a buffer object is used in a live,
not persisted, `bookmark-alist').

We can try to jump through hoops for this.  It's
a choice how many hoops we want to jump through.

We could `condition-case'-handle the case where
`bookmark-default-handler' is used, and try to
deal with errors such as those.  We already do
that for condition `bookmark-error-no-filename'.

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

[Stefan Monnier]

> 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,

The current code accepts both and that seems quite reasonable to me
(even though I dislike this same fact in many other places).

> and (2) that this is not a "normal" bookmark property, that is, one
> whose value can be saved.

There's no readable printed representation of a buffer object, so that
goes without saying.


        Stefan

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

[Drew Adams]

> > 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,
> 
> The current code accepts both and that seems quite reasonable to me
> (even though I dislike this same fact in many other places).

Yes, right.

> > and (2) that this is not a "normal" bookmark property, that is, one
> > whose value can be saved.
> 
> There's no readable printed representation of a buffer object, so that
> goes without saying.

I don't think it goes without saying, here.
I don't know why we wouldn't want to mention this.

I know of no other possibility of a bookmark that
does not have a print/read representation, and so
can't be persisted.  Persisting bookmarks is a
pretty fundamental part of Emacs bookmarking.

(Even the temporary bookmarks you can create with
Bookmark+ have a print/read representation, so
you can save them if you want.)

Of course, just because I don't know of other
such unsavable bookmarks doesn't mean they aren't
created by some library somewhere.

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

[Lars Ingebrigtsen]

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

> 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).
> So maybe it should be documented in the docstring of
> `bookmark-default-handler` for the benefit of other handlers.

I've now done thing in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Drew Adams]

> I've now done thing in Emacs 29.

Can you please say what "thing" you've done?

I guess you've documented this bookmark
property/field (?).

But have you also documented that this is an
_exceptional_ bookmark property, in that (1)
it can be added ephemerally on the fly by a
bookmark jump function, to pass along a buffer
object, but (2) it can't be persisted as part
of a saved bookmark.

1. It's important that the intended use of
this field be doc'd.

2. It's important that users understand that
they can't create a bookmark with a buffer
(as opposed to a buffer name) as the value of
such a property, and then save that bookmark.

#1 guides you wrt something you can do.
#2 advises you not to do something that will
break your bookmark file.