Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data

["Basil L. Contovounesios" <contovob@tcd.ie>]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> Can you please explain the rationale behind this?  I'm not sure it's a
>> good idea:
>>
>> 1. Arbitrary data can already be associated with buttons and retrieved
>>    in their action functions using button-get, button-put, and on button
>>    creation with make{,-text}-button et al.
>>
>> 2. AIUI this is a potentially backward-incompatible change, as existing
>>    action functions, which expect to receive a button, may now receive
>>    the value of the button-data property instead.
>>
>> So at first glance I'm not convinced the minor convenience of not having
>> to manually (button-get button 'button-data) or similar in the action
>> function is worth messing with the pre-existent and cleaner API.  Am I
>> missing something?
>
> Button callback functions currently often recreate the data they need by
> looking at the extent of the buttons.

I don't understand what you mean by "recreate the data" or "looking at
the extent of the buttons".

If an action function depends on some data associated with its button,
then it is up to the creator or modifier of the button to tag it with
that data.  The action function then need only do a property lookup via
button-get.  Alternatively, action functions can also be closures.

> This is an awkward interface,

Why?

> because when creating the buttons, the functions have already
> determined what the data should be.

Shouldn't they store that data as a property of the button, or in a
closure, then?

> So it'd a way to de-duplicate code.

Can you please give me an example of such duplication?

> If you look at the functions in Gnus that use this now, you can see
> how the callback functions can be completely oblivious to having be
> called through a button.el callback, which is how it should be.

Which functions are those?  The ones that call gnus-article-add-button?
If so, gnus-article-add-button should specify an adapter function as the
button's action, rather than an arbitrary non-button-related function.

I don't think it's good to change the button API in a
backward-incompatible way just to allow specifying e.g. browse-url as a
button action function: browse-url is inherently button-unaware, and
should be wrapped for use with buttons.

> As for the name collision issue -- that's why I didn't call it just
> `data' or the like.  button.el already uses property names like
> `action', which is unfortunate, as that really is prone to naming
> collisions, but I grepped through the tree, and there are no in-tree
> usages of the `button-data' property.

I don't mind the name 'button-data', and I wasn't worried about naming
collisions.

What I'm worried about is the existence of buttons in the wild, whose
existing action functions will break if said buttons happen to be given
a button-data property.  This seems unnecessarily brittle and
backward-incompatible, in exchange for what seems like an insufficiently
useful convenience.  Unless I'm missing something, that is.

For example, existing code like the following:

(defun my-action (button)
  (browse-url (button-get button 'shr-url)))

(define-button-type 'my-type 'action #'my-action)

will now break if the button happens to be given a button-data property.

Am I the only one who thinks this is a problem, and not as elegant or as
flexible as the pre-existent API?

Thanks,

-- 
Basil