[PATCH] Let bookmark handlers do everything, including display the destination

[Drew Adams]

tl;dr:

Let's change the handling of a bookmark - including
displaying the bookmark destination - to be the
responsibility of its handler.

___

Attached is a patch that does that.  It moves the displaying
of a bookmark's "destination" from `bookmark-jump' to the
bookmark itself - specifically, to its handler.

Currently `bookmark-jump', rather than a bookmark's handler,
displays the bookmark destination.  And the behavior's
hard-coded: a bookmark always displays a destination, and
that's always the last thing done before the after-jump
hook.

That's too rigid.  It means that a handler function can't do
something more after the display, unless it finagles to put
that after-display processing on `bookmark-after-jump-hook'
(temporarily).  And it means that a handler can't choose its
own display function - the DISPLAY-FUNCTION passed to
`bookmark-jump' is always used.  And it means that a handler
can't dispense with display altogether.

By moving the responsibility for display to the handler, the
patch gets rid of this rigidity.

Specifically, instead of `bookmark--jump-via' systematically
invoking its DISPLAY-FUNCTION argument, it saves the
argument value in variable `bookmark-jump-display-function'.
And then the default handler, `bookmark-default-handler'
invokes that function.

Most bookmarks don't have their own handler; they use the
default handler.  So their behavior doesn't change at all
with this approach.  Most bookmarks use `bookmark-jump's
DISPLAY FUNCTION as the last thing before running
`bookmark-after-jump-hook' (and most should do that).

The same applies to bookmarks that do have their own
handler, if the handler invokes the default handler at the
end.  For example, the Info bookmark handler,
`Info-bookmark-jump' sets up what it needs, and ends by
invoking the default handler:

  (bookmark-default-handler
   `("" (buffer . ,buf)
     . ,(bookmark-get-bookmark-record bookmark)))

What changes with the proposed approach is only what can
happen for a bookmark that has its own handler, if that
handler either (1) invokes the default handler at some point
other than at the end - so it does something after
displaying, or (2) it doesn't invoke the default handler.

If a handler doesn't invoke the default handler, it can call
whatever display function it likes, whenever it likes.  Or
it can forego display altogether.

This approach gives the handler more control - it can decide
whether, when, and how to display the destination.  The
entire behavior of a bookmark is then up to its handler.

And this allows for more kinds of bookmarks, including
bookmarks that don't display a destination.

For example, Bookmark+, which uses this approach, predefines
several kinds of bookmarks with handlers that either provide
their own kind of display or don't display anything.  This
includes bookmarks that do these things:

 . Combine a sequence of other bookmarks (composition)
 . Invoke a function
 . Open `*Bookmark List*' for a set of bookmarks, a sort
   order, etc.
 . Switch to a different bookmark file or load another
   bookmark file
 . Copy a snippet to the kill-ring
 . Switch to a desktop
 . Restore a set of keyboard macros
 . Restore a set of defvars

Other Bookmark+ predefined bookmark types have handlers that
do invoke `bookmark-default-handler' (with a definition
similar to that in the patch, invoking DISPLAY-FUNCTION),
which is the typical case.

Though Bookmark+ handling is different from that of vanilla
Emacs (it's essentially that of the patch), this difference
isn't a problem, in general - nearly all vanilla bookmarks
work with Bookmark+.

It is a problem in a few cases, however.  There's at least
one 3rd-party library, PDF Tools, whose handler doesn't make
use of the default handler, and it expects that
`bookmark--jump-via' will display the destination after
handling.  There may be a few others; dunno.

PDF Tools lives here: https://github.com/vedang/pdf-tools/.
It was formerly here: https://github.com/politza/pdf-tools/.
Both have the same code for the handler,
`pdf-view-bookmark-jump-handler'.

That does some finagling that puts a function on the
after-jump hook and then uses `set-buffer'.  It does nothing
to display the buffer - it counts on `bookmark-jump' to
display it after the handler ends.  The rest of the
handling, after display, is done on the after-jump hook.
   
The after-jump hook function depends on the buffer already
being displayed.  And it just uses the buffer of the
currently `selected-window' if the destination buffer isn't
displayed.

See example #1 in attachment `pdf-tools-handler-fix.el'.

As a result, such a bookmark doesn't work with Bookmark+.
E.g., if invoked in the window of buffer `*Bookmark List*'
it just uses that instead of the intended PDF-file buffer,
which is no good.

For it to work with Bookmark+, the handler needs to display
the intended buffer.  E.g., instead of `set-buffer' it needs
to call a function such as `pop-to-buffer-same-window'.
With such an edit the bookmark works with Bookmark+ (and
with vanilla Emacs with the patch).

See example #2 in attachment `pdf-tools-handler-fix.el'.

Another way to fix the handler is for it to call the default
handler.

See example #3 in attachment `pdf-tools-handler-fix.el'.

Yet another way to fix it is to advise the handler, to have
it invoke the default handler at the end:

  (defun my-pdf-handler-advice (bookmark)
    (bookmark-default-handler
      (bookmark-get-bookmark bookmark)))

  (advice-add 'pdf-view-bookmark-jump-handler
              :after 'my-pdf-handler-advice)

That's example #4 in attachment `pdf-tools-handler-fix.el'.

But really, the complications `pdf-view-bookmark-handler'
goes through - creating a closure on the fly and adding it
to the after-jump hook (and subsequently removing it) - are
a workaround for the fact that vanilla `bookmark.el' doesn't
let a handler display the destination when it wants to.

PDF Tools wants to set up the buffer, display it, and then
go to the right PDF page etc.  So it sets up the buffer,
ends the handler, counts on `bookmark-jump' to display the
buffer, and then has a (temporary) after-jump hook do the
rest of the handling: go to the right page etc.  (The hook
function captures some info from the bookmark, so it needs
to be a closure.)  A clever workaround.

With the approach provided by the patch, the handler can do
all of that itself: set up the buffer, display it, then go
to the page etc.  No after-jump-hook workaround.  The code
is quite a bit simpler.

Between setting up the buffer and doing the additional stuff
that was relegated to a temporary after-jump hook, the
handler can either (1) just invoke the default handler or
(2) just call a display function.  Code for each of those
handler definitions is also attached, for comparison with
the above code.

Those solutions are examples #5 and #6, respectively, in
attachment `pdf-tools-handler-fix.el'.

I mention the PDF Tools handler as an example.  I'm not
aware of other handlers that depend on `bookmark-jump'
displaying some buffer and selecting its window, and then
depend on the after-jump hook for additional handling in
that buffer, but there may be some out there.

This is a general problem for Bookmark+, even if it seems to
be run into rarely because most handlers invoke the default
handler (which in Bookmark+ displays the destination using
the DISPLAY-FUNCTION passed to `bookmark-jump').

But this is not just about getting rid of an incompatibility
with Bookmark+'s bookmark handling.  It's also about
improving vanilla `bookmark.el', by letting the handler
handle a bookmark's behavior - all of it.  Give handlers the
control over display, and have the default handler invoke
the `bookmark-jump' DISPLAY-FUNCTION arg.

That makes possible bookmarks that don't necessarily display
a destination - they perform some other action.  And it
allows bookmarks to do any kind of displaying whatsoever
(not just what the `bookmark-jump' DISPLAY-FUNCTION
provides).

Regardless of whether we adopt the proposed behavior, we
might anyway want to post a guideline to let people who
define custom bookmark handlers know that it's
common/typical for a handler to invoke the default handler -
that repositions the bookmark as needed etc.

But with the new behavior we should also make clear that for
a bookmark destination to be displayed, a custom handler
needs to invoke either `bookmark-default-handler' or a
display function.

More generally, the thing to make clear is that a handler
completely defines a bookmark's behavior.  And in
particular, it decides whether, when, and how to display a
bookmark destination.

I think the proposed approach (attached patch or similar)
would affect very few people.  It would affect only library
authors who define custom bookmark handlers (few do) that
don't invoke the default handler (fewer still), AND whose
code relegates some of the handling to the after-jump hook
and depends on `bookmark-jump' invoking the DISPLAY-FUNCTION
(very few, I expect).

The PDF Tools maintainers would need to change their handler
code, by having it invoke a display function or the default
handler.  (The code will be simpler.)

If they do that, then there's no problem for users,
regardless of what Emacs version they use.  In a version
without this change, only `bookmark--jump-via' invokes
DISPLAY-FUNCTION.  In a version with this change, only the
default handler invokes it.  The behavior is the same.

On the other hand, if they don't change their handler code
then users of vanilla `bookmark.el' will suffer the same
problem Bookmark+ users suffer now with such bookmarks -
they won't work because the destination won't be displayed.

If, for example, after this change to `bookmark.el', PDF
Tools didn't update its handler to display the destination,
then `bookmark.el' users of its PDF bookmarks would suffer
the same problem they suffer now with Bookmark+ (doesn't
work).

The patch updates the doc string of variable
`bookmark-alist', to clarify the use of alist entry
`handler'.  Please check that.

Please take a look at the patch and see what you think.

(Thanks to Roxana Dior for testing the PDF Tools fixes.)

bookmark-2022-08-11.patch
Description: bookmark-2022-08-11.patch

bookmark-patched-2022-08-11.el
Description: bookmark-patched-2022-08-11.el

bookmark-2022-08-11.el
Description: bookmark-2022-08-11.el

pdf-tools-handler-fix.el
Description: pdf-tools-handler-fix.el