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 <agri...@chromium.org> wrote: > 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 >
