On Thu, Jan 9, 2014 at 9:25 AM, Joe Bowser <bows...@gmail.com> wrote: > 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. My reasons were listed below.
> >> 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? 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
