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

[Karl Fogel]

On 29 Aug 2022, Stefan Monnier wrote:
> > 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`?

Of course -- obvious, now that you point it out.  Thank you for 
reviewing, Stefan.

Drew, does that sound good to you?

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

Agreed.

> > +(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`).

I think I understand: by taking your advice, we don't have to 
check the function's existence -- we can just call it 
unconditionally, and if it happens to do nothing (i.e., is 
`ignore'), that's fine.  A specific package may still override the 
default handler and do whatever display magic is called for; 
otherwise, bookmark.el's default handler will Do The Right Thing 
for displaying.  But all this raises the question you raise 
below...

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

...which is that we now seem to have two possibly-redundant ways 
to control displaying: write a custom handler, *or* bind 
`bookmark-jump-display-function' to something?

Or perhaps I'm misunderstanding something.  Drew, if you post the 
next revision of the patch incorporating Stefan's points above, I 
will review that in detail, when fully alert, and should be able 
to distill any remaining questions -- I think we'd be pretty close 
at that point.

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

I agree that that makes sense as a long term goal.

Best regards,
-Karl