cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Hawkins <kevin.hawkins.cord...@gmail.com>
Subject Re: CDVViewController.commandDelegate
Date Wed, 20 Feb 2013 16:17:20 GMT
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.cordova@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
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message