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