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 17:12:02 GMT
Thanks guys.  I'll file a bug, and should have a pull request shortly.
 Andrew, for the record, I didn't see anything particular in Jira around
public symbols maintenance, from a cursory search.

Cheers,
Kevin


On Wed, Feb 20, 2013 at 8:42 AM, Andrew Grieve <agrieve@chromium.org> wrote:

> 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