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

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

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