I didn't actually add onReset to the IPlugin interface, just to the Plugin class, for exactly this reason.
On Fri, Sep 28, 2012 at 11:20 AM, Andrew Grieve <agri...@chromium.org>wrote: > Did some more thinking about this yesterday, and I think that we should > remove IPlugin (regardless of other changes). Here's my thinking: > > If we provide an interface for plugins, then we're promising to support it. > Supporting it means not changing it, since *any* change to it will break > classes that try to implement it. We just broke this rule recently by > adding an "onReset()" function to it, so once 2.2.0 goes out with this > change, any non-Plugin-based implementation will have compile errors. > > If we change this to supporting a Plugin base class, then that gives us a > *lot* more flexibility to change things without breaking existing plugins. > e.g. We can add an onReset() and just have an empty default implementation. > Another straight-forward tweak is to add the execute(String, String, > String), which forwards to execute(String, JSONArray, String) by default. > With a base class, this is a safe change, but with an interface, this will > break your compile. > > > My second attempt got rid of the executeV2 name. I hate that as well. > > > On Thu, Sep 27, 2012 at 7:50 PM, Dave Johnson <dave.c.john...@gmail.com > >wrote: > > > There was a lot of discussion many moons ago about that plugin arch > > and I think we ended up on IPlugin + Plugin just so that people could > > go with IPlugin and implement everything themselves or use our > > hopefully sane defaults in Plugin. There's likely very few people > > using IPlugin. > > > > Also, can we avoid the "executeV2" method name by any chance? Reminds > > me of net.rim.device.api.browser.field2 :) > > > > On Thu, Sep 27, 2012 at 10:28 PM, Andrew Grieve <agri...@chromium.org> > > wrote: > > > Here it is: > > > https://github.com/mmocny/incubator-cordova-android/pull/2/files > > > > > > I delete IPlugin in this version, but that probably wasn't a great idea > > > since other people may have used the symbol. Still not understanding > what > > > it's utility is though, but if we go ahead with this, I'll at *least* > > > revert deleting IPlugin, and do that in a separate change, or more > > likely, > > > just leave IPlugin alone. > > > > > > > > > On Wed, Sep 26, 2012 at 2:58 PM, Andrew Grieve <agri...@chromium.org> > > wrote: > > > > > >> Aha, okay. So on iOS they happen asynchronously to the webcore thread, > > but > > >> all execute in order on the UI thread, I think even moving away from > > having > > >> each execute on a new thread by default would bring the behaviour > > closer to > > >> iOS. > > >> > > >> The other big unknown to me, is the question of why the IPlugin > > interface > > >> exists instead of just using the Plugin base class. Does anyone know > of > > any > > >> other implementations of IPlugin besides Plugin? > > >> > > >> I'm going to take a stab at re-writing this change to use a > super-class > > to > > >> have the new signature, and have the existing Plugin class extend that > > and > > >> provide the shim, and will report back for everyone to review again. > > >> Probably won't get to it today, but maybe tomorrow. > > >> > > >> > > >> On Wed, Sep 26, 2012 at 2:04 PM, Simon MacDonald < > > >> simon.macdon...@gmail.com> wrote: > > >> > > >>> Yeah, sorry I meant to get back to you on that. The major reason for > > >>> switching everything to async was that iOS can only do async and this > > >>> helped keep the code bases/API consistent. > > >>> > > >>> Simon Mac Donald > > >>> http://hi.im/simonmacdonald > > >>> > > >>> > > >>> On Wed, Sep 26, 2012 at 1:47 PM, Andrew Grieve <agri...@chromium.org > > > > >>> wrote: > > >>> > Okay, so I think everyone is on the same page in terms of not > > breaking > > >>> > existing plugins. > > >>> > > > >>> > Does anyone know what the reason was for making plugins async by > > >>> default? > > >>> > > > >>> > > > >>> > On Wed, Sep 26, 2012 at 1:38 PM, Mike Reinstein < > > >>> reinstein.m...@gmail.com>wrote: > > >>> > > > >>> >> Agreed! If we can get to some kind of stability with the API > > exposed to > > >>> >> plugin developers it will go a long way. > > >>> >> > > >>> >> > > >>> >> > > >>> >> On Wed, Sep 26, 2012 at 1:32 PM, Simon MacDonald > > >>> >> <simon.macdon...@gmail.com>wrote: > > >>> >> > > >>> >> > Agreed. We've broken the plugins so many times that I'm more > that > > >>> sure > > >>> >> > that 3rd party devs are sick of it. The last time we broken the > > >>> >> > interface on Android was in 1.9.0 and then we broke it again in > > 2.0.0 > > >>> >> > on the JavaScript side. I'd rather not break it again for 2.2.0. > > >>> >> > > > >>> >> > Also, when I say "break" I mean the code I wrote to the previous > > >>> >> > specification will no longer compile so I need to make changes > to > > my > > >>> >> > plugin. Often we can get around this by adding in a shim which I > > >>> >> > believe is the best way to go. > > >>> >> > > > >>> >> > Simon Mac Donald > > >>> >> > http://hi.im/simonmacdonald > > >>> >> > > > >>> >> > > > >>> >> > On Wed, Sep 26, 2012 at 3:56 AM, Brian LeRoux <b...@brian.io> > wrote: > > >>> >> > > The only concern I have is the deprecation path needs to be > long > > >>> and > > >>> >> > > noisy---this is probably the biggest possible breaking change > we > > >>> could > > >>> >> > > introduce to the platform. > > >>> >> > > > > >>> >> > > Maybe even longer than our usual 6months / but wait until 3.0 > > >>> >> > > > > >>> >> > > Thoughts on that? > > >>> >> > > > > >>> >> > > > > >>> >> > > On Tue, Sep 25, 2012 at 4:56 PM, Andrew Grieve < > > >>> agri...@chromium.org> > > >>> >> > wrote: > > >>> >> > >> Michal - Yep, good summary, that's exactly the case. > > >>> >> > >> Simon - totally agree. I'll change what I've got to add a > > second > > >>> >> > executeV2 > > >>> >> > >> which takes in a JSONArray, and have the String-based one > just > > >>> call > > >>> >> > that. > > >>> >> > >> > > >>> >> > >> The reason to need an executeV2 is threading, so I'll focus > on > > >>> that. > > >>> >> > >> > > >>> >> > >> My biggest gripe against the current signature, is that it > > >>> defaults to > > >>> >> > >> running things on a background thread. I expect most calls > > will be > > >>> >> fast > > >>> >> > >> enough to execute inline, some calls to need to run on the UI > > >>> thread, > > >>> >> > and > > >>> >> > >> then only some to require doing a lot of work on a background > > >>> thread. > > >>> >> > >> Furthermore, those that do require a background would often > > >>> benefit > > >>> >> from > > >>> >> > >> doing some param/state checking on the calling thread before > > >>> moving to > > >>> >> > the > > >>> >> > >> background thread. > > >>> >> > >> > > >>> >> > >> I wouldn't be proposing a new signature if there was a way to > > >>> change > > >>> >> > >> isSync() from defaulting to false to defaulting to true, but > I > > >>> don't > > >>> >> > think > > >>> >> > >> that's a safe thing to change. > > >>> >> > >> > > >>> >> > >> On iOS, plugins execute on the calling thread and it's up to > > them > > >>> to > > >>> >> > >> dispatch background threads if they need them. > > >>> >> > >> > > >>> >> > >> Michal pointed out that you can't comment on a diff in > github, > > so > > >>> I > > >>> >> > opened > > >>> >> > >> a pull request with the patch to enable commenting: > > >>> >> > >> > > >>> >> > > > >>> >> > > >>> > > > https://github.com/agrieve/incubator-cordova-android/commit/a73dffc99847b14031c1138611bb8772dc9d7b7e > > >>> >> > >> > > >>> >> > >> > > >>> >> > >> > > >>> >> > >> > > >>> >> > >> > > >>> >> > >> On Tue, Sep 25, 2012 at 10:51 AM, Simon MacDonald < > > >>> >> > simon.macdon...@gmail.com > > >>> >> > >>> wrote: > > >>> >> > >> > > >>> >> > >>> Here is what I was thinking on: > > >>> >> > >>> > > >>> >> > >>> https://issues.apache.org/jira/browse/CB-1530 > > >>> >> > >>> > > >>> >> > >>> In the PluginManager change the code so that is calls: > > >>> >> > >>> > > >>> >> > >>> plugin.execute(string, string, string); > > >>> >> > >>> > > >>> >> > >>> Then in the Plugin class add a new default method that does > > the > > >>> >> > following: > > >>> >> > >>> > > >>> >> > >>> public PluginResult execute(String action, String args, > String > > >>> >> > callbackId) > > >>> >> > >>> { > > >>> >> > >>> return execute(action, new JSONArrary(args), > callbackId); > > >>> >> > >>> } > > >>> >> > >>> > > >>> >> > >>> so that all the current plugins continue to work without > > needing > > >>> any > > >>> >> > >>> changes. If someone wants to provide their own JSON parsing > > they > > >>> can > > >>> >> > >>> override the plugin.execute(string, string, string) method > and > > >>> do it > > >>> >> > >>> themselves. > > >>> >> > >>> > > >>> >> > >>> Simon Mac Donald > > >>> >> > >>> http://hi.im/simonmacdonald > > >>> >> > >>> > > >>> >> > >>> > > >>> >> > >>> On Tue, Sep 25, 2012 at 10:33 AM, Michal Mocny < > > >>> mmo...@chromium.org> > > >>> >> > >>> wrote: > > >>> >> > >>> > > > >>> >> > >>> > Summarizing what I think I'm hearing: > > >>> >> > >>> > > > >>> >> > >>> > The current exec signature will currently: > > >>> >> > >>> > (a) automatically parse JSON arguments, and > > >>> >> > >>> > (b) automatically move async calls onto a background > thread. > > >>> >> > >>> > > > >>> >> > >>> > While both of the features simplify plugin developers in > > most > > >>> >> cases, > > >>> >> > >>> > sometimes manual control is desired (ie, for the two bugs > > you > > >>> link > > >>> >> > to). > > >>> >> > >>> > > > >>> >> > >>> > That sounds reasonable, however, I think I'm also hearing > a > > >>> >> proposal > > >>> >> > to > > >>> >> > >>> > replace the existing execute signature (deprecating the > > current > > >>> >> > one). If > > >>> >> > >>> > for the majority of cases we are happy with the current > > >>> signature, > > >>> >> > then > > >>> >> > >>> is > > >>> >> > >>> > there perhaps a less intrusive solution? Or maybe we > aren't > > >>> happy > > >>> >> > with > > >>> >> > >>> the > > >>> >> > >>> > current signature, and this new signature is generally > more > > >>> future > > >>> >> > proof, > > >>> >> > >>> > more performant, etc, giving us other reasons for > changing? > > >>> Also, > > >>> >> > how > > >>> >> > >>> does > > >>> >> > >>> > this compare with other platforms? > > >>> >> > >>> > > > >>> >> > >>> > -Michal > > >>> >> > >>> > > > >>> >> > >>> > > > >>> >> > >>> > On Mon, Sep 24, 2012 at 11:50 PM, Andrew Grieve < > > >>> >> agri...@google.com> > > >>> >> > >>> wrote: > > >>> >> > >>> > > > >>> >> > >>> > > Means to address two bugs: > > >>> >> > >>> > > > > >>> >> > >>> > > https://issues.apache.org/jira/browse/CB-1530 > > >>> >> > >>> > > https://issues.apache.org/jira/browse/CB-1532 > > >>> >> > >>> > > > > >>> >> > >>> > > I wanted to gather some opinions from those who have > been > > >>> around > > >>> >> > for > > >>> >> > >>> > > longer. Here is the proposed change: > > >>> >> > >>> > > > > >>> https://github.com/agrieve/incubator-cordova-android/compare/ft3 > > >>> >> > >>> > > > > >>> >> > >>> > > My main motivation is for FileTransfer, I need to > register > > >>> the > > >>> >> > transfer > > >>> >> > >>> > > synchronously so that a subsequent abort() will not > have a > > >>> race > > >>> >> > >>> condition. > > >>> >> > >>> > > I then perform the transfer in a background thread. I > > *could* > > >>> >> > implement > > >>> >> > >>> > > this using the current signature by returning true in > > >>> isSync() > > >>> >> and > > >>> >> > then > > >>> >> > >>> > > returning a NO_RESULT result, but I think the intentions > > are > > >>> >> > clearer > > >>> >> > >>> with > > >>> >> > >>> > > the new signature. > > >>> >> > >>> > > > > >>> >> > >>> > > >>> >> > > > >>> >> > > >>> > > >> > > >> > > >
