KPeople Review

[Vishesh Handa]

On Tue, Mar 12, 2013 at 7:55 PM, Martin Klapetek
<martin.klapetek at gmail.com>wrote:

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

We use telepathy:acountIdentifier in the presence model. -
PersonsPresenceModel::queryNepomukForAccountId( .. ). Though we do not need
the ontologies to be present in our code for this. We are just using a
simple string.

I'm dropping the ontologies from kpeople.


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

Right. I meant that it's a little hard to understand.

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

Great. I can implement it then.


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

Well, that excludes me then! ;) Though seriously, if the ontology related
code is just a little bit, people who are more experienced with models can
easily help out, without getting scared by the ontologies.


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

But we already do everything PersonData does in ContactItem as well. Look
at the ContactItem::pullResourceProperties function. There are valid cases
when we just need to fetch all the contact data into a ContactItem for just
1 contact. Eg - a new contact has been added.


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

Right. I have some ideas. Lets see.


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

Alright. Lets ignore this for now :)


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



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