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

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

Drew, below you quite reasonably suggest that I show what I'm 
proposing by coding it up.  I didn't want to just let your message 
dangle without a reply, so this is just to say that I'm not sure 
if or when I'll have time to do that.  While I can volunteer to 
review code in a timely manner, I'm much more cautious about 
volunteering to write code these days.

I didn't want you to think my silence meant that I disagreed with 
your original idea or your followup -- I don't, it's just a matter 
of limited hacking time.

Best regards,
-Karl

On 06 Sep 2022, Drew Adams 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.
>
>Retaining that setting till it's reset could
>be a feature, not a bug. ;-)
>
>I imagine I won't have a problem if you bind
>it lexically in `bookmark--jump-via', but I'm
>not sure.
>
>Bookmark+ is designed to work also with Emacs
>versions that don't have lexical binding.
>Maybe using `lexical-let' here'll suffice,
>maybe not.
>
>A `nil' value does matter - it's not the same
>as calling a function unconditionally, even
>when the function is `ignore'.
>
>E.g., my version of `bookmark-default-handler'
>has this code, which doesn't raise the frame if
>there's no display going on (a bookmark need
>not display anything):
>
>(when bmkp-jump-display-function
>  (save-current-buffer
>    (funcall bmkp-jump-display-function (current-buffer)))
>  (raise-frame))
>
>Sure, I could change (when XXXX ...) to
>(when (eq XXXX 'ignore) ...), but that wouldn't
>really improve anything, IMO.
>
>And I test the display function in some handlers,
>for conditional handling/dispatch.  E.g., my
>handler for Dired bookmarks:
>
>(defun bmkp-jump-dired (bookmark)
>  "..."
>  (let ((dir
>         (bookmark-prop-get bookmark 'dired-directory))
>        (mfiles
>         (bookmark-prop-get bookmark 'dired-marked))
>        (switches
>         (bookmark-prop-get bookmark 'dired-switches))
>        (subdirs
>         (bookmark-prop-get bookmark 'dired-subdirs))
>        (hidden-dirs
>         (bookmark-prop-get bookmark 'dired-hidden-dirs)))
>    (case bmkp-jump-display-function
>      ((nil bmkp--pop-to-buffer-same-window display-buffer)
>       (dired dir switches))
>      ((bmkp-select-buffer-other-window
>        pop-to-buffer
>        switch-to-buffer-other-window)
>       (dired-other-window dir switches))
>      (t (dired dir switches)))
>    (let ((inhibit-read-only  t))
>      (dired-insert-old-subdirs subdirs)
>      (dired-mark-remembered 
>       (mapcar (lexical-let ((dd  dir))
>                 (lambda (mf)
>                   (cons (expand-file-name mf dd) 42)))
>               mfiles))
>      (save-excursion
>        (dolist (dir  hidden-dirs)
>          (when (dired-goto-subdir dir) (dired-hide-subdir 1)))))
>    (let ((pos  (bookmark-get-position bookmark)))
>      (when pos (goto-char pos)))))
>
>Obviously testing for functional equality isn't
>possible.  But testing a function symbol lets a
>particular kind of bookmark jump differently
>depending on current context/settings.  
>
>> > Indeed :-(
>
>Not so obvious to me.  Fine as a general guideline.
>Not clear that it's a real win (no loss) here.
>
>> > Why can't `bookmark--jump-via` let-bind
>> > `bookmark-jump-display-function` around 
>> > the call to `bookmark-handle-bookmark`?
>> 
>> Of course -- obvious... 
>> Drew, does that sound good to you?
>
>See above.  And please show your proposed code.
>
>> > 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)
>> > ...
>> > the above goes significantly beyond what one usually
>> > expects from a docstring.  It would fit much better
>> > in a Texinfo manual instead.
>
>Feel free to write a TexInfo manual for bookmarking,
>including for writing bookmark handlers, etc.  What
>exists now, and is as old as `bookmark.el', is just
>some code comments and that doc string.
>
>That doc string documents the data structure for a
>bookmark - it's THE place where that's specified.
>
>The handler part of that is important for people
>who create a new bookmark type (always has been).
>It's the one bit of bookmark data that really needs
>explaining, as it's the one part that provides
>behavior (code).
>
>> >> +  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.
>
>How is saying that a custom handler can invoke the
>default handler, and summarizing what that does,
>redundant here?
>
>Sure, it could just say that your handler can invoke
>the default handler, and leave it at that.  But that
>just begs the question "Huh? Why?".  And then, when
>you go off to look at the doc of that function, all
>you find is this (unmodified):
>
> Default handler to jump to a particular bookmark location.
> BMK-RECORD is a bookmark record, not a bookmark name (i.e., not 
> a string).
> Changes current buffer and point and returns nil, or signals a 
> `file-error'.
>
> If BMK-RECORD has a property called `buffer', it should be a 
> live
> buffer object, and this buffer will be selected.
>
>(Why shouldn't it be able to be a bookmark name, BTW?)
>
>Does that doc string help you, if you're writing
>your own handler?  The code helps, but not that
>doc string.  It doesn't tell you what jumping
>_means/does_ in the default case.
>
>IMO there's little sense in saying only that your
>handler _can_ call the default handler.  And even
>less sense in saying neither that it can nor what
>calling it does, i.e., _why_ you might want your
>handler to invoke the default handler.
>
>But TexInfo manual - welcome.  Go for it, please.
>
>> Agreed.
>
>Please show your proposed code (e.g. doc string).
>
>It's a deficiency of the existing doc string
>that it doesn't really tell you anything useful
>about the HANDLER part, IMO.
>
>But as I said, feel free to put what you like in
>any of the doc strings.
>
>The patch doesn't _require_ any mention of the
>possibility or use case of calling the default
>handler from a nondefault handler.  I think it's
>good to mention that, but you needn't do so to
>make the code change.
>
>> >> +(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`).
>
>See above.  I don't think anything's really gained
>by doing that here.  As a general guideline, sure,
>no problem.  But let's hear a specific argument
>for doing that here, please.
>
>> 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.
>
>Not so fine, IMO.  And you might still need to test
>its existence - with `(eq 'ignore)' instead of just
>non-nil.  And then there's trying to test for
>functions "equivalent" to `ignore'...
>
>[FWIW, a somewhat related feature, which I didn't
>include in my patch (but which could be considered
>for another change), wraps most of the code of
>`bookmark--jump-via' in a `catch', to which a
>handler can `throw' to avoid doing anything else
>(e.g. after-jump hook, auto-showing annotations).]
>
>> 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.
>
>If "otherwise" means that if a handler doesn't
>use a display function then the display of the
>default handler is used, then that's wrong.  A
>handler needs to be able to not display anything.
>
>> But all this raises the question you raise below...
>
>What question below?  Do you mean the question
>"Why `save-current-buffer'?"
>
>> >> @@ -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`?
>
>That's already in the code that the patch
>replaces.  It's in `bookmark--jump-via'.
>Displaying is now in the default handler (or
>another handler).  That's all.
>
>Take out that ` save-current-buffer', if you
>think you don't need it.  I'll likely leave it
>in my code.
>
>You can fiddle with the `bookmark--jump-via'
>code if you like, to ensure that the behavior
>change I described is the only one that's made.
>I think that's already the case, but feel free.
>
>The code right after `(save-current-buffer...)'
>expects the buffer that is current not to have
>changed.  In the unpatched code that right-after
>code is in `bookmark--jump-via'.  In the patched
>code it's in the default handler.
>
>> >>      (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`,
>
>Not in the code I patched or in the patched result,
>it isn't.
>
>`bookmark-insert' calls `bookmark-handle-bookmark'.
>That just invokes the default handler for _some_
>bookmarks.  In general it doesn't mean that any
>displaying occurs, and it doesn't say what
>"displaying" means for any given kind of bookmark.
>
>> > so I think `bookmark-insert` should explicitly bind
>> > `bookmark-jump-display-function` to `ignore`,
>
>But I don't have a problem if you want to do that.
>
>The problem you raise comes from `bookmark-insert'
>trying to use a handler to insert all of the text
>from the buffer that happens to be current after
>the bookmark is handled.  That assumes quite a lot.
>
>`bookmark-insert' is old (a vestige).  It seems to
>assume that you're just jumping to a file (see the
>doc string: "text of the file").  Or at least a
>text buffer.
>
>A more reasonable fix might be for `bookmark-insert'
>to do nothing or to raise an error if the bookmark
>type isn't a file bookmark.  (Or maybe a text file.
>Try it on a bookmark whose jumping displays an image
>file.)
>
>> > and of course that begs the question of what to do
>> > for handlers which don't obey 
>> > `bookmark-jump-display-function`.
>
>(Obey?  How about use?)  See above.  It's more than
>just a question of whether/how a bookmark displays
>something.  `bookmark-insert' apparently assumes a
>narrow (even if common) sort of bookmark.
>
>(BTW, do you notice `save-current-buffer' there?
>And that's before giving the handler any freedom
>to display.)
>
>> > 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.
>
>What caller?  Caller of what - a bookmark handler?
>Do you see any other callers of handlers, besides
>`bookmark-insert' (which is already broken for
>bookmarks that don't go to a text buffer/file)?
>
>What kind of display control do you want a caller
>of a handler to have?  And why does that seem to
>you to be a need?  Can you elaborate on some
>handler-calling you have in mind?
>
>> ...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?
>
>Redundant?  A handler's responsible for any
>displaying.  It can, but it need not, use the
>value of `bookmark-jump-display-function' to do
>that.  That's all.  
>
>What's the question?  By non-custom handler I
>guess you mean the default handler - it invokes
>`bookmark-jump-display-function'.
>
>BTW, with & without the patch there's this
>"redundancy": `bookmark-bmenu-switch-other-window'
>calls `bookmark--jump-via', passing a display
>function.  `bookmark-bmenu-other-frame' calls
>`bookmark-jump', passing a display function.
>
>The change the patch provides is to give handlers
>control over display - including choosing not to
>display anything.
> 
>> 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.
>
>I posted a patch that I think does what I propose.
>It's very close to the code of Bookmark+, which
>has been used for quite a while now.  Feel free to
>not apply the patch or to apply it and change any
>doc strings.
>
>If you replace var `bookmark-jump-display-function'
>with binding a function in `bookmark--jump-via'
>then I'll try to adjust my code for that.  If I
>can't do so reasonably then the syncing of handler
>behavior between Bookmark+ and bookmark.el might
>not be so seamless.
>
>If you have an alternative patch to propose,
>please do so.  I'll try to take a look.