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

[Karl Fogel <kfogel@red-bean.com>]

On 13 Aug 2022, Drew Adams wrote:
>tl;dr:
>
>Let's change the handling of a bookmark - including
>displaying the bookmark destination - to be the
>responsibility of its handler.

In general, +1 to this idea.

(My ability to be involved in actually making it happen may be 
limited right now, due to some family responsibilities.  I'll try 
to at least kibbitz constructively though.)

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

*nod*

There's some parallelism-unsafety going on here, obviously.  I'd 
love a way to wrap all this in a clean closure, instead of setting 
a global variable which then retains that setting until the next 
time we happen to set it.

But I don't really see a way to do tihngs more cleanly than with 
the carry-out variable you propose.  If there's some creative 
solution for connecting `bookmark--jump-via' to the 
"corresponding" handler call that happens later, I don't see it, 
lexically or dynamically.  Maybe is just how Emacs usually handles 
these kinds of situations?

>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.
>
>[...some example usages from Bookmark+...]

This is all very well-explained; thank you, Drew.

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

Yup; that makes sense to me.

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

My main concern here is just that compatibility issue.  I have no 
idea how many bookmark-using packages will break if we install 
this patch.  You've explained very clearly how to fix them, and 
how in many cases their code is likely to get cleaner as a result, 
and I agree.

But still, compatibility-breaking changes are a serious thing. 
I'd like to know what others here think about this.  Your 
suggested guidelines below seem good *if* we decide to make this 
change:

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

Yup.

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

I expect you are right.  I'm even inclined to risk the breakage 
and help callers fix it, just to get this long-term structural 
improvement.

But I'm not confident in this opinion, and would like to know what 
others here, especially the maintainers, think.

I reviewed the bookmark.el patch briefly, and overall it had the 
shape I expected.  I might have a few suggestions about doc 
strings and such; if we decide to go down this road, I'll do a 
real patch review.

Best regards,
-Karl