Re: Proposal for a new Plugin.execute() signature on Android

[Andrew Grieve <ag...@chromium.org>]

Just confirmed that it looks like you did actually add the method to the
interface. Any reasons for keeping this interface? Otherwise, I'll go ahead
and remove it.


On Fri, Sep 28, 2012 at 11:43 AM, Braden Shepherdson <br...@chromium.org>wrote:

> 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 <agrieve@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.johnson@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 <ag...@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 <agrieve@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.macdonald@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 <
> agrieve@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.mike@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
> > > >>> >> <si...@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 <
> > > >>> agrieve@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.macdonald@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 <
> > > >>> mmocny@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 <
> > > >>> >> agrieve@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.
> > > >>> >> > >>> > >
> > > >>> >> > >>>
> > > >>> >> >
> > > >>> >>
> > > >>>
> > > >>
> > > >>
> > >
> >
>