Bug 36881

Andre is working on a quick doc update at the moment, which really comes in handy given the current state of our docs. His branch is at the URL - I'm going to review it now.

As a general remark: Andre, please, try to do moves of lumps of code/comments around for whatever reason in separate commits from the actual changes. Otherwise it's extremely difficult to find out what has changed, what has been removed, and what has just been moved. Thus it was somewhat impossible to review the Account signal documentation changes now, for example.

Account
=======

- * Account adds the following features compared to using
- * Client::AccountManagerInterface directly:
- * <ul>
- *  <li>Status tracking</li>
- *  <li>Getting the list of supported interfaces automatically</li>
- * </ul>
- *

Yeah, these very outdated "additional functionality" are good to remove.

+ * Enabling Account features can be achieved by calling becomeReady()
+ * with the desired set of features as an argument, and waiting for the resulting
+ * PendingOperation to finish, or by enabling the feature in the AccountFactory
+ * used by the AccountManager owning the Account object.

I'd like to at least reverse the order of mentioning becomeReady and the AccountFactory, as using the factory will be more convenient for the bulk of the usecases. Perhaps as a general structure we could use something like "Account features can be enabled by constructing an AccountFactory and enabling the desired features, and passing it to AccountManager or ClientRegistrar when creating them as appropriate. However, if a particular feature is only ever used in a specific circumstance, such as an user opening some settings dialog separate from the general view of the application, features can be later enabled as needed by calling becomeReady with the additional features." ?

This kind of explanation however will be mostly the same for Accounts, Connections and Channels, so perhaps just mention in the Acc/Conn/Chan docs the specific features offered and needed for various portions of the API, and explain e.g. in the "asynchronous object model" page the relationship between the factories and becomeReady?

Speaking of that, even though our time for the doc update now is limited, the aforementioned asynchronous object model page REALLY needs to be either rewritten or temporarily hidden in any case, as it is totally obsolete in the factory age. So please do this as the *highest* priority adjustment to the branch.

Namely, the page should explain the factory concept a bit, and better separate our asynchronous object initialization (readiness) and asynchronous D-Bus method wrapper concepts. Readiness should be explained: you make the objects ready asynchronously so that you can then access (read) the state synchronously and receive change notification. This has nothing to do with further calls to the service, but still currently the page refers to some sort of bogus relationship between the proxy readiness and the asynchronousness needed to call further D-Bus methods, a relationship which doesn't actually exist. (The lowlevel proxies also make asynchronous d-bus calls, even though they have no concept of readiness).

- * \return Read-only pointer to the factory.
+ * \return Read-only pointer to the contact factory used by this account.

Why these changes? In my opinion, they just increase redundancy between the method description and the return value description. The method description should describe what the method does "returns WHAT of WHICH" and the return value description should describe HOW and in WHAT FORM the return value is. Then there is no redundancy - and that was just the case earlier in the docs.

A similar thing:

- * \return The avatar requirements for avatars passed to setAvatar().
+ * \return The avatar requirements for avatars passed to setAvatar() on this account.

It's not a static method. Therefore it's rather self-evident that it pertains to this object. Good documentation needs to be as concise as it can be to avoid the tl;dr; syndrome (but no briefer than that)!

In general, though, I like how you added much-needed cross-referencing to the Account methods, which helps building the big picture.

Small nitpicks:

+ * Some protocols, like XMPP and SIP, are used by various different user-recognised brands,
+ * such as Google Talk and Ovi by Nokia. On accounts for such services, this method can be used

We could remove the mentions to Ovi (here and in the service list) given that http://techland.time.com/2011/05/16/nokia-drops-ovi-brand/ :) I realize this is probably copypasted from the spec, but we don't need to cargo-cult their outdatedness :)

AccountManager
==============

- * The instance will use an account factory creating Tp::Account objects with Account::FeatureCore
- * ready, a connection factory creating Tp::Connection objects with no features ready, and a channel
- * factory creating stock Telepathy-Qt4 channel subclasses, as appropriate, with no features ready.
+ * The instance will use an account factory creating Account objects with Account::FeatureCore
+ * ready, a connection factory creating Connection objects with no features ready, a channel
+ * factory creating stock Channel subclasses, as appropriate, with no features ready, and a contact
+ * factory creating Contact objects with no features ready.

Nice catch adding the Contact factory there. However, I deliberately included the namespace in the documentation here to emphasize that the factories construct that specific class (ours), vs. constructing some subclass, something which the factories can be used to achieve. Did it break the hotlinking or something? If so, couldn't you rather fix it by using #?

In general, I'm not bound to like arbitrary changes to documentation we've written recently (after adding the factories etc.), unless it's adding missing things :) After all, the reasons to write the docs in the way they were a few months ago haven't changed.

- * \return Read-only pointer to the account factory.
+ * \return Read-only pointer to the account factory used by this account manager.

Same complaint as I made for the similar changes in Account: increases redundancy. I'd actually rather even remove the account/connection/channel words - they're redundant with both the method descriptions and the method *names*.

+ * Newly accounts are signaled via newAccount().

that's missing "added and/or discovered", I think.

+ * account manager ready and the same connection, channel and contact factories used by this
+ * account manager.
+ *

as used*

Connection
==========

- * Its basic capability is to provide the facility to request and receive
- * channels of differing types (such as text channels or streaming media
- * channels) which are used to carry out further communication.

VEERY good removal :)

- * \return Read-only pointer to the factory.
+ * \return Read-only pointer to the channel factory used by this connection.

Again...

- * Return the handle which represents the user on this connection, which will
- * remain valid for the lifetime of this connection, or until a change in the
- * user's identifier is signalled by the selfHandleChanged() signal. If the
- * connection is not yet in the ConnectionStatusConnected state, the value of this
+ * Return the handle representing the user on this connection.
+ *
+ * This property will remain valid for the lifetime of this connection, or until a change in the
+ * user's identifier is signalled by the selfHandleChanged() signal.

Good thinking splitting that overtly long "short" description. However, what you split out is now a bit incorrect: the property *does* remain valid forever, that being the point of SelfHandleChanged tracking anyway. The old comment referred specifically to that old *handle* being valid (as in, referenced in the CM and referring to the same (self) contact).

Nowadays with the introduction of immortal handles on most CMs handle validity isn't very interesting anyway. Thus, you could just do away with that validity comment and just mention that there is a change notification signal.

Andre updated his branch. Re-reviewing.

The relationship of factories and becomeReady looks spot on now in the Account and Connection docs!

+/**
+ * \fn QString Channel::GroupMemberChangeDetails::reason() const
+ *
+ * Return the reason for the change, if known.
+ *
+ * \return The reason for the change, or an empty string if the reason is unknown.

The reason is not a string, it's a ChannelGroupChangeReason.

- * if supported, which is indicated by ChannelGroupFlagMessageDepart and/or
- * ChannelGroupFlagMessageReject.
+ * if supported, which is indicated by #ChannelGroupFlagMessageDepart and/or
+ * #ChannelGroupFlagMessageReject.
  *

Good catches! /me should learn to actually generate and see what the docs are like when I'm writing API docs...

  * Cached access to state of the group interface on the associated remote
  * object, if the interface is present. Almost all methods return undefined
  * values if the list returned by interfaces() doesn't include
- * #TELEPATHY_INTERFACE_CHANNEL_INTERFACE_GROUP or if the object is not ready.
+ * #TP_QT4_IFACE_CHANNEL_INTERFACE_GROUP or if the object is not ready.

The accessors returning invalid stuff on non-Group channels hasn't actually almost ever been true, due to the fact that we simulate the Group interface on them.

  * corresponding groupMembersChanged() signal, as the local user being
- * removed usually causes the remote Channel to be closed.
+ * removed usually causes the remote channel to be closed.

I wonder what the "remote" word was, and is doing there.

 * Return whether the value returned by groupSelfContact() is guaranteed to
- * stay synchronized with what groupInterface()->GetSelfHandle() would
+ * stay synchronized with what group interface SelfHandle property would
  * return. Older services not providing group properties don't necessarily
  * emit the SelfHandleChanged signal either, so self contact changes can't be
  * reliably tracked.

The actual Channel.Group D-Bus interface properties are sufficiently obscure to the user to this API that I don't think we should even mention them here. Rather, it should say something like "groupSelfContact() is guaranteed to accurately represent the local user even after nickname changes etc.". We should also stress that this will always be true for a Group channel on any not totally ancient service.


- * Return whether this channel implements the
- * org.freedesktop.Telepathy.Channel.Interface.MergeableConference interface.
+ * Return whether this channel implements the mergeable conference interface.
 
- * Return whether this channel implements the
- * org.freedesktop.Telepathy.Channel.Interface.Splittable interface.
+ * Return whether this channel implements the splittable interface.

Please either continue to mention the actual low-level interface name, or say "supports conference merging" / "supports splitting", and refer to any actual high-level operations we have on the subject. Just saying "the splittable interface" is completely obscure.

+ * \return A pointer to the existing Client::ChannelInterface object for this
+ *         Account object.

Not an Account object!

Thanks for the clear organization of the commits, it made reviewing a breeze!

+ * Construct a info fields instance with the given details. The instance will indicate it's valid.

"that it is valid", and we're not constructing from "details" but fields here.

+ *
+ * See avatar token specific methods' documentation for more details.

I don't think these kinds of comment lines are very helpful (Will agrees on bug 31769). Just what "more details" do the methods convey (and which ones are they! I'm a beginning tp-qt4 programmer and can't tell!)?

If you want to have useful docs for the features, they must refer to the actual accessors which they make to work.

Posting this now so you can work on these for the moment, will continue the review in another comment.

- * Return whether the information for this contact has been received
+ * Return whether the information for this contact has been received.

"information" might be a bit too broad word to use here. Say "info card" or "VCard info" or something?

+ * Start a request that the local user stops receiving presence from this contact.

"request for the local user to stop receiving". Same for the sending part.

+ * Start a request that the local user's presence is sent to this contact
+ * (i.e. that this contact publish attribute becomes Contact::PresenceStateYes).

This rather authorizes their request to see our presence. We can't force anybody to see our presence, a capability which the current description implies.

+ * Start a request to block/unblock this contact (i.e. whether this contact stops receiving presence
+ * from the local user while blocked).

Besides being long for the short description, the sentence in the parentheses is incorrect. What blocking means is protocol-dependent; usually it means at least that they can't send you messages, and try to add you to their contact list. That sentence would be more appropriate for removing from publish.

(Also: bah, I didn't remember we have this bool block = true API here in contact as well. We deprecated it in ContactManager in favor of a clearer blockContacts()/unblockContacts() API. Should have done the same here. Could you add a FIXME please?).

+ * This signal is emitted when this contact is added to the contact list \a group.

A bit awkward, perhaps "added to \a group of the contact list". Same for the removal signal.

+ * emit a signal \link Tp::PendingOperation::finished() \endlink when the operation

emit the signal

+ * Additionally Telepathy-Qt4 introduces a concept model in which object features need

no, not a concept model, just a concept :P

Otherwise the async model changes look good, giving our docs a very important improvement.

Thanks for the review. Branch updated again with all the suggestions applied and some other changes.

Note that some classes docs were updated but not fully, which should be done later.

- * \brief The AbstractClient class represents a Telepathy client.
+ * \brief The AbstractClient class represents a \telepathy client.

- * \brief The AbstractClientObserver class represents a Telepathy observer.
+ * \brief The AbstractClientObserver class represents a \telepathy observer.

- * \brief The AbstractClientApprover class represents a Telepathy approver.
+ * \brief The AbstractClientApprover class represents a \telepathy approver.

- * \brief The AbstractClientHandler class represents a Telepathy handler.
+ * \brief The AbstractClientHandler class represents a \telepathy handler.

- * \brief The AccountManager class represents a Telepathy account manager.
+ * \brief The AccountManager class represents a \telepathy account manager.

... etc

I think these links are completely confusing. Imagine one using a client library to access Telepathy, and examining the docs for implementing e.g. a Handler. Why would they specifically there want to be directed to the Telepathy project front page? IMO we should only have a link to the Tp project front page from the documentation front page / overview description of the entire library.

Something that is actually be useful would be to link "Telepathy Client" -> spec for Client, "Telepathy Observer" -> spec for Client.Observer etc.

To rephrase, when reading documentation for a specific part of the TpQt4 API, you don't suddenly get an urge to go and find out what the Telepathy project is all about; but you might want to know about this specific thing in the Tp spec, or D-Bus documentation, etc.

+ * Telepathy-Qt4 uses \dbus to communicate with applications implementing the \telepathy_spec.

This is fine though, because it's in an overview section where one is likely to want to go to project overviews from.

I realize you probably just searched and replaced all occurrences of the word Telepathy in the docs by a link to the telepathy front page, and would rather not spend time linking to the actually relevant specific parts of the Telepathy spec instead. This is fine; the branch is mergeable as long as you revert your change to link to the Telepathy project front page from 84 API specific sections, and only keep the links in overviews, and the spec links. One just doesn't want to be directed 84 times to an overview of an open-source project!

Otherwise the latest changes, and the branch as a whole, look fine.

I noticed from the links to the spec that some of the doc comments in which they are contained are incorrect:

/**
 * Return a map containing extensible error details related to
 * connectionError().
 *
 * The keys for this map are defined by the \telepathy_spec.
 * They will typically include <literal>debug-message</literal>, which is a debugging
 * message in the C locale.
 *
 * Note that this method may return a different value from the one returned by
 * Connection::errorDetails() on this account connection.
 * See connectionStatus() for more details.
 *
 * This method requires Account::FeatureCore to be enabled.
 *
 * \return A map containing extensible error details related to
 *         connectionError().
 * \sa connectionError(), connectionStatus(), connectionStatusReason(), connectionStatusChanged(),
 *     Connection::ErrorDetails.
 */
Connection::ErrorDetails Account::connectionErrorDetails() const

It doesn't return a map with some well-known keys, it returns a higher-level ErrorDetails object.

- * \brief The ProtocolInfo class represents a Telepathy protocol information.
+ * \brief The ProtocolInfo class represents a \telepathy protocol information.

Still a link to the overview from specific API, and has grammatical issues. Rather, say "represents a Telepathy Protocol". Bonus points for linking to  http://telepathy.freedesktop.org/spec/Protocol.html.

+ * \brief A class representing a Telepathy abstract tube.

There's no such thing as a Telepathy abstract tube. Perhaps "This is the base class for all tube types"?

Tnx for the review, branch merged, will be in tp-qt4 0.6.1 and 0.7.0.