[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 #2 from Olli Salli <ollisal at gmail.com> 2011-12-09 11:52:48 PST ---
Comments:

- Use requestAllProperties() instead of direct Properties iface GetAll
QDBusPendingCall

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

If there is some clear reason I'm not seeing why
ProtocolInfo::connectionManager() makes sense, please remove
ProtocolInfo::cmName() etc completely then, as it's wholly redundant with just
doing connectionManager()->name() etc.

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

+ * \param vcardField The vCard field of the address we are normalizing.

The address is the value of the vCard field, an address doesn't have multiple
vCard fields, which we could be normalizing one of. So "The vCard field the
address belongs to" or something?

+ *         when the \a uri has being normalized or an error occurred.

has been normalized (in 2 places)

There's also a glaring omission: you don't use the immutable properties
introspecting ConnectionManager.Protocols gives you. Note that it's a
dictionary of *fully qualified* (multiple interfaces) property values. In fact
it seems currently they're used only for the core Protocol interface
properties, but separate GetAll calls are still done for all the auxiliary
interface properties. This is inefficient - so please utilize them for the two
Addressing props here, both of which are immutable as far as I can see.

Please also file separate bugs to use the immutable props for Avatars and
Presence ifaces too. You don't have to worry about them in the context of this
work though. But later on, we have to fix that as well.

This is especially inexcusable, because we originally justified not having
ConnectionManager::FeaturePresence, FeatureAvatars etc by "they can anyway be
got from the Properties with no additional cost for each protocol". But
currently we make up to three additional GetAll calls for each protocol, even
if the immutable properties map common to all protocols actually included
properties for them too! So instead of 1 call to GetAll everything in
ConnectionManager, we do that, and in addition up to 3*number of protocols
GetAll calls. This makes ConnectionManager/Account::FeatureProtocolInfo
introspection way slower than it should be.

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