cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: CDVViewController.commandDelegate
Date Wed, 20 Feb 2013 16:42:31 GMT
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.cordova@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.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