On Thu, Jan 9, 2014 at 6:06 AM, Andrew Grieve <agri...@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 <purplecabb...@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 <bows...@gmail.com> wrote: >>> >>>> On Wed, Jan 8, 2014 at 6:58 PM, Andrew Grieve <agri...@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
