[Bug 23274] Implement Hold interface support

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=23274





--- Comment #3 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-08-13 16:01:42 PST ---
(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.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.