KPeople Review

[Vishesh Handa]

 Hey Martin

As discussed in the PIM Sprint - I've been reviewing the kpeople code so
that we can get it ready by the end of march for the first release. I've
already made some basic changes, but there are still big changes to be made
-

1. Ontologies - Who is responsible for installing the ontologies?
Considering that the telepathy nepomuk feeder is pushing the properties
maybe we should only let it install the ontology? We do have a run-time
dependency on the feeder.

2. Rename PersonModelContactItem to ContactItem and PersonModelItem to
PersonItem? We can currently make them abstract enough to reuse them in
other places such as PersonData, so they won't exactly be tied to the model.

3. Clear Separation Between Nepomuk and kpeople - I know that kpeople
relies on Nepomuk heavily, but we need to have clear boundaries of how
Nepomuk components are going to be used. In case it's not apparent I'm
referring to the use case of using Nepomuk Ontologies as keys.

a. Their is a QHash<QString, QUrl> which maps the sparql variables to their
respective properties. This hash is constructed by using a regex and
modifying the property names. It just happens that the property names turn
out to be the exact same as those in the queries. Eg -

 http://www.semanticdesktop.org/ontologies/2007/03/22/nco#emailAddress  ->
nco_emailAddress

It might be simpler to construct all the mappings ourselves - hash.insert(
NCO::emailAddress(), "nco_emailAddress" ); That way we can avoid using such
long binding names if we want.

b. We shouldn't be using Nepomuk properties as keys - In the ContactItem we
have a QMultiHash<QUrl, QVariant>. They QUrl over here is a nepomuk
property. When we are asked of a particular role say emailAddress we
translate it to its corresponding property - nco:emailAddress and then
return the value. This is confusing cause nco:emailAddress has a domain of
a nco:EmailAddress not of a Contact. Ditto in the case of contact group
names.

Instead it would be better to have a QHash<Role, Variant> and directly use
the Roles as keys, that way we wouldn't even have to have switch case
statements.

The hash in (a) would then be of the form QHash<Role, QString>. And we
could manually map the binding names to the roles - hash.insert( EmailRole,
"nco_emailAddress" ).

With this the only Nepomuk code in the model would be that of the query. A
new comer trying to contribute would find it a LOT simpler, cause now they
don't need to understand the ontologies and wonder why they are being
mapped around.

c. If (b) is implemented, then we cannot use the Nepomuk Resource class as
an argument to the ContactItem. It would be a lot simpler to just issue a
small query and fetch the required properties and map them with the hash
described above.

4. PersonsData - It can just be a light wrapper over PersonItem and
ContactItem. They already have all the data. No point duplicate code.

5. Loading properties with cardinality > 1 - In the PersonModel you
currently have a check to make sure the same contact uri is not returned
twice - They will get returned twice if the contact contains multiple
emails/photos/groups - Each time with a different combination of any of
those 3. We need to handle that case as well.

It seems quite simple. We can have a list of bindings whose cardinality > 1
and insert only those if the uri appears twice.

6. ResourceWatcher - The watcher will fail for the cases when a contact
group, email, or photo is changed. Anything that is not a direct property
of the nco:PersonContact - Mainly cause it will return -

propertyChanged - nco:photo
from: nepomuk:/res/oldUri
to: nepomuk:/res/newUri

and the code does this - hash.replace( prop, to ); which will replace
file://location/something to nepomuk:/res/newUri. Also if we go ahead with
step 3 (b), then we will need to map these properties to the roles. I'm not
sure if this is a good idea. Maybe we should just drop live updates for
now? Not sure.

7. Realtime loading of the contact list - Currently the query constructs
all the ContactItems when iterating over the query results, but it only
adds them at the end cause we need to group them properly. It might be
better from a user point of view to add them the moment they are
constructed. That way the user gets to see the contact list being populated
instead of having to stare at a blank screen for some time. In my case it
is about 7 seconds. Once the query is finished, then the contacts can be
re-arranged into people.

8. ContactType and ContactTypeRole - What is this? It's not used anywhere.
Also how can a contact be of a single type - Email, IM, Phone, MobilePhone
or Postal? Can't it have a combination of these 5? What do you want to do
about this?

9. Merging Contacts into People - The current code is quite wrong as it
uses mergeResource - We just need to group them under the same person, not
merge them into. I think we discussed this during the pim sprint.

10. Unmerging - In that case we want to delete the pimo:Person if it only
consists of one nco:PersonContact

11. Automatic Merging - Let me know if you want me to look into that code.
I haven't really taken a look. From what my notes say, we had marked this
as optional for the next release.

----

I'm willing to implement most of the points above. Points 1-5 to quite
simple and shouldn't take more an hour or two + testing.

Let me know what you think. We have about 3 weeks.

Vishesh Handa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130310/d52a2d6d/attachment-0001.html>