Bug 23274

I'd prefer the hold state to be called localHoldState, like it is in the D-Bus API: the remote hold state is per-contact, is part of CallStates, and does not need pending states.

The docs for requestHold are inappropriate:

+ * If the connection manager can immediately tell that the requested state
+ * change could not possibly succeed, the resulting PendingOperation SHOULD fail
+ * with error code %TELEPATHY_ERROR_NOT_AVAILABLE.

SHOULD refers to the CM implementation. We're not implementing a CM, so say "will fail with".

+ * If the requested state is the same as the current state, the resulting
+ * PendingOperation SHOULD finish successfully.

"will finish successfully"

+ * Otherwise, this method SHOULD immediately set the hold state to
+ * Tp::LocalHoldStatePendingHold or Tp::LocalHoldStatePendingUnhold (as
+ * appropriate), emitting holdStateChanged() if this is a change, and the
+ * resulting PendingOperation should finish successfully.

Again, this is described (in the spec) from the CM developer's perspective. You need to describe it from the client developer's perspective. In particular, "immediately" is wrong here.

"Otherwise, the channel's hold state will change to Tp::LocalHoldStatePendingHold or Tp::LocalHoldStatePendingUnhold (as appropriate), then the PendingOperation will finish successfully" perhaps?

+ * If the channel has multiple streams, and the connection manager succeeds in
+ * changing the hold state of one stream but fails to change the hold state of
+ * another, it SHOULD attempt to revert all streams to their previous hold
+ * states.

SHOULD -> will

+ * If the channel does not support the %TELEPATHY_INTERFACE_CHANNEL_INTERFACE_HOLD
+ * interface, the resulting PendingOperation will fail with error code
+ * %TELEPATHY_ERROR_NOT_IMPLEMENTED.

I think the word "resulting" just obscures the meaning here (we already said it, so "the PendingOperation" is enough). Remove it.

+        // should we fail here or just ignore?
+        mPriv->readinessHelper->setIntrospectCompleted(FeatureHoldState,
+                false, reply.error());

Better to DEBUG() << "ignoring error getting hold state and assuming we're not on hold: " << reply.error().message() (and then do so, obviously), I think.

The simulation of Hold and the tests look reasonable. I'd also like to see a simulation of inability to reacquire a resource (which might look like this: HELD -> PENDING_UNHOLD -> PENDING_HOLD -> HELD, as described by the spec).

Where's your telepathy-glib branch adding this to the example in telepathy-glib? That should have come first :-P

(In reply to comment #1)
> I'd prefer the hold state to be called localHoldState, like it is in the D-Bus
> API: the remote hold state is per-contact, is part of CallStates, and does not
> need pending states.
The D-Bus API says GetHoldState, but yeah, I changed it to localHoldState and localHoldStateReason. I also changed the Feature to FeatureLocalHoldState and the request method to requestLocalHold.

> The docs for requestHold are inappropriate:
> 
> + * If the connection manager can immediately tell that the requested state
> + * change could not possibly succeed, the resulting PendingOperation SHOULD
> fail
> + * with error code %TELEPATHY_ERROR_NOT_AVAILABLE.
> 
> SHOULD refers to the CM implementation. We're not implementing a CM, so say
> "will fail with".
Fixed

> + * If the requested state is the same as the current state, the resulting
> + * PendingOperation SHOULD finish successfully.
> 
> "will finish successfully"
Fixed
 
> + * Otherwise, this method SHOULD immediately set the hold state to
> + * Tp::LocalHoldStatePendingHold or Tp::LocalHoldStatePendingUnhold (as
> + * appropriate), emitting holdStateChanged() if this is a change, and the
> + * resulting PendingOperation should finish successfully.
> 
> Again, this is described (in the spec) from the CM developer's perspective. You
> need to describe it from the client developer's perspective. In particular,
> "immediately" is wrong here.
> 
> "Otherwise, the channel's hold state will change to
> Tp::LocalHoldStatePendingHold or Tp::LocalHoldStatePendingUnhold (as
> appropriate), then the PendingOperation will finish successfully" perhaps?
Fixed
 
> + * If the channel has multiple streams, and the connection manager succeeds in
> + * changing the hold state of one stream but fails to change the hold state of
> + * another, it SHOULD attempt to revert all streams to their previous hold
> + * states.
> 
> SHOULD -> will
Fixed
 
> + * If the channel does not support the
> %TELEPATHY_INTERFACE_CHANNEL_INTERFACE_HOLD
> + * interface, the resulting PendingOperation will fail with error code
> + * %TELEPATHY_ERROR_NOT_IMPLEMENTED.
> 
> I think the word "resulting" just obscures the meaning here (we already said
> it, so "the PendingOperation" is enough). Remove it.
> 
> +        // should we fail here or just ignore?
> +        mPriv->readinessHelper->setIntrospectCompleted(FeatureHoldState,
> +                false, reply.error());
> 
> Better to DEBUG() << "ignoring error getting hold state and assuming we're not
> on hold: " << reply.error().message() (and then do so, obviously), I think.
Fixed 

Also changed StreamedMediaChannel to just emit localHoldStateChanged when it actually changed.

(In reply to comment #2)
> The simulation of Hold and the tests look reasonable. I'd also like to see a
> simulation of inability to reacquire a resource (which might look like this:
> HELD -> PENDING_UNHOLD -> PENDING_HOLD -> HELD, as described by the spec).
Done

> Where's your telepathy-glib branch adding this to the example in
> telepathy-glib? That should have come first :-P
Here it is http://git.collabora.co.uk/?p=user/andrunko/telepathy-glib.git;a=shortlog;h=refs/heads/callable-hold :D. Should I file a bug report for this?

(In reply to comment #4)
> (In reply to comment #2)
> > The simulation of Hold and the tests look reasonable. I'd also like to see a
> > simulation of inability to reacquire a resource (which might look like this:
> > HELD -> PENDING_UNHOLD -> PENDING_HOLD -> HELD, as described by the spec).
> Done
> 
> > Where's your telepathy-glib branch adding this to the example in
> > telepathy-glib? That should have come first :-P
> Here it is
> http://git.collabora.co.uk/?p=user/andrunko/telepathy-glib.git;a=shortlog;h=refs/heads/callable-hold
> :D. Should I file a bug report for this?

No need.

I have three coding-style complaints:

* this is C (and isn't C99), so don't use C++ // comments
* this is C (and isn't C99), so leave a blank line between initialized declarations and other statements, to make it easier to avoid interleaving declarations and statements
* there should be blank lines around blocks (if/while/...) anyway, for clarity

(Portability to e.g. Windows suffers if you rely on C99 in C code.)

I'll fix these myself and get someone else to insta-review, since they're just comments/whitespace changes.

Code looks fine. One documentation adjustment needed, and optionally one API tweak:

I think it would be useful to change the docstring for localHoldState like this:

>- * Return the channel local hold state.
>+ * Return whether the local user has placed this call on hold.
>  *
>  * This method requires StreamedMediaChannel::FeatureHoldState to be enabled.
>  *
>  * \return The channel local hold state.
>  * \sa requestLocalHold()
>  */
> LocalHoldState StreamedMediaChannel::holdState() const
> LocalHoldState StreamedMediaChannel::localHoldState() const

Similarly, localHoldStateReason could say "the reason why localHoldState() changed to its current value", or something, as its one-liner.

Otherwise, the docstring is no more helpful than the method name! I know it's very easy to document methods like "ExampleWindow::accountCreationWidget(): return the account creation widget", but that sort of thing looks rather as though the author has grudgingly documented things in order to comply with mandatory coding standards, rather than thinking about how to make the docs useful (for some reason, in my mind this conjures up images of javadoc, but I'm sure there are similar offenders in every language).

Even in cases where the method's functionality is entirely obvious, you can often make the docs seem less mechanically-generated by turning it into a phrase like "return the window's account creation widget", or better, "return a widget that can be used to create accounts". Bonus points if your text can suggest concisely why the library user might want to call this function.

(In reply to comment #3)
> The D-Bus API says GetHoldState, but yeah, I changed it to localHoldState and
> localHoldStateReason. I also changed the Feature to FeatureLocalHoldState and
> the request method to requestLocalHold.

I think FeatureLocalHoldState is a good change too, yes.

I'm not so sure about requestLocalHold - what other hold state could we possibly be requesting? "Local" is somewhat redundant here, since whether the other guy has put the call on hold is clearly not our decision - all we can possibly hope to change by our request is whether *we* have the call on hold.

Keep it as-is if you value consistency over conciseness, though.

Please reinstate the patch keyword when this is ready for review again.

(In reply to comment #6)
> Code looks fine. One documentation adjustment needed, and optionally one API
> tweak:
> 
> I think it would be useful to change the docstring for localHoldState like
> this:
> 
> >- * Return the channel local hold state.
> >+ * Return whether the local user has placed this call on hold.
> >  *
> >  * This method requires StreamedMediaChannel::FeatureHoldState to be enabled.
> >  *
> >  * \return The channel local hold state.
> >  * \sa requestLocalHold()
> >  */
> > LocalHoldState StreamedMediaChannel::holdState() const
> > LocalHoldState StreamedMediaChannel::localHoldState() const
> 
> Similarly, localHoldStateReason could say "the reason why localHoldState()
> changed to its current value", or something, as its one-liner.
> 
> Otherwise, the docstring is no more helpful than the method name! I know it's
> very easy to document methods like "ExampleWindow::accountCreationWidget():
> return the account creation widget", but that sort of thing looks rather as
> though the author has grudgingly documented things in order to comply with
> mandatory coding standards, rather than thinking about how to make the docs
> useful (for some reason, in my mind this conjures up images of javadoc, but I'm
> sure there are similar offenders in every language).
> 
> Even in cases where the method's functionality is entirely obvious, you can
> often make the docs seem less mechanically-generated by turning it into a
> phrase like "return the window's account creation widget", or better, "return a
> widget that can be used to create accounts". Bonus points if your text can
> suggest concisely why the library user might want to call this function.
Done. Completely agree here, I will try to write more useful docs next time. I am not good at writing docs but I will try my best :)

> (In reply to comment #3)
> > The D-Bus API says GetHoldState, but yeah, I changed it to localHoldState and
> > localHoldStateReason. I also changed the Feature to FeatureLocalHoldState and
> > the request method to requestLocalHold.
> 
> I think FeatureLocalHoldState is a good change too, yes.
> 
> I'm not so sure about requestLocalHold - what other hold state could we
> possibly be requesting? "Local" is somewhat redundant here, since whether the
> other guy has put the call on hold is clearly not our decision - all we can
> possibly hope to change by our request is whether *we* have the call on hold.
> 
> Keep it as-is if you value consistency over conciseness, though.
Done. Reverted to requestHold.
 

Ship it.

Fixed upstream, will be in next release 0.1.10