[Kde-pim] Review Request 122453: Improve KPeople documentation, in order to become a Framework

[Martin Klapetek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122453/#review75548
-----------------------------------------------------------



README.md
<https://git.reviewboard.kde.org/r/122453/#comment52248>

    I find the second part a bit confusing
    
    "and the people who hold them"
    
    reads to me like "I hold a contact (in my hand?) and so KPeople provides access to me". It's also not clear what "all contacts" is.
    
    I'd simply say "A contact aggregation library" perhaps?



README.md
<https://git.reviewboard.kde.org/r/122453/#comment52249>

    I've read this over and over and still find it unclear of what it is.
    
    "KPeople offers unified way to access contacts from multiple different sources, allowing to aggregate related contacts into 'persons'. These sources are plugin-based which allows for easy extension of the contacts collection." ....something like that?



README.md
<https://git.reviewboard.kde.org/r/122453/#comment52250>

    "we want to integrate...into our" --> this whole Usage part talks about "you", so you'll want to use "you want to integrate...into your"
    
    Same for "We will mainly find"



README.md
<https://git.reviewboard.kde.org/r/122453/#comment52251>

    This for a change talks in "it" form :) I'd change that to "you" again. "You should _only_ use it..." etc
    
    "providing support for a new backend type" --> only new backend, not a new type.



src/persondata.h
<https://git.reviewboard.kde.org/r/122453/#comment52252>

    "all e-mail addresses"? As "all e-mails" confusingly reads like "all emails you received from this contact"
    
    (in fact, the method should probably not be named allEmails as well for the same reason)



src/widgets/mergedialog.h
<https://git.reviewboard.kde.org/r/122453/#comment52254>

    "By properly merging contacts, the user will end up with more information on each contact"...?
    
    (it's not like two different email addresses will give me better information on who someone _is_ ;)



src/widgets/persondetailsview.h
<https://git.reviewboard.kde.org/r/122453/#comment52253>

    --> "for which the data will be displayed"


- Martin Klapetek


On Feb. 6, 2015, 3:14 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122453/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2015, 3:14 p.m.)
> 
> 
> Review request for KDE Frameworks, KDEPIM and Telepathy.
> 
> 
> Repository: libkpeople
> 
> 
> Description
> -------
> 
> Adds a README.md and a metainfo.yaml.
> It also documents some classes and methods that didn't have any documentation.
> 
> 
> Diffs
> -----
> 
>   src/widgets/CMakeLists.txt bd2cd7e 
>   src/widgets/mergedialog.h 305d07b 
>   src/widgets/persondetailsview.h 0769d51 
>   CMakeLists.txt 00177f3 
>   README.md PRE-CREATION 
>   metainfo.yaml PRE-CREATION 
>   src/CMakeLists.txt 7e01976 
>   src/duplicatesfinder_p.h 05f0063 
>   src/persondata.h 0fab902 
>   src/personsmodel.h 32dc1f5 
> 
> Diff: https://git.reviewboard.kde.org/r/122453/diff/
> 
> 
> Testing
> -------
> 
> Unit tests still pass ;).
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/