Review Request: Contact Avatar support for Nepomuk Service

[Daniele Elmo Domenichelli]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101633/#review3916
-----------------------------------------------------------


The patch looks good, but let's discuss about the changes to the ontology, as you requested


nepomuk-storage.cpp
<http://git.reviewboard.kde.org/r/101633/#comment3194>

    is this Q_ASSERT necessary?



telepathy.trig
<http://git.reviewboard.kde.org/r/101633/#comment3193>

    I agree with this change, but maybe it could be the opposite:
    
    telepathy:avatarForIMAccount
       a rdf:Property ;
       rdfs:domain nie:DataObject ;
       rdfs:range nco:IMAccount ;
       rdfs:subPropertyOf nie:dataSource ;
    
    
    But I just realized that to be able to do that, nco:IMAccount (or even nco:ContactMedium) should be rdfs:subClassOf nie:DataSource, this change could make sense as well, since nie:Datasource is "A superclass for all entities from which DataObjects can be extracted" [1] and the avatar is actually a DataObject that is extracted from the IMAccount
    
    
    [1]http://oscaf.sourceforge.net/nie.html#nie:DataSource



telepathy.trig
<http://git.reviewboard.kde.org/r/101633/#comment3195>

    Another issue is that avatar can be changed, but just dropping this property means that it will be impossible to search for all the pictures that have been avatar for a certain account.
    To fix this, we could have a generic avatar property and a currentAvatar subProperty, but perhaps this is useless, what do you think?


- Daniele Elmo


On June 15, 2011, 6:16 p.m., George Goldberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101633/
> -----------------------------------------------------------
> 
> (Updated June 15, 2011, 6:16 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This patch reinstates support for Contact Avatars in the Telepathy Nepomuk Service.
> 
> Avatar support for local accounts looks trickier so will come in a subsequent patch.
> 
> I'd particularly like comments on the changes to the Telepathy ontology. The rationale behind adding the telepathy:avatar property to nco:IMAccount is that although we can use nco:photo, this can contain multiple photos of the contact and we need to be able to identify the one that is the avatar for this IM account. The reason for it not being upstream is that it seems like telepathy-level implementation detail to me (although I will of course discuss this with upstream in due course to see whether they think it might belong there). The result is you are meant to identify one particular nco:photo of the nco:personContact to be the telepathy:avatar.
> 
> 
> This addresses bug 270864.
>     http://bugs.kde.org/show_bug.cgi?id=270864
> 
> 
> Diffs
> -----
> 
>   abstract-storage.h 7d1be9b96711500ddd2ab5b6d9aa8791e51bcea1 
>   account.h b57ebf881e1bd661bd8db069cc3f5147ce10b515 
>   account.cpp e0cfdf0931c75126339e18fb2458a93d510b6e28 
>   contact.h ccc482aa36c3d8e38cfa89608043f198ef8be674 
>   contact.cpp bc5a29100e380bbcb7ae48930813d19842b0a836 
>   controller.cpp f320326534c8b76ea73097c906d82c2b90a35f22 
>   nepomuk-storage.h 216239371b0656dff61c4a189a709703094b7418 
>   nepomuk-storage.cpp 7d154494b759e2b9bca95d086f164f1a71ab1095 
>   telepathy.trig d5a07d202185cc0d9aaa0fbd681cfd0202fa88c1 
>   tests/controller-test.h cd85a675d731b43c0960e0b5ba3fb02d8aacade7 
>   tests/storage-test.h ab5f05f2e5faf6b0710c2dfd9f8d3f21ae06d330 
>   tests/storage-test.cpp 2ff9428f57f2515c60c9fe7fb24c46d9f6907b7d 
> 
> Diff: http://git.reviewboard.kde.org/r/101633/diff
> 
> 
> Testing
> -------
> 
> Unit tests (updated appropriately) all pass.
> 
> 
> Thanks,
> 
> George
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110615/14c5d7b9/attachment-0001.htm