[Bug 24656] Improve documentation

[bugzilla-daemon at freedesktop.org]

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





--- Comment #4 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-11-18 10:28:01 PST ---
Partial review up to commit 1120df0 of the rebased branch (stopping just before
channel-dispatch-operation.cpp in the diff):

(In reply to comment #1)
> + * The .client files must contain UTF-8 text with the same syntax as Desktop
> + * Entry files (although the allowed groups, keys and values differ). Every
> + * .client file must contain a group whose name is the name of this interface.
> + * See AbstractClientObserver::observerChannelFilter() for more details.
> 
> Perhaps this should just point into http://telepathy.freedesktop.org/spec/ ?

Not addressed. I think this does need fixing: telepathy-qt4 shouldn't be
duplicating half of the description of how to make a .client file. I'd replace
the three paragraphs "As an optimization, [...] for more details" in \class
AbstractClient with something like this:

As an optimization, service-activatable clients should install a file
$XDG_DATA_DIRS/telepathy/clients/clientname.client containing a cached version
of their immutable properties. The syntax of these files is <a
href="http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Client.html">documented
in the Telepathy D-Bus API Specification</a>.

Similarly, AbstractClientObserver::observerChannelFilter has a lot of redundant
wording stolen from telepathy-spec; I'll suggest something in a subsequent
comment.

In AbstractClientObserver::observeChannels:
> + * The observer must not return from this method call until it is ready for a

This is true on D-Bus but not true in C++. Please say something more like "The
observer must not call MethodInvocationContext::setFinished() until it is ready
for...". If you want a verb to use to mean replying to a D-Bus method call
message, I think "reply" would be more appropriate.

AbstractClientApprover::addDispatchOperation:
> + * method is ready to return and then MethodInvocationContext::setFinished() 

Similarly, please avoid the use of "return" here to mean something that isn't
the C++ "return" keyword.

AbstractClientHandler::handleChannels:
> + * After handleChannels() returns successfully, the client process is considered

Similarly, this isn't a C++ return - use "replies" instead of "returns", or
make it explicit by mentioning setFinished, or even say "replies successfully
by calling MethodInvocationContext::setFinished"?

AbstractClientHandler::wantsRequestNotification:
> + * Return whether this handler wants to receive channel requests
> + * notification via addRequest() and removeRequest().
[...]
> + * This property is set on constructor and cannot be changed after that.

grammar: "notification of channel requests" and "set by the constructor"

AbstractClientHandler::addRequest:
> + * The use of "probably" is because you can't necessarily tell from a channel
> + * request which handler will handle particular channels. A reasonable heuristic
> + * would be to match the request against the handler filter, and respect the
> + * preferred handler (if any).

This rationale isn't relevant to telepathy-qt4, only to the channel dispatcher.
Please remove this paragraph.

AbstractClientHandler::addRequest:
> + * The channel dispatcher should remember which handler was notified, and if
> + * the channel request succeeds, it should dispatch the channels to the expected
> + * handler, unless the channels do not match that handler's filter. If the
> + * channels are not dispatched to the expected handler, the handler that was
> + * expected is notified by the channel dispatcher calling its removeRequest()
> + * method with the %TELEPATHY_ERROR_NOT_YOURS error.
> + *
> + * Expected handling is for the UI to close the window it previously opened.

This is written from the CD's perspective. A better description from the
client's perspective would be:

The channel dispatcher will attempt to ensure that handleChannels() is called
on the same handler that received addRequest(). If that isn't possible,
removeRequest() will be called on the handler that previously received
addRequest(), with the special error %TELEPATHY_ERROR_NOT_YOURS, which
indicates that some other handler received the channel instead.

AbstractClientHandler::addRequest:
> + * Calls to this method are merely a notification.

This is meaningless to the C++ code, please remove it (also the corresponding
statement in removeRequest). In telepathy-spec it's the rationale for not
having a defined error behaviour.

Tp::Account:
> + * The account object provides 3 features that may be enabled in order to access
> + * some methods. For instance to retrieve the account protocol info one need to
> + * call becomeReady() with the feature FeatureProtocolInfo as argument.
> + * The same applies for avatar specific info, where the feature
> + * FeatureAvatar should be used when calling becomeReady().
> + * All other methods either requires FeatureCore to be enabled or can be used
> + * directly.

This looks like a recipe for disaster: we'll add features and fail to update
this paragraph, rendering it untrue. How about this?

To avoid unnecessary D-Bus traffic, some methods only return valid information
after a specific feature has been enabled by calling becomeReady with the
desired set of features as an argument, and waiting for the resulting pending
operation to finish. For instance, to retrieve the account protocol
information, it is necessary to call becomeReady with FeatureProtocolInfo
included in the argument. The required features are documented by each method.


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