Would definitely be better to expose that property as a protocol. That's the whole point of having the protocol in the first place :P.
I forget if I created a bug for it yet, but cutting down on the number of public symbols is definitely a TODO for both iOS and Android codebases. On Wed, Feb 20, 2013 at 11:17 AM, Kevin Hawkins < kevin.hawkins.cord...@gmail.com> wrote: > Not beyond the twitch, no. :) The current de facto implementation works for > our uses. The latter (header) issue was honestly the first time I'd > noticed, doing a code review where "#import CDVCommandDelegateImpl.h" was > being added to my view controller's .m file, and I didn't understand why. > > I can file and fix it, either way. Would you all prefer the property type > switch, or simply fixing the header imports? The only risk I see around > the property change is the fact that it's public interface, so could > generate warnings (errors?) at compile time, for people upgrading their > existing projects. > > Cheers, > Kevin > > > > On Wednesday, February 20, 2013, Michal Mocny wrote: > > > Thanks for mentioning this. Would you like to file that bug and/or > submit > > a pull request? > > > > Also, do you have some motivating reason for moving to a "more > > protocol-based property"? I understand your twitch, but am curious if > you > > are trying to replace with another implementation? > > > > -Michal > > > > On Tue, Feb 19, 2013 at 7:59 PM, Kevin Hawkins < > > kevin.hawkins.cord...@gmail.com <javascript:;>> wrote: > > > > > Hi all, > > > > > > I'm looking through the CDVViewController implementation details on > iOS, > > > and I'm not clear why its (public) commandDelegate property references > > the > > > concrete implementation class of the CDVCommandDelegate protocol > > > (CDVCommandDelegateImpl), as opposed to defining a more generic > > > protocol-based property. From an object-oriented design standpoint, > > that's > > > something I didn't expect. Is there a reason that this is different > than > > > CDVPlugin's property definition? > > > > > > It's not a super big deal, though it sets off something twitchy in my > > > brain. ;-) What does need to change if it stays as-is, however, is > that > > > CDVViewController.h should be #importing CDVCommandDelegateImpl.h, not > > > CDVCommandDelegate.h. The way it is currently, the responsibility of > the > > > former header's inclusion is being improperly passed on to the > inheriting > > > view controller class. And the latter header file's inclusion is > > > superfluous. > > > > > > I figured I'd get thoughts before filing a bug one way or another. > > > > > > Thanks, > > > Kevin > > > > > >
