cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: Plugin Hooks on Android
Date Thu, 09 Jan 2014 14:06:21 GMT
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 <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).

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

Mime
View raw message