[Bug 28367] Add StreamTube interface support

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Wed Feb 16 23:52:11 CET 2011


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

--- Comment #16 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-02-16 14:52:11 PST ---

So here goes my 2 cents:

Minor coding style issues:
- On private classes always declare the methods before the variables

- On class members initialization the "," should be in the same line as the
member initialization and not in a new line.

- Add a new line (in incoming-stream-tube-channel.h):
+class QIODevice;
+namespace Tp
+{

Same thing here (in outgoing-stream-tube-channel.h):
+class QLocalServer;
+namespace Tp
+{

Check other places also.

- Remove trailing whitespaces:
I can see at least one in TubeChannel::Private::introspectTube:
+    Client::ChannelInterfaceTubeInterface *tubeInterface = 

- Remove the empty line after StreamTubeChannel::mPriv declaration.

- I like to name params in the slots declarations, but only do this if you are
in the mood

- When using templates, only add spaces if really required.
+    QHash< uint, Tp::ContactPtr > contactsForConnections() const;
should be
+    QHash<uint, Tp::ContactPtr> contactsForConnections() const;

- Move the signals documentation to the bottom of the classes.

- Don't use \brief in methods documentation, the first line is considered the
\brief doc.

- Lots of documentation only have \return (for eg.
PendingStreamTubeConnection::addressType). Please add a brief description also
in the first line.

- I would like to see this TODO and also any other TODO removed and implemented
before merging:
+    // TODO: Use error to provide more detailed error messages
+    Q_UNUSED(error)

Other minor details:
- Make IncomingStreamTubeChannel::device() const, same for
PendingStreamTubeConnection::device()

- When rebasing against master there will be a conflict in account.h, as Olli
just added hints support there. Please fix this and add a default "hints =
ChannelRequestHints()" to the createStreamTube methods and pass it to
PendingChannelRequest. 
  Ps.: No need to add new methods that take hints as param, just change the
current methods to get a hints default param, as this is not in our API/ABI
yet.

- Make it Private *mPriv instead of Private * const here (and in other places
as well):
+    Private * const mPriv;
+
+    friend class PendingStreamTubeConnection;
  Ps.: Also move the "friend class PendingStreamTubeConnection" declaration to
be located immediately after the private keyword. Check channel.h for examples.
Do the same for all "friend class" declarations other than "friend struct
Private"

- Make sure all public classes have a fancy header (not sure if there is any
missing).

- No need to have a virtual destructor in PendingStreamTubeConnection::Private,
please also check other places where it is not needed.

I think that is pretty much it for now. I will double-check again when the
branch is updated. Almost there.

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



More information about the telepathy-bugs mailing list