[Bug 36881] Wrong documentation for Tp::Account::connectionStatusChanged (and a bunch of other parts of the API)

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=36881

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Wrong documentation for     |Wrong documentation for
                   |Tp::Account::connectionStat |Tp::Account::connectionStat
                   |usChanged                   |usChanged (and a bunch of
                   |                            |other parts of the API)
                URL|                            |http://cgit.collabora.com/g
                   |                            |it/user/andrunko/telepathy-
                   |                            |qt4.git/log/?h=docs
             Status|NEW                         |ASSIGNED
         AssignedTo|telepathy-bugs at lists.freede |andrunko at gmail.com
                   |sktop.org                   |
                 CC|                            |ollisal at gmail.com

--- Comment #1 from Olli Salli <ollisal at gmail.com> 2011-05-27 05:00:27 PDT ---
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.

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