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

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

[Basil L. Contovounesios]

larsi@gnus.org (Lars Ingebrigtsen) writes:

> branch: master
> commit b75fb81e362b8afbf37da0d2480676269430694c
> Author: Lars Ingebrigtsen <larsi@gnus.org>
> Commit: Lars Ingebrigtsen <larsi@gnus.org>
>
>     Extend button.el to take callback data

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?

Thanks,

-- 
Basil

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

[Lars Ingebrigtsen]

"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.  This is an awkward interface,
because when creating the buttons, the functions have already determined
what the data should be.  So it'd a way to de-duplicate code.  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.

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.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Basil L. Contovounesios]

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

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

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

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

Here's a typical usage:

      (make-text-button start (point)
			'face 'rcirc-url
			'follow-link t
			'rcirc-url url
			'action (lambda (button)
				  (browse-url (button-get button 'rcirc-url))))

with the "button knows the data" change, it's:

      (make-text-button start (point)
			'face 'rcirc-url
			'follow-link t
			'burron-data url
			'action #'browse-url)

That looks like an interface improvement to me.

> Alternatively, action functions can also be closures.

They can be, in lexical packages.

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

But you said you're not worried about naming collisions?   How would
these buttons then "happen to be given" that property?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Stefan Monnier]

>       (make-text-button start (point)
> 			'face 'rcirc-url
> 			'follow-link t
> 			'burron-data url
> 			'action #'browse-url)

Odd.  I thought the benefit was for

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

    [...]
    
       (make-text-button start (point)
                         'type 'my-type
                         'burron-data url)

where a closure is not an option.  In your above case, you could just use

    (make-text-button start (point)
                      'face 'rcirc-url
                      'follow-link t
                      'action (lambda () (browse-url url)))

>> Alternatively, action functions can also be closures.
> They can be, in lexical packages.

In this for the purpose of designing new APIs, we can take it for
granted that lexical-scoping is available.  It's easy enough to convert
an old package to use lexical-scoping.


        Stefan

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

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> 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. 
>
> Here's a typical usage:
>
>       (make-text-button start (point)
> 			'face 'rcirc-url
> 			'follow-link t
> 			'rcirc-url url
> 			'action (lambda (button)
> 				  (browse-url (button-get button 'rcirc-url))))
>
> with the "button knows the data" change, it's:
>
>       (make-text-button start (point)
> 			'face 'rcirc-url
> 			'follow-link t
> 			'burron-data url
> 			'action #'browse-url)
>
> That looks like an interface improvement to me.

I'm arguing that something like the following is a better interface
improvement, because it doesn't require changing the interface:

  (defun button-callback-action (button)
    "Call BUTTON's `button-callback' property."
    (apply #'funcall (button-get button 'button-callback)))

  (define-button-type 'rcirc-url
    'face 'rcirc-url
    'follow-link t
    'action #'button-callback-action)

  (make-text-button start (point)
                    'type 'rcirc-url
                    'button-callback (list #'browse-url url))

(Using define-button-type is optional, but encouraged.)

Providing such a convenience function in button.el (or one of its client
libraries) affords the same brevity as the button-data approach, without
changing the button API, making existing buttons more brittle, or
sacrificing generality.

Button action functions have until now expected a button as an argument,
but now they may or may not be given a button, and this is determined by
the presence or not of a button property.  Why introduce this brittle
backward incompatibility?

>> Alternatively, action functions can also be closures.
>
> They can be, in lexical packages.

Sure; I only mentioned closures for completeness, not as a total
solution.

>> 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.
>
> But you said you're not worried about naming collisions?   How would
> these buttons then "happen to be given" that property?

Oh, we seem to be interpreting "naming collision" slightly differently.

I took it to mean that a client of the button library has used a
property name for one purpose, which is used for another purpose
elsewhere.

But what is happening here is that existing code is written with the
assumption that button action functions receive a button as argument.
This assumption now breaks down on buttons with a non-nil button-data
property.  Why make this unnecessary and backward-incompatible change?

Thanks,

-- 
Basil

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

[Lars Ingebrigtsen]

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

>>       (make-text-button start (point)
>> 			'face 'rcirc-url
>> 			'follow-link t
>> 			'burron-data url
>> 			'action #'browse-url)
>
> Odd.  I thought the benefit was for
>
>     (define-button-type 'my-type 'action #'browse-url)
>
>     [...]
>
>        (make-text-button start (point)
>                          'type 'my-type
>                          'burron-data url)
>
> where a closure is not an option.

Ah, yes, that seems useful.

> In this for the purpose of designing new APIs, we can take it for
> granted that lexical-scoping is available.  It's easy enough to convert
> an old package to use lexical-scoping.

In this case, I just wanted to rewrite some stuff to avoid using
widgets, which can be rather convoluted to reason about.  But it does
have callback values like this, so this brings button a feature to make
conversion easier.

But, yes, everything should be converted to lexical binding, and then
my example use case is much less of a thing.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>   (defun button-callback-action (button)
>     "Call BUTTON's `button-callback' property."
>     (apply #'funcall (button-get button 'button-callback)))
>
>   (define-button-type 'rcirc-url
>     'face 'rcirc-url
>     'follow-link t
>     'action #'button-callback-action)
>
>   (make-text-button start (point)
>                     'type 'rcirc-url
>                     'button-callback (list #'browse-url url))
>
> (Using define-button-type is optional, but encouraged.)

Button is such a nice and light-weight library (especially compared to
widget).  Having to write these wrapper functions to do these trivial
transforms is the one bad thing in the API.

> Button action functions have until now expected a button as an argument,
> but now they may or may not be given a button, and this is determined by
> the presence or not of a button property.  Why introduce this brittle
> backward incompatibility?

I don't see how it's brittle, and it's not backwards incompatible.

If you add a 'button-data 'foo to your buttons, then you presumably want
your action to get the data.  Otherwise, why would you do it?

> Oh, we seem to be interpreting "naming collision" slightly differently.
>
> I took it to mean that a client of the button library has used a
> property name for one purpose, which is used for another purpose
> elsewhere.
>
> But what is happening here is that existing code is written with the
> assumption that button action functions receive a button as argument.
> This assumption now breaks down on buttons with a non-nil button-data
> property.  Why make this unnecessary and backward-incompatible change?

I still don't understand what you mean here.

If somebody writes a new button that has 'button-data, how does that
affect the existing button?  If you write a new button, and say
'button-data, then the 'action you've written takes that data as the
argument, not the button.

There's nobody randomly adding 'button-data to random existing code,
surely?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no