Mailing-List: contact dev-help@cordova.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cordova.apache.org
Received-SPF: pass (nike.apache.org: domain of agrieve@google.com designates
 209.85.220.50 as permitted sender)
MIME-Version: 1.0
Sender: agrieve@google.com
In-Reply-To: 
 <CAOBL_k40G1z0XUmNDHHBcsk3d_Z2Qmnj4X5oOXv878yGnPm=EQ@mail.gmail.com>
References: 
 <CABiQX1V0xF6Ua-QbvX5WM02GmLkXNhppOg=aHa9r=GkYUVn=Gw@mail.gmail.com>
 <CAOBL_k5-uQacab12D6a3tkHJY2tnQB7=DGO4frNkG33VLzaN9g@mail.gmail.com>
 <52ce3648.4166420a.7305.5f76@mx.google.com>
 <CABiQX1UUVa=CssAiBV76+Jsr873E44aYVDJw5+z-F93xMNA5sw@mail.gmail.com>
 <CAOBL_k40G1z0XUmNDHHBcsk3d_Z2Qmnj4X5oOXv878yGnPm=EQ@mail.gmail.com>
From: Andrew Grieve <agrieve@chromium.org>
Date: Thu, 9 Jan 2014 10:42:50 -0800
Message-ID: 
 <CABiQX1U-nYPG-d1ch8Pv-jfvW38Yi7XWmFoqfJ7e7Qi7PXtq9Q@mail.gmail.com>
Subject: Re: Plugin Hooks on Android
To: dev <dev@cordova.apache.org>, "bowserj@apache.org" <bowserj@apache.org>
Content-Type: text/plain; charset=UTF-8

On Thu, Jan 9, 2014 at 9:25 AM, Joe Bowser <bowserj@gmail.com> wrote:
> On Thu, Jan 9, 2014 at 6:06 AM, Andrew Grieve <agrieve@chromium.org> wrote:
>> Good point about postMessage allowing devs to add their own messages.
>> That's a very compelling argument to keep postMessage around, but I
>> don't think it means we should be using it for our "officially
>> supported" hooks.
>>
>
> Why? This just sounds like personal preference.
My reasons were listed below.

>
>> On Wed, Jan 8, 2014 at 9:40 PM, purplecabbage <purplecabbage@gmail.com> wrote:
>>> I'm with Joe.
>>> Stay flexible, and future friendly.
>>> The messages that are posted probably just need to be better documented, or we need to point plugin devs towards Android's docs.
>>>
>>> Sent from my iPhone
>>>
>>>> On Jan 8, 2014, at 9:11 PM, Joe Bowser <bowserj@gmail.com> wrote:
>>>>
>>>>> On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <agrieve@chromium.org> wrote:
>>>>> Breaking out a discussion Joe & I started on
>>>>> https://issues.apache.org/jira/browse/CB-5351.
>>>>>
>>>>> Some plugin hooks on Android are made available as functions on the
>>>>> CordovaPlugin class. If you are interested in the hook, you override
>>>>> the function.
>>>>>
>>>>> Some hooks are made available through the onMessage(String name,
>>>>> Object obj) function. Examples that I could find:
>>>>>
>>>>> postMessage("spinner", "stop");
>>>>> postMessage("exit", null);
>>>>> postMessage("splashscreen", "show");
>>>>> postMessage("onScrollChanged", myEvent); <-- data is a custom class
>>>>> postMessage("onPageFinished", url);
>>>>> postMessage("onPageStarted", url);
>>>>> postMessage("onReceivedError", data); <-- data is a JSONObject
>>>>> postMessage("onCreateOptionsMenu", menu); <-- a Menu object
>>>>> postMessage("onPrepareOptionsMenu", menu);  <-- a Menu object
>>>>> postMessage("onOptionsItemSelected", item); <-- a MenuItem object
>>>>>
>>>>> The split seems about 50/50.
>>>>>
>>>>> Now, what I'd like to propose is that we do away with postMessage
>>>>> pattern. Reasons:
>>>>> 1. It will make what hooks exist much more discoverable
>>>>>  - Right now there's no list of what postMessages exist within
>>>>> CordovaPlugin.java
>>>>
>>>> I disagree!  I personally prefer this pattern and think that we should
>>>> keep this pattern instead of adding hundreds of hooks that do the same
>>>> thing, because it's easier to test postMessage than it is to test the
>>>> possibly hundreds of hooks.  Does every plugin need it's own special
>>>> hook, or can plugins that extend activities take advantage of
>>>> postMessage and onMessage?  I think that this reduces the API's
>>>> flexibility.  Forcing people to have to request a special hook for
>>>> their plugin is going to suck.  For example, right now the author of
>>>> this issue can work around not having onOptionsMenu working by adding
>>>> the postMessages to his custom activity, and it's a relatively small,
>>>> non-intrusive amount of code.  If we were doing plugin hooks, they'd
>>>> have to change plugin and it could break all the plugins.
>>
>> I don't follow your testing argument. It's trivial to detect if a
>> method is called, even in a test. Yes, it's more code than what's
>> currently in pluginStub, but it's trivial code (override each
>> function, which Eclipse has generators for, and record the call in a
>> variable).
>>
>
> Why do non-menu plugins need to know about Menu events?
They don't. Plugins *optionally* override the hook functions that they
care about. CordovaPlugin isn't an interface, it's a class.


>
>>>>
>>>> I do think all the messages being posted need to be documented, but I
>>>> do think that this is a far more flexible solution than adding tons of
>>>> hooks to Plugins.java and making plugin developers lives suck even
>>>> more.
>>
>> This doesn't add more hooks, it just makes the hooks easier to
>> discover and more convenient to use. By more convenient, I mean:
>>  - Impossible to mess up the types for params
>>  - No need to unpack args in the case of stuffing multiple args
>>  - No need to to string comparisons to see which hook is being
>> triggered. Just override the correct function.
>>
>> All the hooks are just empty functions by default, so this has no
>> effect on plugin maintenance. Making plugin hooks more obvious will
>> make lives better, not worse.
>>
>
>
>> To fix the documentation, we'll be adding a string constant to
>> CordovaPlugin.java for each message that we post, along with comments
>> describing the types of params & return values. I think this is just
>> as verbose as writing stub functions.
>>
>
> One is verbose code, the other is verbosity in the docs.

The extreme example of this is that every class just have a single
function with a single Object parameter + a lot of documentation. My
argument is that code is better than docs.
- Code is checked by the compiler, provides autocomplete, provides type-safety.
- Code organization (no need for a single big multiplexing function)
- You can annotate code with its own comments and @Annotations (e.g.
we could @Deprecate them)


>
>
>>>>
>>>>> 3. Hooks can have multiple parameters
>>>>>  - This would make sense for onScrollChanged and onReceivedError,
>>>>> which use a custom object and a JSONObject to pass multiple params.
>>>>
>>>> I don't think this is a compelling argument, since this will be things
>>>> that I'll have to write tests for.
>>
>> Shouldn't everything have tests? I can write tests for the change. The
>> point of this was that there would be less boxing & unboxing and so
>> should be less code involved instead of more code.
>
> This is more of the same duplicate code.
>
>>
>> In the specific case of scroll events, I think there's also a
>> performance consideration since we shouldn't be allocating a new
>> object for every frame of a scroll.
>>
>
> Seriously? We're dealing with a class that's the Java equivalent of a
> struct.  I'd be way more concerned with any class that used
> JSONObjects instead of a custom class due to the known problems with
> JSON on Android.  onReceivedError is probably more memory inefficient
> in reality.

Sorry, my point with this is that scroll events are
performance-sensitive because they fire on every frame of a scroll. We
don't want to trigger a GC during a scroll.


>
>>>>
>>>>> I actually didn't know half of these hooks existed, so I think even
>>>>> just #1 is really compelling. We'd obviously need to keep delivering
>>>>> these message for backwards-compatibility (except for maybe scroll -
>>>>> it's pretty new)
>>>>
>>>> Honestly, I think that onMessage and postMessage are more flexible and
>>>> are better than the alternative, which is to create hooks for
>>>> everything, which will all have to have their own tests, and which
>>>> will mean even more changes to plugins, which could break plugins and
>>>> make the lives of the people who have to write APIs with our stuff
>>>> suck even more.  This is at least another ten hooks that we're looking
>>>> to add here, perhaps more.  It also means that if someone wants to
>>>> extend this, they'll have to file another issue to get a hook added
>>>> instead of just using postMessage and onMessage.  That's why I think
>>>> this change is bad for the plugin developer, since this makes us even
>>>> more of a bottleneck than we are now.
>>
>> I don't see any risk of breaking plugins in this change. It doesn't
>> create any new test scenarios, but just fiddles how you detect a call
>> to a plugin. This would be a completely backwards compatible change,
>> I'm not proposing any breaking changes.
>>
>> I don't see this as adding new hooks. This is making existing hooks
>> more explicit. To look at it another way, would you advocate changing
>> the current existing explicit functions hooks into onMessage hooks?
>
> I think that this is adding more code for the sake of adding more code.
>
>> I don't think there's any getting around having people file bugs to
>> get new hooks. Plugin developers do not want custom steps required for
>> their plugin to work, which is why even with postMessage, this has
>> been brought up.
>
> Right. In this case, there's a work-around that was done for the code
> to work.  If we didn't have onMessage, there would be no work-around.
Totally with you on this point.

>
> Honestly, this sounds like work for the sake of doing work.  If you
> want to re-write all these hooks and add more code to plugin, as long
> as it doesn't break all the plugins, and as long as the current tests
> are re-written, I'll have to deal with it.  I don't have super strong
> opinions about this change other than the fact that it looks like more
> code, and my gut feeling that adding more code for the sake of adding
> more code is bad.

This isn't high on my priority list (I don't intend to spend time in
the short term doing this), but I thought we should at least get on
the same page in terms of how to add new plugin hooks.


>
> Joe