[Bug 43598] Add high-level API for Proto.I.Addressing interface

[bugzilla-daemon at freedesktop.org]

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

--- Comment #4 from Olli Salli <ollisal at gmail.com> 2011-12-11 11:18:57 PST ---
(In reply to comment #3)
> (In reply to comment #2)
> > Comments:
> > 
> > - Use requestAllProperties() instead of direct Properties iface GetAll
> > QDBusPendingCall
> Done, also done for the other Protocol interfaces and the main
> ConnectionManager interface while at it.

Great!

> 
> > > ProtocolInfo: Add connectionManager() accessor and make isValid() check if CM is still valid.
> > 
> > It's a bit questionable to do this in this branch. The ProtocolInfos aren't
> > really standalone objects - just records which ConnectionManager contains a
> > list of, and Account has one. So it doesn't make sense to store a ProtocolInfo
> > reference somewhere which can't access the ConnectionManager easily anyway
> > (either directly or through an Account), but still can guarantee it stays
> > alive.
> > 
> > It seems to me you only added this for constructing the Proto.I.Addressing
> > proxy when needed. I think you should rather just store the manager's dbus
> > connection, service name and object path on ProtocolInfo initialization
> > instead, and use them later for constructing the proxy. Then the CM doesn't
> > even need to stay alive for that API to work (less preconditions for users to
> > worry about).
> > 
> Yes, I added the constructor param only to get the dbus connection, bus name
> and object path from the ConnectionManager object to build the Proto.I.Addr
> proxy. I've now removed the accessor but left the constructor receiving the
> ConnectionManagerPtr object, so I can retrieve the info above + the CM name
> from it without passing 4 params.
> We can always remove, update this constructor later without breaking API/ABI as
> this is an unexported private constructor.
> ProtocolInfo::isValid() also only cares about mPriv.constData() again.
> 

Yeah, looks good now. The constructor is fine, what matters is only storing the
needed info - how to pass them to the ctor is no issue.

> > + * An example would be a vCard TEL field with a formatted number in the form
> > of
> > + * +1 (206) 555 1234, this would be normalized to +12065551234.
> > 
> > -> For example, a vCard TEL field formatted as +1 (206)... could be normalized
> > to +1206...
> Done

Still says "a formatted number in the form of". If you want it to say "number",
then "a number in a vCard TEL field formatted as +1 (234) 567 could be
normalized to +1234567" would be better.

> Done. Actually refactored this code for all Protocol interfaces at once, it was
> easier. Now if all the info needed is in the immutable properties no
> introspection will be made.

Great, that you did this for all the ifaces! Could you test all three of these
cases:

1) all info in .manager file
2) no .manager file, but some immutable properties present, though not all
3) all immutable props present

+    bool loadImmutableProperties();
+    bool loadMainProperties(const QVariantMap &props);
+    bool loadAvatarsProperties(const QVariantMap &props);
+    bool loadPresenceProperties(const QVariantMap &props);
+    bool loadAddressingProperties(const QVariantMap &props);

To be consistent with the rest of tp-qt, we should call these kinds of
functions extract<iface short name>Properties(). See e.g. Channel and its
subclasses. Also usually they don't have a bool return value - and except for
loadImmutableProperties, I don't think you're using them either. So you could
take the return values out, except maybe for loadImmutableProperties.

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