[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Let bookmark handlers do everything, including display the d
|
From: |
Karl Fogel |
|
Subject: |
Re: [PATCH] Let bookmark handlers do everything, including display the destination |
|
Date: |
Thu, 18 Aug 2022 13:14:09 -0500 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) |
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