KPeople Review

Sun Mar 10 13:59:34 UTC 2013

On Sun, Mar 10, 2013 at 11:07 AM, 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.
>
> 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.
>
> The logic of this (though it may be horrendously wrong) is we have many
usecases in KTp as well as other potential KTp users where we need data for
one contact and one contact only.
Such as in our text-ui when chatting I want access to the person. I don't
want to load the entire contact list. Same for when I see a file is tagged
with a person in Dolphin.

Duplicate code is bad, but querying 7000 contacts (for example) to get data
on 1 is silly we do have a way to solve that.

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

Possibly.. though it's also bad to have everything jumping about like crazy
for 7 seconds. I was under the impression this 7 seconds could be reduced
significantly by only selecting IM contacts in the inital query?

>

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.

In my earlier email I said and I thought we had agreed on, offsetting the
kpeople release by two weeks from 0.6.0. This is so we can promo it
seperately and it will be generally less confusing to users and packagers
than releasing two versions that have different features at the same time.
Also I don't want my developers (me anyway) getting distracted from bug
fixing before 0.6.0. There is a lot to fix.

This also gives you more than 3 weeks.
Martin, can you confirm you're still doing this? (you made a comment on
reddit that implied you were wanting to release the same time as 0.6.0)

David

>
>
> Vishesh Handa
>
> _______________________________________________
> KDE-Telepathy mailing list
> KDE-Telepathy at kde.org
> https://mail.kde.org/mailman/listinfo/kde-telepathy
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130310/769c751a/attachment.html>