(In reply to comment #0)
> As you know, I have been working on high-level bindings for the Call1 spec. I
> believe that that those bindings are now ready for a first review round.
Great, bellow you can find the first review round. All in all it looks really good.

> This branch contains only high-level bindings for Call. I will do the farstream
> wrapper library in a separate branch.
It would be good to have the farstream bits in place before merging, so it could be released in 0.9.1

> Some open issues/questions in this branch:
> * Should SMChannel and all the related methods be deprecated? Currently it's a
> bit confusing which API to use in classes like Account (ensureAudioCall or
> ensureStreamedMediaAudioCall? that is the question!)
I would not deprecate it for now, lets for it later in 0.9.2 or so as this is the first version of Call and there may be others still using SM. Lets give them a chance to update first.

> * Should Tp::CallStateReason be wrapped into some higher level class? It's a
> bit ugly as it is, although in most cases it's just a useless object. If it is
> wrapped in a high level class, though, how can a Contact be constructed in the
> place of the actor handle without making the emission of all those signals
> async?
IMO we can use CallStateReason as is, no need for a high-level class.

Now the review:

- missing pretty headers for CallContent and CallStream, and the newly added public pending operations.

+++ account.cpp

+        if (!audioName.isEmpty()) {
+            request.insert(TP_QT_IFACE_CHANNEL_TYPE_CALL + QLatin1String(".InitialVideoName"),
+                           audioName);
+        }
s/audioName/videoName/g

- I guess audio/video content name are not that important API wise and may be ignored in some cases and be generated internally? So I believe we should have helper methods overloads in Account that don't take the content name as argument. For all cases.

- Please also update copyright in account.cpp to 2008-2012, we completely forgot to update it, it is still 2008.

- Move PendingCallContent to call-content.h/cpp instead of call-channel.h/cpp.
 
+Features ChannelFactory::featuresForMediaCalls(const QVariantMap &additionalProps) const
+{
+    return featuresFor(ChannelClassSpec::audioCall(additionalProps));
+}
+
+void ChannelFactory::addFeaturesForMediaCalls(const Features &features,
+        const QVariantMap &additionalProps)
+{
+    addFeaturesFor(ChannelClassSpec::audioCall(additionalProps), features);
+    addFeaturesFor(ChannelClassSpec::videoCall(additionalProps), features);
+}
+
+void ChannelFactory::setConstructorForMediaCalls(const ConstructorConstPtr &ctor,
+        const QVariantMap &additionalProps)
+{
+    setConstructorFor(ChannelClassSpec::audioCall(additionalProps), ctor);
+    setConstructorFor(ChannelClassSpec::videoCall(additionalProps), ctor);
+}
+
I guess you want a ChannelClassSpec::mediaCall and use it here instead?

I will have to double check the branch later again before merging as I just skin read it, but as most of the code is from tp-qt-yell which was already reviewed, I guess we will be fine.

Review from my side, besides a ++ to all of Andre's comments. Not really thorough as I did it in a hurry, but still:

+    if (withVideo) {
+        request.insert(TP_QT_IFACE_CHANNEL_TYPE_CALL + QLatin1String(".InitialVideo"),
+                       true);
+        if (!audioName.isEmpty()) {
+            request.insert(TP_QT_IFACE_CHANNEL_TYPE_CALL + QLatin1String(".InitialVideoName"),
+                           audioName);
+        }
+    }

copy/pasta error on videoName vs. audioName

+                self->properties->GetAll(
+                    QLatin1String(TP_QT_IFACE_CHANNEL_TYPE_CALL)),

TP_QT_IFACE_CHANNEL_TYPE_CALL should be a QLatin1String already, no need to recreate one. Please check for other occurrences of that, there are a few.

- I don't get why you have several features which are basically getting all of the properties for the same interface. As long as Content makes some sense since it just pulls a single property, features CallState and CallMembers basically just do the same thing, just storing a different set of properties in the end. Given that the whole purpose of Features is to save DBus roundtrips, wouldn't it make more sense to merge at least State and Members together? (I'd be in favor of merging Contents as well, but there might be a use case for it)

+    return interfaces().contains(TP_QT_IFACE_CALL_CONTENT_INTERFACE_DTMF);

Nitpick: maybe use hasInterface()?

+            Q_ASSERT(mPriv->remoteMembersContacts.contains(handle));

I guess we don't want to assert here


Overall looks really good though :)

(In reply to comment #1)
> - missing pretty headers for CallContent and CallStream, and the newly added
> public pending operations.
> 

CallContent/CallStream headers are there. You added them ;)
PendingCallContent was missing one, fixed now.

> - I guess audio/video content name are not that important API wise and may be
> ignored in some cases and be generated internally? So I believe we should have
> helper methods overloads in Account that don't take the content name as
> argument. For all cases.

The content name is considered significant, but not required, so I made it an optional argument:

    PendingChannelRequest *ensureAudioCall(
            const QString &contactIdentifier,
            const QString &initialAudioContentName = QString(),
            const QDateTime &userActionTime = QDateTime::currentDateTime(),
            const QString &preferredHandler = QString(),
            const ChannelRequestHints &hints = ChannelRequestHints());

Do you think we really need extra overloads without this argument?


> +Features ChannelFactory::featuresForMediaCalls(const QVariantMap
> &additionalProps) const
> +{
> +    return featuresFor(ChannelClassSpec::audioCall(additionalProps));
> +}
> +
> +void ChannelFactory::addFeaturesForMediaCalls(const Features &features,
> +        const QVariantMap &additionalProps)
> +{
> +    addFeaturesFor(ChannelClassSpec::audioCall(additionalProps), features);
> +    addFeaturesFor(ChannelClassSpec::videoCall(additionalProps), features);
> +}
> +
> +void ChannelFactory::setConstructorForMediaCalls(const ConstructorConstPtr
> &ctor,
> +        const QVariantMap &additionalProps)
> +{
> +    setConstructorFor(ChannelClassSpec::audioCall(additionalProps), ctor);
> +    setConstructorFor(ChannelClassSpec::videoCall(additionalProps), ctor);
> +}
> +
> I guess you want a ChannelClassSpec::mediaCall and use it here instead?

I purposefully didn't add a mediCall() method there, as according to the spec a Call1 channel that has neither InitialAudio nor InitialVideo is not valid. I think the public API should only have things that users are safe to use.

Quoting the spec:
"The RequestableChannelClasses for Call1 channels can be:
[( Fixed = { ...ChannelType: ...Call1,
            ...TargetHandleType: Contact,
            ...InitialVideo: True
          },
  Allowed = [ ...InitialVideoName,
              ...InitialAudio,
              ...InitialAudioName
            ]
),
( Fixed = { ...ChannelType: ...Call1,
            ...TargetHandleType: Contact,
            ...InitialAudio: True
          },
  Allowed = [ ...InitialAudioName,
              ...InitialVideo,
              ...InitialVideoName
            ]
)]
Clients aren't allowed to make outgoing calls that have neither initial audio nor initial video"


Btw, now that I look at it again, maybe I should do s/MediaCall/Call/g in the relevant ChannelFactory and ChannelClassSpec methods? We have things like:

bool hasMediaCallInitialAudioFlag() const
bool hasStreamedMediaInitialAudioFlag() const

which is a bit confusing... If it reflected the channel name in the spec, maybe it would be more obvious that the first method is for Call and the other is for SM.

(In reply to comment #2)
> - I don't get why you have several features which are basically getting all of
> the properties for the same interface. As long as Content makes some sense
> since it just pulls a single property, features CallState and CallMembers
> basically just do the same thing, just storing a different set of properties in
> the end. Given that the whole purpose of Features is to save DBus roundtrips,
> wouldn't it make more sense to merge at least State and Members together? (I'd
> be in favor of merging Contents as well, but there might be a use case for it)

FeatureContents should be a separate feature, since it also creates the CallContent and CallStream objects with all their features.

FeatureCallMembers and FeatureCallState maybe... I separated them because they are basically different things. You may want to know the state, but not the members and vice versa. Members maintains internal hash tables with contacts that might cost in memory/cpu if you have a lot of them, but other than that, I guess the only other thing that prevents me from merging them is how to call the combined feature :)

> 
> +            Q_ASSERT(mPriv->remoteMembersContacts.contains(handle));
> 
> I guess we don't want to assert here

Why not? It's a condition that should be met at this point. Saves us from accidentally  creating a null ContactPtr without noticing (when calling mPriv->remoteMembersContacts[handle] below).

(In reply to comment #3)
> (In reply to comment #1)
> > - missing pretty headers for CallContent and CallStream, and the newly added
> > public pending operations.
> > 
> 
> CallContent/CallStream headers are there. You added them ;)
> PendingCallContent was missing one, fixed now.
heh, I am getting crazy, it is fine now :P
 
> > - I guess audio/video content name are not that important API wise and may be
> > ignored in some cases and be generated internally? So I believe we should have
> > helper methods overloads in Account that don't take the content name as
> > argument. For all cases.
> 
> The content name is considered significant, but not required, so I made it an
> optional argument:
> 
>     PendingChannelRequest *ensureAudioCall(
>             const QString &contactIdentifier,
>             const QString &initialAudioContentName = QString(),
>             const QDateTime &userActionTime = QDateTime::currentDateTime(),
>             const QString &preferredHandler = QString(),
>             const ChannelRequestHints &hints = ChannelRequestHints());
> 
> Do you think we really need extra overloads without this argument?
No, that is ok.

> 
> > +Features ChannelFactory::featuresForMediaCalls(const QVariantMap
> > &additionalProps) const
> > +{
> > +    return featuresFor(ChannelClassSpec::audioCall(additionalProps));
> > +}
> > +
> > +void ChannelFactory::addFeaturesForMediaCalls(const Features &features,
> > +        const QVariantMap &additionalProps)
> > +{
> > +    addFeaturesFor(ChannelClassSpec::audioCall(additionalProps), features);
> > +    addFeaturesFor(ChannelClassSpec::videoCall(additionalProps), features);
> > +}
> > +
> > +void ChannelFactory::setConstructorForMediaCalls(const ConstructorConstPtr
> > &ctor,
> > +        const QVariantMap &additionalProps)
> > +{
> > +    setConstructorFor(ChannelClassSpec::audioCall(additionalProps), ctor);
> > +    setConstructorFor(ChannelClassSpec::videoCall(additionalProps), ctor);
> > +}
> > +
> > I guess you want a ChannelClassSpec::mediaCall and use it here instead?
> 
> I purposefully didn't add a mediCall() method there, as according to the spec a
> Call1 channel that has neither InitialAudio nor InitialVideo is not valid. I
> think the public API should only have things that users are safe to use.
> 
> Quoting the spec:
> "The RequestableChannelClasses for Call1 channels can be:
> [( Fixed = { ...ChannelType: ...Call1,
>             ...TargetHandleType: Contact,
>             ...InitialVideo: True
>           },
>   Allowed = [ ...InitialVideoName,
>               ...InitialAudio,
>               ...InitialAudioName
>             ]
> ),
> ( Fixed = { ...ChannelType: ...Call1,
>             ...TargetHandleType: Contact,
>             ...InitialAudio: True
>           },
>   Allowed = [ ...InitialAudioName,
>               ...InitialVideo,
>               ...InitialVideoName
>             ]
> )]
> Clients aren't allowed to make outgoing calls that have neither initial audio
> nor initial video"
Ok then, makes sense.

> 
> Btw, now that I look at it again, maybe I should do s/MediaCall/Call/g in the
> relevant ChannelFactory and ChannelClassSpec methods? We have things like:
> 
> bool hasMediaCallInitialAudioFlag() const
> bool hasStreamedMediaInitialAudioFlag() const
> 
> which is a bit confusing... If it reflected the channel name in the spec, maybe
> it would be more obvious that the first method is for Call and the other is for
> SM.
I agree, I think we can remove "Media" from the names there.

Please add Farstream support and comment here and I will do a final review. The branch is looking really good.

(In reply to comment #4)
> (In reply to comment #2)
> > - I don't get why you have several features which are basically getting all of
> > the properties for the same interface. As long as Content makes some sense
> > since it just pulls a single property, features CallState and CallMembers
> > basically just do the same thing, just storing a different set of properties in
> > the end. Given that the whole purpose of Features is to save DBus roundtrips,
> > wouldn't it make more sense to merge at least State and Members together? (I'd
> > be in favor of merging Contents as well, but there might be a use case for it)
> 
> FeatureContents should be a separate feature, since it also creates the
> CallContent and CallStream objects with all their features.
> 
> FeatureCallMembers and FeatureCallState maybe... I separated them because they
> are basically different things. You may want to know the state, but not the
> members and vice versa. Members maintains internal hash tables with contacts
> that might cost in memory/cpu if you have a lot of them, but other than that, I
> guess the only other thing that prevents me from merging them is how to call
> the combined feature :)
Features are used exactly to avoid unnecessary D-Bus round-trips. As you said, users may want to just check the call state, not caring about members, etc. So in this case I would keep them as separated features.

(In reply to comment #4)
> (In reply to comment #2)
> 
> > 
> > +            Q_ASSERT(mPriv->remoteMembersContacts.contains(handle));
> > 
> > I guess we don't want to assert here
> 
> Why not? It's a condition that should be met at this point. Saves us from
> accidentally  creating a null ContactPtr without noticing (when calling
> mPriv->remoteMembersContacts[handle] below).

Sure, my point is that instead of asserting we should return an error - we don't want the application to crash if shit happens.

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > - I don't get why you have several features which are basically getting all of
> > > the properties for the same interface. As long as Content makes some sense
> > > since it just pulls a single property, features CallState and CallMembers
> > > basically just do the same thing, just storing a different set of properties in
> > > the end. Given that the whole purpose of Features is to save DBus roundtrips,
> > > wouldn't it make more sense to merge at least State and Members together? (I'd
> > > be in favor of merging Contents as well, but there might be a use case for it)
> > 
> > FeatureContents should be a separate feature, since it also creates the
> > CallContent and CallStream objects with all their features.
> > 
> > FeatureCallMembers and FeatureCallState maybe... I separated them because they
> > are basically different things. You may want to know the state, but not the
> > members and vice versa. Members maintains internal hash tables with contacts
> > that might cost in memory/cpu if you have a lot of them, but other than that, I
> > guess the only other thing that prevents me from merging them is how to call
> > the combined feature :)
> Features are used exactly to avoid unnecessary D-Bus round-trips. As you said,
> users may want to just check the call state, not caring about members, etc. So
> in this case I would keep them as separated features.

Actually Dario is right, with the extra feature we do one more D-Bus round-trip and we only save application resources.

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #2)
> > 
> > > 
> > > +            Q_ASSERT(mPriv->remoteMembersContacts.contains(handle));
> > > 
> > > I guess we don't want to assert here
> > 
> > Why not? It's a condition that should be met at this point. Saves us from
> > accidentally  creating a null ContactPtr without noticing (when calling
> > mPriv->remoteMembersContacts[handle] below).
> 
> Sure, my point is that instead of asserting we should return an error - we
> don't want the application to crash if shit happens.

No, if you look carefully at the (complex) code of this function, you will notice that this assertion can never be false, hence it is reasonable for it to be an assertion. It's just there to prevent us from messing up with the code of this function.

Branch updated with fixes from this review + tp-farstream bindings

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (In reply to comment #2)
> > > > - I don't get why you have several features which are basically getting all of
> > > > the properties for the same interface. As long as Content makes some sense
> > > > since it just pulls a single property, features CallState and CallMembers
> > > > basically just do the same thing, just storing a different set of properties in
> > > > the end. Given that the whole purpose of Features is to save DBus roundtrips,
> > > > wouldn't it make more sense to merge at least State and Members together? (I'd
> > > > be in favor of merging Contents as well, but there might be a use case for it)
> > > 
> > > FeatureContents should be a separate feature, since it also creates the
> > > CallContent and CallStream objects with all their features.
> > > 
> > > FeatureCallMembers and FeatureCallState maybe... I separated them because they
> > > are basically different things. You may want to know the state, but not the
> > > members and vice versa. Members maintains internal hash tables with contacts
> > > that might cost in memory/cpu if you have a lot of them, but other than that, I
> > > guess the only other thing that prevents me from merging them is how to call
> > > the combined feature :)
> > Features are used exactly to avoid unnecessary D-Bus round-trips. As you said,
> > users may want to just check the call state, not caring about members, etc. So
> > in this case I would keep them as separated features.
> 
> Actually Dario is right, with the extra feature we do one more D-Bus round-trip
> and we only save application resources.

Care to elaborate? I don't see how this is true. With FeatureCallState you just need to download some properties and that is it. While with FeatureCallMembers you need to download properties and build Contact objects (that requires D-Bus round trips). Also CallStateChanged/CallMembersChanged is only connected when needed with separate features.
So if the user only cares about the state, it will certainly do a lot of extra unnecessary D-Bus round trips for example.

So please keep it separated.

Btw, now that I saw it, please use PendingVariantMap *Interface::requestAllProperties() instead of Properties::GetAll

(In reply to comment #11)
> Care to elaborate? I don't see how this is true. With FeatureCallState you just
> need to download some properties and that is it. While with FeatureCallMembers
> you need to download properties and build Contact objects (that requires D-Bus
> round trips). Also CallStateChanged/CallMembersChanged is only connected when
> needed with separate features.
> So if the user only cares about the state, it will certainly do a lot of extra
> unnecessary D-Bus round trips for example.
> 
> So please keep it separated.

True, I didn't think of the Contact objects. Subject closed.

> 
> Btw, now that I saw it, please use PendingVariantMap
> *Interface::requestAllProperties() instead of Properties::GetAll

Fixed.

Ok, here goes another review round.

There is just one issue apart from some nitpickings (below) that I believe we should really fix before merging.
CallChannel is parsing immutable props on construction, but if the channel is constructed directly without passing immutable props (in which case it should still work), some accessors will be broken, such as initialTransportType(). So please, add a proper FeatureCore (not just an alias for Channel::FeatureCore) that will check if some immutable prop is missing and if so, retrieve them via dbus. Check Channel::Private::introspectMainProperties for reference.

Some nitpicking:

diff --git a/CMakeLists.txt b/CMakeLists.txt
+set(FARSTREAM_MIN_VERSION "0.1.0")
+find_package(Farstream)
+macro_log_feature(FARSTREAM_FOUND "Farstream"
+                  "A Framework for dealing with audio/video conferencing protocols"
+                  "http://www.freedesktop.org/wiki/Software/Farstream" FALSE "0.1.0"
+                  "Needed, together with GStreamer and Telepathy-Farstream, to build telepathy-qt-farstream")
Please use FARSTREAM_MIN_VERSION in place of "0.1.0" here so we don't forget to update when we bump the required dependencies.

+# Find tp-farsight
+set(TELEPATHY_FARSTREAM_MIN_VERSION "0.2.2")
+find_package(TelepathyFarstream)
+macro_log_feature(TELEPATHYFARSTREAM_FOUND "Telepathy-Farstream"
+                  "A Framework for dealing with audio/video conferencing protocols"
+                  "http://telepathy.freedesktop.org/wiki/" FALSE "0.2.2"
Same here

diff --git a/TelepathyQt/Farstream/CMakeLists.txt b/TelepathyQt/Farstream/CMakeLists.txt
+    # We are building Telepathy-Qt4-Yell-Farstream
*Telepathy-Qt

diff --git a/TelepathyQt/Farstream/TelepathyQtFarstream.pc.in b/TelepathyQt/Farstream/TelepathyQtFarstream.pc.in
{QT_MIN_VERSION}, QtDBus < ${QT_MAX_VERSION}, telepathy-glib >= 0.17.5, telepathy-farstream >= 0.2.0, TelepathyQt${QT_VERSION_MAJOR} = ${PACKAGE_VERSION}
Please use macros for tp-glib/farstream versions. I noted also that we have hardcoded version in TpFarsight.pc.in, it would be good if you could also update it.
The same happens in the uninstalled versions (btw, do we really need them when using cmake?). Tbh I guess we could just remove the uninstalled versions as this in theory should be used by libtool only.

diff --git a/TelepathyQt/Farstream/channel.cpp b/TelepathyQt/Farstream/channel.cpp
+namespace Tp {
+namespace Farstream {
\n{. Please also replace all other similar occurrences.

+    if (!channel->handlerStreamingRequired()) {
+        setFinishedWithError(TP_QT_ERROR_NOT_AVAILABLE,
+                QLatin1String("Handler streaming not required"));
+        return;
+    }
Please add some debug warnings here also and in other early-returns.

+CallChannelPtr PendingChannel::callChannel() const
+{
+    return CallChannelPtr::staticCast(object());
Please use qObjectCast here.

diff --git a/TelepathyQt/channel-factory.h b/TelepathyQt/channel-factory.h
There are some extra blank lines there that could be removed.

Apart from those, excellent work, feel free to merge once the above issues are addressed.

(In reply to comment #13)
> CallChannel is parsing immutable props on construction, but if the channel is
> constructed directly without passing immutable props (in which case it should
> still work), some accessors will be broken, such as initialTransportType(). So
> please, add a proper FeatureCore (not just an alias for Channel::FeatureCore)
> that will check if some immutable prop is missing and if so, retrieve them via
> dbus. Check Channel::Private::introspectMainProperties for reference.

I had the code like this in the beggining, but then thought that if immutable props are passed in the constructor, it doesn't need to do the extra dbus round-trip. This constructor argument seems a bit confusing... Anyway, I reverted the commit and updated it.

> diff --git a/TelepathyQt/Farstream/TelepathyQtFarstream.pc.in
> b/TelepathyQt/Farstream/TelepathyQtFarstream.pc.in
> {QT_MIN_VERSION}, QtDBus < ${QT_MAX_VERSION}, telepathy-glib >= 0.17.5,
> telepathy-farstream >= 0.2.0, TelepathyQt${QT_VERSION_MAJOR} =
> ${PACKAGE_VERSION}
> Please use macros for tp-glib/farstream versions. I noted also that we have
> hardcoded version in TpFarsight.pc.in, it would be good if you could also
> update it.
> The same happens in the uninstalled versions (btw, do we really need them when
> using cmake?). Tbh I guess we could just remove the uninstalled versions as
> this in theory should be used by libtool only.

I don't really know. I've never seen uninstalled .pc files in use. The truth is that no cmake project uses them, but then again most cmake projects don't install pkgconfig files anyway.

> Apart from those, excellent work, feel free to merge once the above issues are
> addressed.

Sorry, as I said in irc, I don't have permissions to merge...

(In reply to comment #14)
> > The same happens in the uninstalled versions (btw, do we really need them when
> > using cmake?). Tbh I guess we could just remove the uninstalled versions as
> > this in theory should be used by libtool only.

"uninstalled" .pc files are nothing to do with libtool. They are non-essential, but seem to be equally useful whether your library is built with Autotools or CMake. Their purpose is to let you satisfy a pkg-config dependency with a source/build tree that you've compiled but not installed.

See: <http://smcv.pseudorandom.co.uk/2008/09/pc-uninstalled/>

(In reply to comment #15)
> (In reply to comment #14)
> > > The same happens in the uninstalled versions (btw, do we really need them when
> > > using cmake?). Tbh I guess we could just remove the uninstalled versions as
> > > this in theory should be used by libtool only.
> 
> "uninstalled" .pc files are nothing to do with libtool. They are non-essential,
> but seem to be equally useful whether your library is built with Autotools or
> CMake. Their purpose is to let you satisfy a pkg-config dependency with a
> source/build tree that you've compiled but not installed.
> 
> See: <http://smcv.pseudorandom.co.uk/2008/09/pc-uninstalled/>

hmm ok, I thought it was something specific to libtool/autotools. Anyway we don't need them as there is no pkg-config dependency to be solved in source/build tree that is required here. I will leave them for now and probably remove in a later version.

(In reply to comment #14)
> (In reply to comment #13)
> > CallChannel is parsing immutable props on construction, but if the channel is
> > constructed directly without passing immutable props (in which case it should
> > still work), some accessors will be broken, such as initialTransportType(). So
> > please, add a proper FeatureCore (not just an alias for Channel::FeatureCore)
> > that will check if some immutable prop is missing and if so, retrieve them via
> > dbus. Check Channel::Private::introspectMainProperties for reference.
> 
> I had the code like this in the beggining, but then thought that if immutable
> props are passed in the constructor, it doesn't need to do the extra dbus
> round-trip. This constructor argument seems a bit confusing... Anyway, I
> reverted the commit and updated it.
Sorry if I wasn't clear here. What I want you to do is to parse the immutable props on construction and only if there is something missing call GetAll. Your introspectCore should check if all props were already passed on construction and if so, just set FeatureCore as ready, if not request the props as you did.

(In reply to comment #17)
> Sorry if I wasn't clear here. What I want you to do is to parse the immutable
> props on construction and only if there is something missing call GetAll. Your
> introspectCore should check if all props were already passed on construction
> and if so, just set FeatureCore as ready, if not request the props as you did.

Got it. Fixed now.

Thanks again for the great work. Merged upstream. It will be in the next release 0.9.1