Plugin Hooks on Android

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

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
2. It will make hook parameters typed
  - Right now you need to cast the second parameter of postMessage to
use it (e.g. to Menu, MenuItem, ScrollEvent, String, JSONObject).
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 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)

Re: Plugin Hooks on Android

[Brian LeRoux <b...@brian.io>]

I personally don't care if we want to go all traditional Java on the
codebase to 'win' code completion,etc but I do question if this is a real
priority issue.

Aside: I think we need to do a hangout. (We should have one anytime email
word count goes above one scroll.)


On Thu, Jan 9, 2014 at 10:42 AM, Andrew Grieve <ag...@chromium.org> wrote:

> On Thu, Jan 9, 2014 at 9:25 AM, Joe Bowser <bo...@gmail.com> wrote:
> > On Thu, Jan 9, 2014 at 6:06 AM, Andrew Grieve <ag...@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 <pu...@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 <bo...@gmail.com> wrote:
> >>>>
> >>>>> On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <ag...@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
>

Re: Plugin Hooks on Android

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

On Thu, Jan 9, 2014 at 9:25 AM, Joe Bowser <bo...@gmail.com> wrote:
> On Thu, Jan 9, 2014 at 6:06 AM, Andrew Grieve <ag...@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 <pu...@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 <bo...@gmail.com> wrote:
>>>>
>>>>> On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <ag...@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

Re: Plugin Hooks on Android

[Joe Bowser <bo...@gmail.com>]

On Thu, Jan 9, 2014 at 6:06 AM, Andrew Grieve <ag...@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.

> On Wed, Jan 8, 2014 at 9:40 PM, purplecabbage <pu...@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 <bo...@gmail.com> wrote:
>>>
>>>> On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <ag...@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?

>>>
>>> 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.


>>>
>>>> 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.

>>>
>>>> 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.

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.

Joe

Re: Plugin Hooks on Android

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

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.

On Wed, Jan 8, 2014 at 9:40 PM, purplecabbage <pu...@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 <bo...@gmail.com> wrote:
>>
>>> On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <ag...@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).

>>
>> 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.

>>
>>> 2. It will make hook parameters typed
>>>  - Right now you need to cast the second parameter of postMessage to
>>> use it (e.g. to Menu, MenuItem, ScrollEvent, String, JSONObject).
>>
>> Why do they need to be typed?  The plugin author should know what
>> they're expecting back if they're listening to the message that's
>> being passed.  Also, how do you deal with multiple plugins that do the
>> same thing?

Basically, the same reason we type anything. Less room for error, no
need for explicit cast.

Multiple plugins that do the same thing are handled the same way they
are now. It's up to PluginManager to decide whether all plugins should
be called, or just call until the first one returns something.

>>
>>> 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.

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.


>>
>>> 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 agree we shouldn't remove onMessage / postMessage. But I don't think
we lose any flexibility by making the events that Cordova itself fires
into explicit functions. There's no current flexibility with them
since we can't change their semantics without breaking users of them.

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 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.

Re: Plugin Hooks on Android

[purplecabbage <pu...@gmail.com>]

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 <bo...@gmail.com> wrote:
> 
>> On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <ag...@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 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.
> 
>> 2. It will make hook parameters typed
>>  - Right now you need to cast the second parameter of postMessage to
>> use it (e.g. to Menu, MenuItem, ScrollEvent, String, JSONObject).
> 
> Why do they need to be typed?  The plugin author should know what
> they're expecting back if they're listening to the message that's
> being passed.  Also, how do you deal with multiple plugins that do the
> same thing?
> 
>> 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.
> 
>> 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.

Re: Plugin Hooks on Android

[Joe Bowser <bo...@gmail.com>]

On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <ag...@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 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.

> 2. It will make hook parameters typed
>   - Right now you need to cast the second parameter of postMessage to
> use it (e.g. to Menu, MenuItem, ScrollEvent, String, JSONObject).

Why do they need to be typed?  The plugin author should know what
they're expecting back if they're listening to the message that's
being passed.  Also, how do you deal with multiple plugins that do the
same thing?

> 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.

> 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.