Re: doc-view support for bookmark.el

[Tassilo Horn]

Karl Fogel <address@hidden> writes:

Hi Karl,

> I like your new bookmark abstractions, Tassilo!

:-)

> A few comments:
>
> Tassilo Horn <address@hidden> writes:
>> --- lisp/bookmark.el 25 Sep 2007 10:43:39 -0000      1.98
>> +++ lisp/bookmark.el 25 Dec 2007 21:51:50 -0000
>> @@ -480,6 +482,22 @@
>>      (interactive-p)
>>      (setq bookmark-history (cons ,string bookmark-history))))
>>  
>> +(defvar bookmark-make-cell-function 'bookmark-make-cell-for-text-file
>> +  "A function that should be called to create the bookmark
>> +record.  Modes may set this variable buffer-locally to enable
>> +bookmarking of non-text files like images or pdf documents.
>> +
>> +The function will be called with two arguments: ANNOTATION and
>> +INFO-NODE.  See `bookmark-make-cell-for-text-file' for a
>> +description.
>> +
>> +The returned record may contain a special cons (handler
>> +. some-function) which sets the handler function that should be
>> +used to open this bookmark instead of `bookmark-jump-noselect'.
>> +It should return a cons (BUFFER . POINT) indicating buffer
>> +showing the bookmarked location and the value of point in that
>> +buffer.  Like `bookmark-jump-noselect' the buffer shouldn't be
>> +selected by the handler.")
>
> Okay, so "(BUFFER . POINT)" is now an exported interface, not just an
> internal interface that we would be free to change inside bookmark.el.
> Therefore, let's make it more extensible than a dotted pair.  The
> cleanest thing would probably be an association list:
>
>    '((buffer BUFFER) (point POINT))
>
> That way, callers will be compatible even if we extend the type later:
>
>    '((buffer BUFFER) (point POINT) ...)

Yes, that seems to be a good idea.  And since it's a trivial change to
adapt current callers, I don't see any reason not to do it.  (Drew, I
hope we can still be friends. ;-))

> And a comment regarding documentation:
>
> Normally, a function type's documentation should all be in one place,
> I think, not referring out to specific instances.  So normally, I
> would suggest replacing this...
>
>    The function will be called with two arguments: ANNOTATION and
>    INFO-NODE.  See `bookmark-make-cell-for-text-file' for a
>    description.
>
> ... with full documentation of the INFO-NODE argument.  But in this
> case, the better thing might be to implement
> `bookmark-make-cell-for-info-node' as a separate instance of
> `bookmark-make-cell-function', adjusting everything else in the
> obvious way.

Definitely.  I didn't want to document INFO-NODE, because it hardly
makes any sense for anything but bookmarks in info docs.  Basically the
yet to be done `bookmark-make-cell-for-info-node' can use
`b-m-c-f-text-file' and install only another handler
(bookmark-jump-info), so that `bookmark-jump-noselect' can be split into
`bookmark-jump-text' and `bookmark-jump-info'.  Then the INFO-NODE arg
could be dropped.

I'm not sure when I find some time to work on that.  Xmas was a good
excuse not to work on my diploma thesis, but I mustn't put it off any
longer.  So if sombody wants to do it, go for it.

Bye,
Tassilo