KPeople Review

Tue Mar 12 14:25:52 UTC 2013

Hey,

On Sun, Mar 10, 2013 at 12:07 PM, Vishesh Handa <me at vhanda.in> wrote:

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

Actually we don't use any Tp ontologies in kpeople, just NCO and PIMO. So
the obvious question is - do we need the Telepathy ontologies at all?

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

Ok.

> 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's not really a regex, simple string parsing rather.

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

This does not "just happen", that's on purpose :)

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

The original idea was that there would be different "facets" for different
usages - IM, PIM etc - and each would have defined a set of properties
(ontologies) it is interested in and the query was supposed to be built up
from that, but that happened only half-way as you can see.

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

I don't think this is necessarily a problem on the client side though.

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

That does sound sensible. And I like it.

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

Tbh. I don't want anyone not understanding the ontologies 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.
>

As David pointed out - this is a nice API wrapper-class around one single
contact. And I like it that way. Mixing it with ContactItem would be a mess.

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

Ok, I will take a look.

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

Right, this is a problem. I don't want to drop live updates. We need
smarter updates handling.

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

I'm against this. I've tried this with the per-contact query and it just
looks horrible.

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

This is an old-ish relic, needs to be removed completely.

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

No, you missed the correct methods :)
See PersonsModel::createPersonFromIndexes().

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

Yup, needs checking - if (Person.groundingOccurances().size() == 1) remove
Person;

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

Later stuff.

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

Perfect. Let's get on with 3 and 6, the rest is quite easy to do.

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

David - Yes, the old plan still holds - release separately after a week or
more after 0.6 is released.

Cheers
-- 
Martin Klapetek | KDE Developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130312/c525c75b/attachment-0001.html>