[Bug 23274] Implement Hold interface support

[bugzilla-daemon at freedesktop.org]

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





--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-08-13 10:43:06 PST ---
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.


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