> On 2011-08-31 21:43:06, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js, > > line 789 > > <https://reviews.apache.org/r/1689/diff/2/?file=37008#file37008line789> > > > > Any particular reason why you chose to do the removeListener this way? > > I would expect the API to take in the listener you want to remove, not a > > token.... > > One disadvantage I could think of is you have to save off this token > > somewhere to use it later on if you want to remove it. Then again you also > > need to save off your listener if you want to remove it as well, so maybe > > there really is no difference. Also as far as consistency, are there other > > listener APIs in the container today? How do they do it?
The examples of registering handlers in actions are all singletons. So is the container's addGadgetLifecycleCallback. I was looking for something a little more flexible for the actions listeners. Allowing the container to register different functions to handle different action invocations (or the same action invocation). I also did not want to have 1 single function callback for all actions, as actions can happen many times... and there is a cost to executing a function. Checking a boolean expression first to see if we should even call the callback based on the id saves us that unneeded function call. If there's significant pressure from the community on conforming to the existing model I'll follow suit, but I think actions are a specific, dynamic, and frequent enough occurrence to justify a more flexible impl. - Dan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1689/#review1705 ----------------------------------------------------------- On 2011-08-31 20:07:34, Dan Dumont wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1689/ > ----------------------------------------------------------- > > (Updated 2011-08-31 20:07:34) > > > Review request for shindig, Ryan Baxter and Stanton Sievers. > > > Summary > ------- > > Specification: > http://code.google.com/p/opensocial-resources/issues/detail?id=1210 > > Add 2 functions to the actions feature. > 1 to allow the container to register a listener for an action being run. The > listener would be run whenever an action matching the id provided was run. > 1 to allow the container to unregister(remove) any listener added with the > previous function. > > > This addresses bug SHINDIG-1612. > https://issues.apache.org/jira/browse/SHINDIG-1612 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js > 1163664 > > Diff: https://reviews.apache.org/r/1689/diff > > > Testing > ------- > > If anyone has any suggestions as to how to approach adding unit tests for > this, I'm all ears. > > It looks like that in order to get the actions api to hit this code, by > calling runAction, I'd need to somehow fake setting up a gadget site and > register a fake action. > > > Thanks, > > Dan > >
