bookmark.el and lisp/gnus/gnus-bookmark.el

bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Does anyone know why we have both 'bookmark.el' and 'gnus-bookmark.el'?

The latter says:

  ;; This file implements real bookmarks for Gnus, closely following the way
  ;; `bookmark.el' handles bookmarks.  Most of the code comes from
  ;; `bookmark.el'.

I'm not sure why GNUS bookmarks should live in a separate space from
other bookmarks, and my feeling is that they ought to be unified --
that is, the gnus-bookmark.el functionality absorbed into bookmark.el,
with compatibility shims to transfer people's ~/.gnus.bmk file
contents to ~/.emacs.bmk.  (Of course, I really mean whatever the
values of `gnus-bookmark-default-file' and `bookmark-default-file'
are, respectively.)

Thoughts?

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Stefan Monnier]

> Does anyone know why we have both 'bookmark.el' and 'gnus-bookmark.el'?
> The latter says:

>   ;; This file implements real bookmarks for Gnus, closely following the way
>   ;; `bookmark.el' handles bookmarks.  Most of the code comes from
>   ;; `bookmark.el'.

> I'm not sure why GNUS bookmarks should live in a separate space from
> other bookmarks, and my feeling is that they ought to be unified --
> that is, the gnus-bookmark.el functionality absorbed into bookmark.el,
> with compatibility shims to transfer people's ~/.gnus.bmk file
> contents to ~/.emacs.bmk.  (Of course, I really mean whatever the
> values of `gnus-bookmark-default-file' and `bookmark-default-file'
> are, respectively.)

> Thoughts?

Indeed, it seems worthwhile to merge them, tho I've never used gnus-bookmarks.
Bastien?


        Stefan

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Does anyone know why we have both 'bookmark.el' and 'gnus-bookmark.el'?
>> The latter says:
>
>>   ;; This file implements real bookmarks for Gnus, closely following the way
>>   ;; `bookmark.el' handles bookmarks.  Most of the code comes from
>>   ;; `bookmark.el'.
>
>> I'm not sure why GNUS bookmarks should live in a separate space from
>> other bookmarks, and my feeling is that they ought to be unified --
>> that is, the gnus-bookmark.el functionality absorbed into bookmark.el,
>> with compatibility shims to transfer people's ~/.gnus.bmk file
>> contents to ~/.emacs.bmk.  (Of course, I really mean whatever the
>> values of `gnus-bookmark-default-file' and `bookmark-default-file'
>> are, respectively.)
>
>> Thoughts?
>
> Indeed, it seems worthwhile to merge them, tho I've never used
> gnus-bookmarks.  Bastien?

Two answers: yes gnus-bookmarks.el should stored Gnus bookmarks in
~/.emacs.bmk (as every mode should do), but no we can't get rid of
gnus-bookmarks.el because it adds some keybindings to Gnus.

I think we should continue and generalize the work done by Tassilo.

He added the buffer-local variable `bookmark-make-cell-function', which
lets you define a handler for a mode: when setting a bookmark, a handler
is added to the record and jumping to the bookmark will use this handler
instead of `bookmark-jump' (see `bookmark-jump-internal'.)

This is all good, but it only works for buffer visiting files and
Info-mode. 


What would be great would be to change `bookmark-set' like this:

- run every function in `bookmark-set-functions' until one returns
  something useful (those functions returning something similar to
  ` bookmark-buffer-file-name');

- if nil, then fall back on `bookmark-buffer-file-name' and return 
  an error if this is nil.


If we go for this, then gnus-bookmarks.el would do this:

- add a function to `bookmark-set-functions', so that it return a
  sensible value (instead of the buffer-file-name)

- locally set `bookmark-make-cell-function' to define the right handler
  (which should select a group/article in Gnus)


I think this way let's people easily define man-bmk.el, or mew-bmk.el or
whatever.  

What you think?

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Bastien <bzg@altern.org> writes:
> Two answers: yes gnus-bookmarks.el should stored Gnus bookmarks in
> ~/.emacs.bmk (as every mode should do), but no we can't get rid of
> gnus-bookmarks.el because it adds some keybindings to Gnus.

Sure -- the existence of the file isn't a problem.  Whatever method we
have of establishing those keybindings in Gnus is fine.  I just meant
that the Gnus keybindings should bind to standard bookmark functions,
and those functions should DTRT for Gnus.  (As you seem to be
proposing below, too.)

> I think we should continue and generalize the work done by Tassilo.
>
> He added the buffer-local variable `bookmark-make-cell-function', which
> lets you define a handler for a mode: when setting a bookmark, a handler
> is added to the record and jumping to the bookmark will use this handler
> instead of `bookmark-jump' (see `bookmark-jump-internal'.)
>
> This is all good, but it only works for buffer visiting files and
> Info-mode. 

Yup.  The idea (as I understand it) was precisely that we could extend
it to handle other things, like Gnus.

> What would be great would be to change `bookmark-set' like this:
>
> - run every function in `bookmark-set-functions' until one returns
>   something useful (those functions returning something similar to
>   ` bookmark-buffer-file-name');
>
> - if nil, then fall back on `bookmark-buffer-file-name' and return 
>   an error if this is nil.
>
>
> If we go for this, then gnus-bookmarks.el would do this:
>
> - add a function to `bookmark-set-functions', so that it return a
>   sensible value (instead of the buffer-file-name)
>
> - locally set `bookmark-make-cell-function' to define the right handler
>   (which should select a group/article in Gnus)
>
> I think this way let's people easily define man-bmk.el, or mew-bmk.el or
> whatever.  
>
> What you think?

It sounds like a good plan to me.

Er, do you have time to do it?  I can add it to my queue, but I'm
behind as it is, and I think it would be some weeks at a minimum
before I could look at it, unfortunately.

-Karl

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Karl Fogel <kfogel@red-bean.com> writes:

> It sounds like a good plan to me.
>
> Er, do you have time to do it?  I can add it to my queue, but I'm
> behind as it is, and I think it would be some weeks at a minimum
> before I could look at it, unfortunately.

Considered the fact that I promised to work on gnus-bookmarks.el quite a
while ago (ahem), I can put this on top of my todo list and do it before
the end of the month.

Or if Tassilo is okay with the plan and he has time, maybe he wants to
work on the part in bookmark.el and I do the part in gnus-bookmark.el.

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Bastien <bzg@altern.org> writes:
> Considered the fact that I promised to work on gnus-bookmarks.el quite a
> while ago (ahem), I can put this on top of my todo list and do it before
> the end of the month.
>
> Or if Tassilo is okay with the plan and he has time, maybe he wants to
> work on the part in bookmark.el and I do the part in gnus-bookmark.el.

If he can't, I'll try to find time to join you on the work as soon as
I can.  I just want to get these 'yank-match' changes off my queue
first, they've been gathering dust for too long.

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Tassilo Horn <tassilo@member.fsf.org> writes:
> your bookmark renaming missed one occurence of
> bookmark-make-cell-function.
>
>   (local-variable-p 'bookmark-make-cell-function)
>
> in bookmark-set.

Wow.  And I even grepped (or thought I did).  Oh, I know what happened:
I dropped it in the conflict resolution after merging in Stefan's
changes, and forgot to grep again.

Thanks, fixed now.

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Karl Fogel <kfogel@red-bean.com> writes:

> Tassilo Horn <tassilo@member.fsf.org> writes:
>> your bookmark renaming missed one occurence of
>> bookmark-make-cell-function.
>>
>>   (local-variable-p 'bookmark-make-cell-function)
>>
>> in bookmark-set.
>
> Wow.  And I even grepped (or thought I did).  Oh, I know what happened:
> I dropped it in the conflict resolution after merging in Stefan's
> changes, and forgot to grep again.

And you prevented me to commit the change I'm describing in my last
email :-) 

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Stefan Monnier]

> He added the buffer-local variable `bookmark-make-cell-function', which
> lets you define a handler for a mode: when setting a bookmark, a handler
> is added to the record and jumping to the bookmark will use this handler
> instead of `bookmark-jump' (see `bookmark-jump-internal'.)

> This is all good, but it only works for buffer visiting files and
> Info-mode. 

I don't understand why you think so.  Could you explain in more details?

BTW bookmark-make-cell-function needs to be fixed so it doesn't receive
any `info-node' argument.  This was needed back when
bookmark-make-cell-function didn't exist, but now that info uses
bookmark-make-cell-function, bookmark.el shouldn't need any
info-specific code.

> - run every function in `bookmark-set-functions' until one returns
>   something useful (those functions returning something similar to
>   ` bookmark-buffer-file-name');

What would it do differently from bookmark-make-cell-function?
What additional behavior would be made available?


        Stefan

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> He added the buffer-local variable `bookmark-make-cell-function', which
>> lets you define a handler for a mode: when setting a bookmark, a handler
>> is added to the record and jumping to the bookmark will use this handler
>> instead of `bookmark-jump' (see `bookmark-jump-internal'.)
>
>> This is all good, but it only works for buffer visiting files and
>> Info-mode. 
>
> I don't understand why you think so.  Could you explain in more
> details?

This boils down to this: having a `bookmark-make-name-function' doing
for the names of the bookmarks what `bookmark-make-cell-function' does
for the records.  This would be locally set depending on the mode we're
in.  

For now the the name of the bookmark is set by bookmark-buffer-file-name
which doesn't return anything unless you are in a buffer visiting a file
or an Info node, or dired.

> BTW bookmark-make-cell-function needs to be fixed so it doesn't receive
> any `info-node' argument.  This was needed back when
> bookmark-make-cell-function didn't exist, but now that info uses
> bookmark-make-cell-function, bookmark.el shouldn't need any
> info-specific code.
>
>> - run every function in `bookmark-set-functions' until one returns
>>   something useful (those functions returning something similar to
>>   ` bookmark-buffer-file-name');
>
> What would it do differently from bookmark-make-cell-function?

Yes, bookmark-set-functions was not the right thing.  
Having a buffer-local value for `bookmark-make-name-function'
should be enough.  

This is rather simple, and will help getting rid of the code.  
Should I go for this?

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Tassilo Horn]

Bastien <bzg@altern.org> writes:

Hi Bastien,

>>> He added the buffer-local variable `bookmark-make-cell-function',
>>> which lets you define a handler for a mode: when setting a bookmark,
>>> a handler is added to the record and jumping to the bookmark will
>>> use this handler instead of `bookmark-jump' (see
>>> `bookmark-jump-internal'.)
>>
>>> This is all good, but it only works for buffer visiting files and
>>> Info-mode.
>>
>> I don't understand why you think so.  Could you explain in more
>> details?
>
> This boils down to this: having a `bookmark-make-name-function' doing
> for the names of the bookmarks what `bookmark-make-cell-function' does
> for the records.  This would be locally set depending on the mode
> we're in.
>
> For now the the name of the bookmark is set by
> bookmark-buffer-file-name which doesn't return anything unless you are
> in a buffer visiting a file or an Info node, or dired.

Hm, looking at the code `bookmark-buffer-file-name' is only called for
buffers that don't have `bookmark-make-record-function' set locally, so
I don't see why you would need that?

Or am I late again and Karl's and Stefan's changes already do care oy
everything we need?

Bye,
Tassilo

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Tassilo Horn <tassilo@member.fsf.org> writes:

> Bastien <bzg@altern.org> writes:
>>
>> This boils down to this: having a `bookmark-make-name-function' doing
>> for the names of the bookmarks what `bookmark-make-cell-function' does
>> for the records.  This would be locally set depending on the mode
>> we're in.
>>
>> For now the the name of the bookmark is set by
>> bookmark-buffer-file-name which doesn't return anything unless you are
>> in a buffer visiting a file or an Info node, or dired.
>
> Hm, looking at the code `bookmark-buffer-file-name' is only called for
> buffers that don't have `bookmark-make-record-function' set locally, so
> I don't see why you would need that?

I think you mean: `bookmark-make-cell-function', right?

Karl added this after I wrote the text you quote above.

> Or am I late again and Karl's and Stefan's changes already do care oy
> everything we need?

Yes, they do!

Now a mode can define `bookmark-make-cell-function' so that it tells
bookmark.el how to set the name of the bookmark.  

Some people talk, some people work. :)

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Tassilo Horn]

Bastien <bzg@altern.org> writes:

>>> This boils down to this: having a `bookmark-make-name-function'
>>> doing for the names of the bookmarks what
>>> `bookmark-make-cell-function' does for the records.  This would be
>>> locally set depending on the mode we're in.
>>>
>>> For now the the name of the bookmark is set by
>>> bookmark-buffer-file-name which doesn't return anything unless you
>>> are in a buffer visiting a file or an Info node, or dired.
>>
>> Hm, looking at the code `bookmark-buffer-file-name' is only called
>> for buffers that don't have `bookmark-make-record-function' set
>> locally, so I don't see why you would need that?
>
> I think you mean: `bookmark-make-cell-function', right?

Nope, Karl has renamed it to bookmark-make-record-function.

> Karl added this after I wrote the text you quote above.
>
>> Or am I late again and Karl's and Stefan's changes already do care oy
>> everything we need?
>
> Yes, they do!

Good boys! ;-)

> Now a mode can define `bookmark-make-cell-function' so that it tells
> bookmark.el how to set the name of the bookmark.

Now it's bookmark-make-record-function, but that has been there (as
b-m-cell-f) for a while now...

,----[ ~/repos/emacs/lisp/ChangeLog ]
| 2007-12-26  Tassilo Horn  <tassilo@member.fsf.org>
| 
| 	* image-mode.el (image-bookmark-make-cell, image-bookmark-jump):
| 	New functions.
| 	(image-mode): Set bookmark-make-cell-function appropriately.
| 
| 	* doc-view.el (doc-view-bookmark-jump): Correct misspelled arg name.
| 
| 	* bookmark.el (bookmark-make-cell-function): New variable.
| 	(bookmark-make): Call bookmark-make-cell-function's function
| 	instead of bookmark-make-cell.
| 	(bookmark-get-handler, bookmark-jump-internal): New functions.
| 	(bookmark-jump, bookmark-jump-other-window, bookmark-insert)
| 	(bookmark-bmenu-2-window, bookmark-bmenu-other-window):
| 	Use bookmark-jump-internal.
| 	(bookmark-make-cell-for-text-file): Rename from bookmark-make-cell.
| 
| 	* doc-view.el (doc-view-bookmark-make-cell)
| 	(doc-view-bookmark-jump): New functions.
| 	(doc-view-mode): Set bookmark-make-cell-function buffer-locally.
`----

And I don't see how it sets the name.  The name is set in `bookmark-set'
by querying the user and using a default value of the current bookmark
(if set) or the buffer name.

> Some people talk, some people work. :)

And others can't follow while others talk. :-)

Bye,
Tassilo
-- 
Richard Stallman has a katana. 'Nuff said.

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Tassilo Horn <tassilo@member.fsf.org> writes:

>>> Hm, looking at the code `bookmark-buffer-file-name' is only called
>>> for buffers that don't have `bookmark-make-record-function' set
>>> locally, so I don't see why you would need that?
>>
>> I think you mean: `bookmark-make-cell-function', right?
>
> Nope, Karl has renamed it to bookmark-make-record-function.

No, there is both 

  bookmark-make-record-function (line 466)
  bookmark-make-cell-function (line 741)

>> Now a mode can define `bookmark-make-cell-function' so that it tells
>> bookmark.el how to set the name of the bookmark.
>
> Now it's bookmark-make-record-function, but that has been there (as
> b-m-cell-f) for a while now...

I think there is a misunderstanding somewhere.  
bookmark-make-record-function makes the *record* depending on the mode
we are in.  But so far, there way no other way to get the *name* of the
bookmark than to rely on `bookmark-buffer-file-name'.  

Now the local value of `bookmark-make-cell-function' is first checked,
in case the mode sets this specifically.

> And others can't follow while others talk. :-)

:)

Hopes it's clearer now...

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Tassilo Horn]

Bastien <bzg@altern.org> writes:

>>>> Hm, looking at the code `bookmark-buffer-file-name' is only called
>>>> for buffers that don't have `bookmark-make-record-function' set
>>>> locally, so I don't see why you would need that?
>>>
>>> I think you mean: `bookmark-make-cell-function', right?
>>
>> Nope, Karl has renamed it to bookmark-make-record-function.
>
> No, there is both 
>
>   bookmark-make-record-function (line 466)
>   bookmark-make-cell-function (line 741)

Yeah, but I think Karl missed the second one (make-cell) while renaming.
I've already sent him a mail.

Looking at the code of bookmark-set, I think I'm right.

  (or
   (local-variable-p 'bookmark-make-cell-function)
   (bookmark-buffer-file-name)
   (error "Buffer not visiting a file or directory"))

If the record-creation is done by another mode, it's fine.  Else, if
(bookmark-buffer-file-name) returns nil, which it does for buffers with
no file, error.  This is needed, because if
bookmark-make-record-function is not set by the mode, the default
bookmark-make-record-for-text-file will be used, and that works only
with buffers associated with a file.

>>> Now a mode can define `bookmark-make-cell-function' so that it tells
>>> bookmark.el how to set the name of the bookmark.
>>
>> Now it's bookmark-make-record-function, but that has been there (as
>> b-m-cell-f) for a while now...
>
> I think there is a misunderstanding somewhere.

Yep, and I think exceptionally not on my side. :)

Bye,
Tassilo

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Tassilo Horn <tassilo@member.fsf.org> writes:
>>>> I think you mean: `bookmark-make-cell-function', right?
>>>
>>> Nope, Karl has renamed it to bookmark-make-record-function.
>>
>> No, there is both 
>>
>>   bookmark-make-record-function (line 466)
>>   bookmark-make-cell-function (line 741)
>
> Yeah, but I think Karl missed the second one (make-cell) while renaming.
> I've already sent him a mail.

Tassilo is right.

All my change(s) did was to rename `bookmark-make-cell-function' to
`bookmark-make-record-function', which is a better name because it's
more suggestive of the actual purpose and less implementation-specific.

(I was in the process of also implementing something very similar to
what Stefan did, but his commit came in first, so I merged in his
changes instead and just did the above variable rename.)

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Karl Fogel <kfogel@red-bean.com> writes:

>> Yeah, but I think Karl missed the second one (make-cell) while renaming.
>> I've already sent him a mail.
>
> Tassilo is right.

He is right about the fact that you forget to rename the variable, but
renaming this variable is not enough - sorry to insist.

We also need to update `bookmark-buffer-name' so that it returns a
sensible value in *any* mode: for now it only returns a value in a
buffer visiting a file, in dired and in info mode.

My proposal is to add this:

(defvar bookmark-make-name-function nil
  "A function that should be called to return the name of the bookmark.
Modes may set this variable buffer-locally to enable a default name to
be proposed when calling `bookmark-set'.")

I thought your mistake about not renaming `bookmark-make-cell-function'
was not a mistake about renaming, but a mistake about forgetting to make 
`bookmark-buffer-name'.handle this new variable.  

I think we should:

1. Update `bookmark-buffer-name' like this:

(defun bookmark-buffer-name ()
  "Return the name of the current buffer's file, non-directory.
In Info, return the current node."
  (cond
   ;; Is the mode defining it's own bookmark name?
   (bookmark-make-name-function
    (funcall bookmark-make-name-function))
   ;; Are we in Info?
   ((derived-mode-p 'Info-mode) Info-current-node)
   ;; Or are we a file?
   (buffer-file-name (file-name-nondirectory buffer-file-name))
   ;; Or are we a directory?
   ((and (boundp 'dired-directory) dired-directory)
    (let* ((dirname (if (stringp dired-directory)
                        dired-directory
                      (car dired-directory)))
           (idx (1- (length dirname))))
      ;; Strip the trailing slash.
      (if (= ?/ (aref dirname idx))
          (file-name-nondirectory (substring dirname 0 idx))
        ;; Else return the current-buffer
        (buffer-name (current-buffer)))))
   ;; If all else fails, use the buffer's name.
   (t
    (buffer-name (current-buffer)))))

2. Use `bookmark-make-name-function' function in `bookmark-set' instead
   of `bookmark-make-record-function':

...
  (or
   (local-variable-p 'bookmark-make-name-function)
   (bookmark-buffer-file-name)
   (error "Buffer not visiting a file or directory"))
...

If you agree, I will make those changes.  They are required for the
rewrite of gnus-bookmark.el.

Thanks for considering this, and for your patience!

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Bastien <bzg@altern.org> writes:
> He is right about the fact that you forget to rename the variable, but
> renaming this variable is not enough - sorry to insist.

Oh, it's okay -- I wasn't claiming anything beyond Tassilo being right
about the intention of my rename.

> We also need to update `bookmark-buffer-name' so that it returns a
> sensible value in *any* mode: for now it only returns a value in a
> buffer visiting a file, in dired and in info mode.
>
> My proposal is to add this:
>
> (defvar bookmark-make-name-function nil
>   "A function that should be called to return the name of the bookmark.
> Modes may set this variable buffer-locally to enable a default name to
> be proposed when calling `bookmark-set'.")

Okay...

> I thought your mistake about not renaming `bookmark-make-cell-function'
> was not a mistake about renaming, but a mistake about forgetting to make 
> `bookmark-buffer-name'.handle this new variable.  

...no, all I was doing was the rename, but...

> I think we should:
>
> 1. Update `bookmark-buffer-name' like this:
>
> (defun bookmark-buffer-name ()
>   "Return the name of the current buffer's file, non-directory.
> In Info, return the current node."

...if we're going to have `bookmark-make-name-function' now, shouldn't
Info mode implement that function just like any other mode would?  Why
have bookmark.el handle Info-related things?

> 2. Use `bookmark-make-name-function' function in `bookmark-set' instead
>    of `bookmark-make-record-function':

You mean for making it buffer-local, right?  (We're not actually calling
it.)  But wouldn't we need to make them *both* buffer-local?  External
code will still need to call `bookmark-make-record-function' too.

> ...
>   (or
>    (local-variable-p 'bookmark-make-name-function)
>    (bookmark-buffer-file-name)
>    (error "Buffer not visiting a file or directory"))
> ...
>
> If you agree, I will make those changes.  They are required for the
> rewrite of gnus-bookmark.el.

The overall plan sounds reasonable to me, modulo the above questions.
Although I've been guilty earlier in this thread of agreeing to readily
without looking at the code, I have looked at the code this time, and
still agree! :-)

Thanks,
-Karl

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien Guerry]

Karl Fogel <kfogel@red-bean.com> writes:

>> (defun bookmark-buffer-name ()
>>   "Return the name of the current buffer's file, non-directory.
>> In Info, return the current node."
>
> ...if we're going to have `bookmark-make-name-function' now, shouldn't
> Info mode implement that function just like any other mode would?  Why
> have bookmark.el handle Info-related things?

Yes, we should move Info-related code out of bookmark.el.

>> 2. Use `bookmark-make-name-function' function in `bookmark-set' instead
>>    of `bookmark-make-record-function':
>
> You mean for making it buffer-local, right?

Right!

> But wouldn't we need to make them *both* buffer-local?  External code
> will still need to call `bookmark-make-record-function' too.

Yes, they should be both buffer-local.

> The overall plan sounds reasonable to me, modulo the above questions.

My single remaining hesitation is this one: having two buffer local
variables is a bit too much, since each mode would have to set them
both.  Better use `bookmark-make-record-function' for both purposes:
returning a name *or* returning a record.

For example, the function for text files would look like:

(defun bookmark-make-record-for-text-file (annotation &optional name)
   "Return the record.  
If optional arg NAME is non-nil, just return the default name for
this bookmark."
   ...)

I think it's easier.  Each mode should have to worry about one
buffer-local variable (and each dev would only read one docstring...)

> Although I've been guilty earlier in this thread of agreeing to readily
> without looking at the code, I have looked at the code this time, and
> still agree! :-)

Ok, I continue to agree !  Unless told otherwise (Tassilo?) I will
implement the solution with one single variable, and let Info-mode
set this variable accordingly.

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Bastien Guerry <bzg@altern.org> writes:
> My single remaining hesitation is this one: having two buffer local
> variables is a bit too much, since each mode would have to set them
> both.  Better use `bookmark-make-record-function' for both purposes:
> returning a name *or* returning a record.

Ick :-).  I don't think having an optional argument change the return
semantics of the function would be a good solution.

Remember, we're already asking external callers to implement two
bookmark functions: the make-record function, and the jump function.
Adding make-name to that is no great burden: clearly, any caller for who
`buffer-file-name' or any of the other default fallbacks won't suffice
is going to have to implement a name generator.  Whether they do that as
a separate function or not doesn't change the burden.

I would suggest writing a `bookmark-initialize' function that sets up
whatever local variables are necessary.  That way, that function's doc
string can remind people what interfaces they have to implement, and
they can go read up more on those interfaces.

> I think it's easier.  Each mode should have to worry about one
> buffer-local variable (and each dev would only read one docstring...)

See above about setting buffer-local and having a doc string.

-Karl

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Karl Fogel <kfogel@red-bean.com> writes:

> Ick :-).  I don't think having an optional argument change the return
> semantics of the function would be a good solution.

Ok, understood, I'm doing this.  Thanks for the advice! 

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien Guerry]

Bastien <bzg@altern.org> writes:

> Karl Fogel <kfogel@red-bean.com> writes:
>
>> Ick :-).  I don't think having an optional argument change the return
>> semantics of the function would be a good solution.
>
> Ok, understood, I'm doing this.  Thanks for the advice! 

Done it.  

BTW, I didn't define `bookmark-initialize' since it would make it
necessary for info.el to require bookmark.el, which doesn't look 
like a good idea to me -- better stick with current way of setting
`bookmark-make-*-function' locally.

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Stefan Monnier]

>>> 2. Use `bookmark-make-name-function' function in `bookmark-set' instead
>>> of `bookmark-make-record-function':
>> You mean for making it buffer-local, right?
> Right!

I do not understand.  Bookmark.el shouldn't make any of those variables
buffer-local, AFAICT.

>> But wouldn't we need to make them *both* buffer-local?  External code
>> will still need to call `bookmark-make-record-function' too.

> Yes, they should be both buffer-local.

Let the modes that use them call make-local-variable.

> My single remaining hesitation is this one: having two buffer local
> variables is a bit too much, since each mode would have to set them
> both.  Better use `bookmark-make-record-function' for both purposes:
> returning a name *or* returning a record.

> For example, the function for text files would look like:

> (defun bookmark-make-record-for-text-file (annotation &optional name)
>    "Return the record.  
> If optional arg NAME is non-nil, just return the default name for
> this bookmark."
>    ...)

> I think it's easier.  Each mode should have to worry about one
> buffer-local variable (and each dev would only read one docstring...)

This might work, tho I'd throw away the `name' arg.  Let the
buffer-local function build a bookmark record (i.e. a cons cell of the
form (NAME . ALIST)) with any name it chooses, and then change the name
according to the user's choice.  I.e. make the bookmark record before
even prompting the user for a name.


        Stefan

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Stefan Monnier]

> This might work, tho I'd throw away the `name' arg.  Let the
> buffer-local function build a bookmark record (i.e. a cons cell of the
> form (NAME . ALIST)) with any name it chooses, and then change the name
> according to the user's choice.  I.e. make the bookmark record before
> even prompting the user for a name.

By the same reasoning we can drop the `annotation' argument and add the
annotation later in bookmark.el.


        Stefan

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (defun bookmark-make-record-for-text-file (annotation &optional name)
>>    "Return the record.  
>> If optional arg NAME is non-nil, just return the default name for
>> this bookmark."
>>    ...)
>
>> I think it's easier.  Each mode should have to worry about one
>> buffer-local variable (and each dev would only read one docstring...)
>
> This might work, tho I'd throw away the `name' arg.  Let the
> buffer-local function build a bookmark record (i.e. a cons cell of the
> form (NAME . ALIST)) with any name it chooses, and then change the name
> according to the user's choice.  I.e. make the bookmark record before
> even prompting the user for a name.

Following Karl advice I've applied a different patch.  There is now
`bookmark-make-name-function' *and* `bookmark-make-record-function',
both buffer local.  For an example on how this is supposed to work 
look at the changes I've made in info.el.  

I think it's okay like this.  If you plan to change anything please 
tell me before I work on gnus-bookmark.el. 

Thanks,

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Stefan Monnier]

>>> (defun bookmark-make-record-for-text-file (annotation &optional name)
>>> "Return the record.  
>>> If optional arg NAME is non-nil, just return the default name for
>>> this bookmark."
>>> ...)
>> 
>>> I think it's easier.  Each mode should have to worry about one
>>> buffer-local variable (and each dev would only read one docstring...)
>> 
>> This might work, tho I'd throw away the `name' arg.  Let the
>> buffer-local function build a bookmark record (i.e. a cons cell of the
>> form (NAME . ALIST)) with any name it chooses, and then change the name
>> according to the user's choice.  I.e. make the bookmark record before
>> even prompting the user for a name.

> Following Karl advice I've applied a different patch.  There is now
> `bookmark-make-name-function' *and* `bookmark-make-record-function',
> both buffer local.  For an example on how this is supposed to work 
> look at the changes I've made in info.el.  

> I think it's okay like this.  If you plan to change anything please 
> tell me before I work on gnus-bookmark.el. 

I think we should do as I suggest above: a single function of no
arguments that is called first that returns a bookmark record, including
a suggested name (which can be nil so as to use the default heuristic
to decide the default bookmark name).

This has another advantage: since the function is called first, it can
signal an error if the bookmark can't be created (e.g. if
buffer-file-name is nil), so we can drop the (or (local-variable-p ...)
(bookmark-buffer-file-name) (error ...)) test which is currently forced
to guess whether the actual bookmark construction function will work.


        Stefan

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> I think we should do as I suggest above: a single function of no
> arguments that is called first that returns a bookmark record, including
> a suggested name (which can be nil so as to use the default heuristic
> to decide the default bookmark name).
>
> This has another advantage: since the function is called first, it can
> signal an error if the bookmark can't be created (e.g. if
> buffer-file-name is nil), so we can drop the (or (local-variable-p ...)
> (bookmark-buffer-file-name) (error ...)) test which is currently forced
> to guess whether the actual bookmark construction function will work.

I think this way is fine too.  I almost proposed it in my first
response, in fact -- well, something close to it.  What I'd suggest is
that `bookmark-make-record-function's value return an alist:

   ((`name' . NAME) (`record' . RECORD) ...)

That's extensible, and if you don't want to supply a name, just leave
out that alist element entirely.

-Karl



   

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Following Karl advice I've applied a different patch.  There is now
>> `bookmark-make-name-function' *and* `bookmark-make-record-function',
>> both buffer local.  For an example on how this is supposed to work 
>> look at the changes I've made in info.el.  
>
>> I think it's okay like this.  If you plan to change anything please 
>> tell me before I work on gnus-bookmark.el. 
>
> I think we should do as I suggest above: a single function of no
> arguments that is called first that returns a bookmark record, including
> a suggested name (which can be nil so as to use the default heuristic
> to decide the default bookmark name).

If we get rid of the annotation argument for this single function, it
means the annotation has to be done before this function returns sth
that can be used as the suggested name for the bookmark.  

I'm all for a single function -- see my "single hesitation" in this
thread.  I suggested an optional argument "name" so that this function
would only return a suggested name for the bookmark, or a complete
record if name is nil.  

(defun bookmark-make-record-for-my-mode (name)
  (if name
      (buffer-file-name) ; or sth else
    [return the record]))

With this solution, we could have a single function and get rid of the
annotation without making it mandatory to *read* the annotation before
`bookmark-set' is able to propose a suggested name.

What do you think?

BTW, reading a new annotation and sending it doesn't restore the window
configuration here.  I will fix this.

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Stefan Monnier]

> If we get rid of the annotation argument for this single function, it
> means the annotation has to be done before this function returns sth
> that can be used as the suggested name for the bookmark.  

AFAICT getting rid of the annotation argument means nothing at all,
other than that the annotation will have to be added by the generic
bookmark-set code rather than being done by every single
bookmark-make-record-function.


        Stefan

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> If we get rid of the annotation argument for this single function, it
>> means the annotation has to be done before this function returns sth
>> that can be used as the suggested name for the bookmark.  
>
> AFAICT getting rid of the annotation argument means nothing at all,
> other than that the annotation will have to be added by the generic
> bookmark-set code rather than being done by every single
> bookmark-make-record-function.

For now `bookmark-set' asks for an annotation (if required), sends it to
`bookmark-make' which calls `bookmark-make-record-function' with the
annotation as an argument.

IIUC Tassilo suggested that the annotation should be directly handled by
`bookmark-make-record-function', which seems correct to me.  What you
propose is to handle annotation in `bookmark-set' (or preferrably in
`bookmark-make') -- this is _feasible_, but it looks weird to me since
the annotation is really part of the record.

BTW, I really think the name suggestion should be taken care of by
`bookmark-make-record-function' (with an optional "name" arg), not 
by a separate call to `bookmark-make-name-function'...

Anyway - I let Karl summary and decide on this!

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Stefan Monnier]

>>> If we get rid of the annotation argument for this single function, it
>>> means the annotation has to be done before this function returns sth
>>> that can be used as the suggested name for the bookmark.  
>> 
>> AFAICT getting rid of the annotation argument means nothing at all,
>> other than that the annotation will have to be added by the generic
>> bookmark-set code rather than being done by every single
>> bookmark-make-record-function.

> For now `bookmark-set' asks for an annotation (if required), sends it to
> `bookmark-make' which calls `bookmark-make-record-function' with the
> annotation as an argument.

> IIUC Tassilo suggested that the annotation should be directly handled by
> `bookmark-make-record-function', which seems correct to me.  What you
> propose is to handle annotation in `bookmark-set' (or preferrably in
> `bookmark-make') -- this is _feasible_, but it looks weird to me since
> the annotation is really part of the record.

The record has various parts: a name to select it, an annotation,
a bunch of data to find the place, there could be more (date the record
was created, when it was last used, ...).

The bookmark-make-record-function should only concern itself with
providing the data to find the place.  It may also provide a default
name, a default annotation, ... but it's better if it doesn't need to
handle annotations since those are (or rather should be) handled
identically for *all* bookmark-make-record-function.


        Stefan

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Karl Fogel]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> For now `bookmark-set' asks for an annotation (if required), sends it to
>> `bookmark-make' which calls `bookmark-make-record-function' with the
>> annotation as an argument.
>
>> IIUC Tassilo suggested that the annotation should be directly handled by
>> `bookmark-make-record-function', which seems correct to me.  What you
>> propose is to handle annotation in `bookmark-set' (or preferrably in
>> `bookmark-make') -- this is _feasible_, but it looks weird to me since
>> the annotation is really part of the record.
>
> The record has various parts: a name to select it, an annotation,
> a bunch of data to find the place, there could be more (date the record
> was created, when it was last used, ...).
>
> The bookmark-make-record-function should only concern itself with
> providing the data to find the place.  It may also provide a default
> name, a default annotation, ... but it's better if it doesn't need to
> handle annotations since those are (or rather should be) handled
> identically for *all* bookmark-make-record-function.

I agree with Stefan's analysis here.  A bookmark record is not an opaque
object: it is okay for `bookmark-set' or any other bookmark.el function
to supply parts of the record that are not specific to the mode
generating the bookmark.  The external mode code should concern itself
with the location data -- that code is both producer and consumer of
that data.  Since bookmark.el is always the consumer of the annotation
data, it should be the producer of that data as well.

Fortunately, we're using alists here, so all data is labeled in
comprehensible ways.  It should be easy to keep things straight.

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Reiner Steib]

On Thu, Mar 06 2008, Bastien wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Karl Fogel:
>>>   ;; This file implements real bookmarks for Gnus, closely following the way
>>>   ;; `bookmark.el' handles bookmarks.  Most of the code comes from
>>>   ;; `bookmark.el'.
>>> I'm not sure why GNUS bookmarks should live in a separate space from
>>> other bookmarks, and my feeling is that they ought to be unified --
>> Indeed, it seems worthwhile to merge them, tho I've never used
>> gnus-bookmarks.  Bastien?
> Two answers: yes gnus-bookmarks.el should stored Gnus bookmarks in
> ~/.emacs.bmk (as every mode should do), but no we can't get rid of
> gnus-bookmarks.el because it adds some keybindings to Gnus.

On Sat, Mar 08 2008, Bastien wrote:
> Following Karl advice I've applied a different patch.  There is now
> `bookmark-make-name-function' *and* `bookmark-make-record-function',
> both buffer local.  For an example on how this is supposed to work 
> look at the changes I've made in info.el.  
>
> I think it's okay like this.  If you plan to change anything please 
> tell me before I work on gnus-bookmark.el. 

I didn't follow this discussion closely.  Do I understand correctly
that after changing gnus-bookmark.el, it will take advantage of new
features in bookmark.el that are not present in Emacs < 23.  I.e. it
wont work in Emacs 22 at all (and XEmacs?).

As `gnus-bookmark.el' is quite new in No Gnus, I don't expect many
Emacs <= 22 users use it intensively.  If it is not too much work, it
would be nice to make it work for all supported Emacsen, cf. (info
"(gnus)Emacsen").  But I don't insist on this.  If you don't support
Emacs < 23, please ...

- document that only Emacs 23 and up are supported,

- make sure that it doesn't disturb compiling Gnus with Emacs < 23 too
  much,

- signal appropriate errors when trying to use it in Emacs < 23.

Bye, Reiner.

P.S.: Please strip me from further CC's (I read the list(s));
      instead include ding@gnus.org if the discussion is Gnus-related.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

> Do I understand correctly that after changing gnus-bookmark.el, it
> will take advantage of new features in bookmark.el that are not
> present in Emacs < 23.  I.e. it wont work in Emacs 22 at all (and
> XEmacs?).

Yes.

> As `gnus-bookmark.el' is quite new in No Gnus, I don't expect many
> Emacs <= 22 users use it intensively.  If it is not too much work, it
> would be nice to make it work for all supported Emacsen, cf. (info
> "(gnus)Emacsen").  But I don't insist on this.  

I think this is too much work, and as you said, not that many people
might be using this feature.

The only thing is this: gnus-bookmark.el was good to get Gnus bookmarks
and *only* them.  This will go away with the new gnus-bookmark.el.  So,
since the new feature of bookmark.el will help many modes provide their
own ways of setting a bookmark, I think it would make sense to have a
"type" key in the bookmark record (similar to the annotation), and be
able to list bookmarks of a certain type only.

C-u C-x r l [Prompt type] Info RET 
  => list bookmark of Info type

C-u C-x r l [Prompt type] Gnus RET 
  => list Gnus bookmarks

If you think it is a good idea, I will do it.

> If you don't support
> Emacs < 23, please ...
>
> - document that only Emacs 23 and up are supported,
>
> - make sure that it doesn't disturb compiling Gnus with Emacs < 23 too
>   much,
>
> - signal appropriate errors when trying to use it in Emacs < 23.

Ok, will do this.

-- 
Bastien

Re: bookmark.el and lisp/gnus/gnus-bookmark.el

[Bastien]

Hi Karl,

Bastien <bzg@altern.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> He added the buffer-local variable `bookmark-make-cell-function', which
>>> lets you define a handler for a mode: when setting a bookmark, a handler
>>> is added to the record and jumping to the bookmark will use this handler
>>> instead of `bookmark-jump' (see `bookmark-jump-internal'.)
>>
>>> This is all good, but it only works for buffer visiting files and
>>> Info-mode. 
>>
>> I don't understand why you think so.  Could you explain in more
>> details?
>
> This boils down to this: having a `bookmark-make-name-function' doing
> for the names of the bookmarks what `bookmark-make-cell-function' does
> for the records.  This would be locally set depending on the mode we're
> in.  

I can see you've made this change, and used bookmark-make-cell-function
instead of bookmark-make-name-function -- can you confirm the change so
that I update gnus-bookmark.el accordingly?

BTW, maybe `bookmark-make-name-function' should be advertized somewhere
in bookmark.el.  And PARG argument is not mentionned in bookmark-set.

Thanks,

-- 
Bastien