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

[Stefan Monnier <monnier@iro.umontreal.ca>]

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

Indeed :-(

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

Hmm... I must be missing something.  Why can't `bookmark--jump-via`
let-bind `bookmark-jump-display-function` around the call to
`bookmark-handle-bookmark`?

Regarding the patch, here are some comments:

> +  To display the destination, HANDLER can call the function that's the
> +  value of variable `bookmark-jump-display-function', which is set by
> +  `bookmark-jump' to automatically accommodate other-window etc.
> +  displaying that depends on the jump command.  For example:
> +
> +   (funcall bookmark-jump-display-function (current-buffer))
> +
> +  Or HANDLER can directly call another display function.  For example:
> +
> +   (switch-to-buffer-other-tab BUFFER)

I skipped most of the rest of the docstring's massaging, which I'm not
sure is an improvement but with which I guess I can live.  But the above
goes significantly beyond what one usually expects from a docstring.
It would fit much better in a Texinfo manual instead.

> +  Or HANDLER can invoke `bookmark-default-handler'.  That displays the
> +  destination.  It then moves to the recorded buffer position, POS,
> +  repositioning point, if necessary, to match the recorded context
> +  strings STR-BEFORE-POS and STR-AFTER-POS.  For instance, the Info
> +  handler, `Info-bookmark-jump', does this at its end:

And this is (or should be) redundant with the docstring of
`bookmark-default-handler` so better keep the link and the the readers
jump to it when they need/want to know what that function does.

> +(defvar bookmark-jump-display-function nil
> + "Function used currently to display a bookmark, or nil if no function.")

Please give it a function as default value (e.g. `ignore`).

> @@ -1350,6 +1391,9 @@
>        ((and buf (get-buffer buf)))
>        (t ;; If not, raise error.
>         (signal 'bookmark-error-no-filename (list 'stringp file)))))
> +    (when bookmark-jump-display-function
> +      (save-current-buffer (funcall bookmark-jump-display-function
> +                                    (current-buffer))))

Why `save-current-buffer`?

>      (if place (goto-char place))
>      ;; Go searching forward first.  Then, if forward-str exists and
>      ;; was found in the file, we can search backward for behind-str.

Hmm... `bookmark-default-handler` is also called via things like
`bookmark-insert`, so I think `bookmark-insert` should explicitly bind
`bookmark-jump-display-function` to `ignore`, and of course that begs
the question of what to do for handlers which don't obey
`bookmark-jump-display-function`.

I think it's good to provide more control to the bookmark's handler, but
there seems to be a need for the caller to control the display as well
to some extent.

BTW, your generalization of bookmarks to "anything" makes it all the
more obvious to me that these should be unified with "registers" (not
necessarily at the UI level, but a register should be able to hold
anything that can stored as a bookmark, and vice versa).


        Stefan