Bug 24656

Up to commit bfd6bb0bc6:

+ * \brief The AbstractClient class provides an object representing a
+ * <a href="http://telepathy.freedesktop.org">Telepathy</a> client.

Do we really need to hyperlink Telepathy? I think we can assume that Telepathy users know where the website is from the README!

+ * 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/ ?

+ * The easiest way to create account objects is trough AccountManager. One can

through

+ * Another way to create an Account object is to just call the create method.

Could you expand on this a little to make it clear that ValidAccounts is the list of stored accounts, and create() adds a new one? At the moment it reads as though this is some sort of alternative API for the same thing.

+ * See avatar specific methods documentation for more details.

avatar-specific methods' documentation

+ *  <li>A fake group implementation when handle type is different from
+ *  HandleTypeContact</li>

when the handle type *is* HandleTypeContact, surely?

+ * undefined until the introspection process is completed; isReady() returns true

until the introspection process is completed, i.e. isReady() returns true

(this text exists in several places)

+ * Channel object will transition to closed too.

I think we should document this in terms of invalidated - we want to encourage people to see invalidated as "the" this-object-is-dead notification.

+ * As a result, approvers should use the first handler by default.

... by default, unless they have a reason to do otherwise.

You seem to be copying <tp:rationale> into these docstrings in a fairly indiscriminate way. I don't think that's necessarily wise - the rationale is to explain the spec, and not all of it needs to be in API docs (for that matter, not everything from the spec needs to be in these docs - we can reference the spec normatively). "More is better" is not necessarily true...

In particular, in PendingOperation *ChannelDispatchOperation::claim(), the rationale seems very much out of place.

+ * A channel has closed before it could be claimed or handled. If this is
+ * emitted for the last remaining channel in a channel dispatch operation, it
+ * must immediately be followed by invalidated() with error
+ * %TELEPATHY_QT4_ERROR_OBJECT_REMOVED.

s/must/will/ to get this into the client's perspective, as usual

+ * This corresponds to the _NET_WM_USER_TIME property in EWMH.

(Unix developers: this corresponds ... in EWMH.)

Ideally we'd reference the Qt equivalent of gtk_window_present_with_time() here, but we can't, because there doesn't seem to be one. Perhaps add a FIXME comment pointing to a bug?

+ * Cancel the channel request. The precise effect depends on the current
+ * progress of the request.

I don't think the detailed explanation is useful to client developers; just link to the description of Cancel() in http://telepathy.freedesktop.org/spec/ for the gory details. Only the last couple of paragraphs need to be kept:

+ * If failed() is emitted in response to this method, the error SHOULD be
+ * %TELEPATHY_ERROR_CANCELLED.
+ *
+ * If the channel has already been dispatched to a handler, then it's too late
+ * to call this method, and the channel request will no longer exist.

and "SHOULD" should be replaced with "will".

+ * Proceed with the channel request.

Does this method even need to be documented? If it's public, I suppose we have to; if it's protected, just keep the first paragraph and point to the spec for the rest.

+ * The easiest way to create connection objects is trough Account. One can

through

+ * Another way to create a Connection object is to just call the create method.
+ * For example:
+ *
+ * \code ConnectionPtr am = Connection::create(busName, objectPath); \endcode

I think this clouds the issue of which one to use when. Perhaps "If you already know the object path, you can just call ..."?

+ * property MAY be zero.

"may" please, this isn't a specification (unlike telepathy-spec, which uses RFC-style MAY/SHOULD/MUST).

Any progress/merges?

I've removed the patch keyword, please re-add it when this branch is ready for review.

(In reply to comment #1)
> Up to commit bfd6bb0bc6:
> 
> + * \brief The AbstractClient class provides an object representing a
> + * <a href="http://telepathy.freedesktop.org">Telepathy</a> client.
> 
> Do we really need to hyperlink Telepathy? I think we can assume that Telepathy
> users know where the website is from the README!
TODO

> + * 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/ ?
TODO
 
> + * The easiest way to create account objects is trough AccountManager. One can
> 
> through
Done

> + * Another way to create an Account object is to just call the create method.
> 
> Could you expand on this a little to make it clear that ValidAccounts is the
> list of stored accounts, and create() adds a new one? At the moment it reads as
> though this is some sort of alternative API for the same thing.
This actually does not create a new account. This is the Account::create method, not the AccountManager::createAccount method, which actually attempts to create a new account
 
> + * See avatar specific methods documentation for more details.
> 
> avatar-specific methods' documentation
Done
 
> + *  <li>A fake group implementation when handle type is different from
> + *  HandleTypeContact</li>
> 
> when the handle type *is* HandleTypeContact, surely?
Done

> + * undefined until the introspection process is completed; isReady() returns
> true
> 
> until the introspection process is completed, i.e. isReady() returns true
> 
> (this text exists in several places)
Done
 
> + * Channel object will transition to closed too.
> 
> I think we should document this in terms of invalidated - we want to encourage
> people to see invalidated as "the" this-object-is-dead notification.
Done

> + * As a result, approvers should use the first handler by default.
> 
> ... by default, unless they have a reason to do otherwise.
Done

> You seem to be copying <tp:rationale> into these docstrings in a fairly
> indiscriminate way. I don't think that's necessarily wise - the rationale is to
> explain the spec, and not all of it needs to be in API docs (for that matter,
> not everything from the spec needs to be in these docs - we can reference the
> spec normatively). "More is better" is not necessarily true...
> 
> In particular, in PendingOperation *ChannelDispatchOperation::claim(), the
> rationale seems very much out of place.
TODO

> + * A channel has closed before it could be claimed or handled. If this is
> + * emitted for the last remaining channel in a channel dispatch operation, it
> + * must immediately be followed by invalidated() with error
> + * %TELEPATHY_QT4_ERROR_OBJECT_REMOVED.
> 
> s/must/will/ to get this into the client's perspective, as usual
Done
 
> + * This corresponds to the _NET_WM_USER_TIME property in EWMH.
> 
> (Unix developers: this corresponds ... in EWMH.)
Done
 
> Ideally we'd reference the Qt equivalent of gtk_window_present_with_time()
> here, but we can't, because there doesn't seem to be one. Perhaps add a FIXME
> comment pointing to a bug?
TODO
 
> + * Cancel the channel request. The precise effect depends on the current
> + * progress of the request.
> 
> I don't think the detailed explanation is useful to client developers; just
> link to the description of Cancel() in http://telepathy.freedesktop.org/spec/
> for the gory details. Only the last couple of paragraphs need to be kept:
Done, did not link the spec, I will do it only in one place so people have a link to the spec, no need to do it everywhere. Same for link Telepathy wiki everywhere.
 
> + * If failed() is emitted in response to this method, the error SHOULD be
> + * %TELEPATHY_ERROR_CANCELLED.
> + *
> + * If the channel has already been dispatched to a handler, then it's too late
> + * to call this method, and the channel request will no longer exist.
> 
> and "SHOULD" should be replaced with "will".
Done

> + * Proceed with the channel request.
> 
> Does this method even need to be documented? If it's public, I suppose we have
> to; if it's protected, just keep the first paragraph and point to the spec for
> the rest.
Done
 
> + * The easiest way to create connection objects is trough Account. One can
> 
> through
Done

> + * Another way to create a Connection object is to just call the create
> method.
> + * For example:
> + *
> + * \code ConnectionPtr am = Connection::create(busName, objectPath); \endcode
> 
> I think this clouds the issue of which one to use when. Perhaps "If you already
> know the object path, you can just call ..."?
Done
 
> + * property MAY be zero.
> 
> "may" please, this isn't a specification (unlike telepathy-spec, which uses
> RFC-style MAY/SHOULD/MUST).
> 
Done

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.

(In reply to comment #3)
> > + * Another way to create an Account object is to just call the create method.
> > 
> > Could you expand on this a little to make it clear that ValidAccounts is the
> > list of stored accounts, and create() adds a new one? At the moment it reads as
> > though this is some sort of alternative API for the same thing.
> This actually does not create a new account. This is the Account::create
> method, not the AccountManager::createAccount method, which actually attempts
> to create a new account

Perhaps for 0.4, we should stop calling pseudo-constructors create(), and call them createObject(), or createProxy(), or even construct(), to avoid this source of confusion. If so, we should add the new version soon, deprecate the old version after a while, then delete the old deprecated version just before 0.4.

Reviewied up to commit 1120df, just a few (grammar) nitpicks:

* TelepathyQt4/abstract-client.cpp:228
  "channel observer is interested." -> "channel observer is interested in."

* TelepathyQt4/connection.cpp:558
  TelepathyQt4/account-manager.cpp:183
  TelepathyQt4/account.cpp:245
  "which will automatically keeps" -> "which will automatically keep"

* TelepathyQt4/account.cpp:873
  Doc for connectionObjectPath() is pasted above the function, and
  once more below it (I assume a copy/paste typo).

(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

(In reply to comment #6)
> Reviewied up to commit 1120df, just a few (grammar) nitpicks:
> 
> * TelepathyQt4/abstract-client.cpp:228
>   "channel observer is interested." -> "channel observer is interested in."
Fixed
 
> * TelepathyQt4/connection.cpp:558
>   TelepathyQt4/account-manager.cpp:183
>   TelepathyQt4/account.cpp:245
>   "which will automatically keeps" -> "which will automatically keep"
Fixed
 
> * TelepathyQt4/account.cpp:873
>   Doc for connectionObjectPath() is pasted above the function, and
>   once more below it (I assume a copy/paste typo).
Fixed

Now branch contains all changes in the docs. All review suggestions here were applied but there are some parts not reviewed.

- Fixed all headerfile definitions
- Fixed group definitions (avoid duplication)
- Fixed examples definition
- More standardization
- Added lots of docs to other classes
- Other fixes/improvements

Master from 4ad4d2 to c37896 contains all changes.

(In reply to comment #9)
> Now branch contains all changes in the docs. All review suggestions here were
> applied but there are some parts not reviewed.
> 
> - Fixed all headerfile definitions
> - Fixed group definitions (avoid duplication)
> - Fixed examples definition
> - More standardization
> - Added lots of docs to other classes
> - Other fixes/improvements
> 
> Master from 4ad4d2 to c37896 contains all changes.

Looks mostly fine, only noticed two typos:

* in 8429e2b: "No further methods must not be called on it." in several places
* in 88ae265: in PendingAccount::account() return value doc "newly create Account object"

Fixed in 0.3.1.