[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 #3 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-12-09 14:11:00 PST ---
(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.

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

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

> + * \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?
Done

> + *         when the \a uri has being normalized or an error occurred.
> 
> has been normalized (in 2 places)
Done

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

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