* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data [not found] ` <20190730132509.77D2820C0A@vcs0.savannah.gnu.org> @ 2019-08-01 12:11 ` Basil L. Contovounesios 2019-08-01 12:22 ` Lars Ingebrigtsen 0 siblings, 1 reply; 8+ messages in thread From: Basil L. Contovounesios @ 2019-08-01 12:11 UTC (permalink / raw) To: emacs-devel; +Cc: Lars Ingebrigtsen 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data 2019-08-01 12:11 ` [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data Basil L. Contovounesios @ 2019-08-01 12:22 ` Lars Ingebrigtsen 2019-08-01 15:19 ` Basil L. Contovounesios 0 siblings, 1 reply; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-08-01 12:22 UTC (permalink / raw) To: Basil L. Contovounesios; +Cc: emacs-devel "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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data 2019-08-01 12:22 ` Lars Ingebrigtsen @ 2019-08-01 15:19 ` Basil L. Contovounesios 2019-08-01 16:12 ` Lars Ingebrigtsen 0 siblings, 1 reply; 8+ messages in thread From: Basil L. Contovounesios @ 2019-08-01 15:19 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data 2019-08-01 15:19 ` Basil L. Contovounesios @ 2019-08-01 16:12 ` Lars Ingebrigtsen 2019-08-01 20:44 ` Stefan Monnier 2019-08-02 0:40 ` Basil L. Contovounesios 0 siblings, 2 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-08-01 16:12 UTC (permalink / raw) To: Basil L. Contovounesios; +Cc: emacs-devel "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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data 2019-08-01 16:12 ` Lars Ingebrigtsen @ 2019-08-01 20:44 ` Stefan Monnier 2019-08-02 18:41 ` Lars Ingebrigtsen 2019-08-02 0:40 ` Basil L. Contovounesios 1 sibling, 1 reply; 8+ messages in thread From: Stefan Monnier @ 2019-08-01 20:44 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Basil L. Contovounesios, emacs-devel > (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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data 2019-08-01 20:44 ` Stefan Monnier @ 2019-08-02 18:41 ` Lars Ingebrigtsen 0 siblings, 0 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-08-02 18:41 UTC (permalink / raw) To: Stefan Monnier; +Cc: Basil L. Contovounesios, emacs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data 2019-08-01 16:12 ` Lars Ingebrigtsen 2019-08-01 20:44 ` Stefan Monnier @ 2019-08-02 0:40 ` Basil L. Contovounesios 2019-08-02 18:46 ` Lars Ingebrigtsen 1 sibling, 1 reply; 8+ messages in thread From: Basil L. Contovounesios @ 2019-08-02 0:40 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data 2019-08-02 0:40 ` Basil L. Contovounesios @ 2019-08-02 18:46 ` Lars Ingebrigtsen 0 siblings, 0 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-08-02 18:46 UTC (permalink / raw) To: Basil L. Contovounesios; +Cc: emacs-devel "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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-08-02 18:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20190730132507.32385.70681@vcs0.savannah.gnu.org> [not found] ` <20190730132509.77D2820C0A@vcs0.savannah.gnu.org> 2019-08-01 12:11 ` [Emacs-diffs] master b75fb81 1/4: Extend button.el to take callback data Basil L. Contovounesios 2019-08-01 12:22 ` Lars Ingebrigtsen 2019-08-01 15:19 ` Basil L. Contovounesios 2019-08-01 16:12 ` Lars Ingebrigtsen 2019-08-01 20:44 ` Stefan Monnier 2019-08-02 18:41 ` Lars Ingebrigtsen 2019-08-02 0:40 ` Basil L. Contovounesios 2019-08-02 18:46 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.