[Bug 24656] Improve documentation

[bugzilla-daemon at freedesktop.org]

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





--- Comment #7 from Andre Moreira Magalhaes <andrunko at gmail.com>  2010-03-30 06:54:29 PST ---
(In reply to comment #4)
> 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>.
Fixed

> 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.
Fixed

> 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.
Fixed

> 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"?
Fixed

> 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"
Fixed

> 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.
Fixed

> 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.
Fixed

> 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.
Fixed

> 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.
Fixed


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