[Kde-pim] Review Request 121639: Split out KF5Contacts dependency

Martin Klapetek martin.klapetek at gmail.com
Mon Jan 5 16:49:02 GMT 2015 

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


Sorry for all those style issues, but it makes the style really random ;)

Overall I think this change is ok, just couple small things below


src/backends/abstractcontact.h
<https://git.reviewboard.kde.org/r/121639/#comment50897>

    "said" -> "given"



src/backends/abstractcontact.h
<https://git.reviewboard.kde.org/r/121639/#comment50907>

    Maybe we should also have First- and Last-NameProperty?
    
    Then the NameProperty would just be combination of those according to user preference/locale



src/backends/abstractpersonaction.h
<https://git.reviewboard.kde.org/r/121639/#comment50898>

    &



src/global.h
<https://git.reviewboard.kde.org/r/121639/#comment50886>

    & and *
    
    Some docs would be useful too as it's being exported



src/global.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50887>

    QString &contactId



src/metacontact.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50888>

    AbstractContact::List &contacts



src/metacontact.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50889>

    QString &key



src/metacontact.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50890>

    AbstractContact::Ptr &contact
    
    same below



src/metacontact.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50891>

    if( -> if (



src/metacontact_p.h
<https://git.reviewboard.kde.org/r/121639/#comment50892>

    &contacts



src/persondata.h
<https://git.reviewboard.kde.org/r/121639/#comment50893>

    "said" -> "given"
    
    same below



src/persondata.h
<https://git.reviewboard.kde.org/r/121639/#comment50894>

    QString &key
    
    (cpp file too)



src/persondata.h
<https://git.reviewboard.kde.org/r/121639/#comment50908>

    Can we make this more explicit? I'd like to avoid clients always having to parse this and check for the content, so I'd suggest something like 
    
    "QPixmap picture() const" (and always load it if it's url)
    "QUrl pictureUrl() const"
    
    Most of the time QPixmap directly will be used anyway
    
    ...actually there's already photo(), so why would we want another QPixmap method?



src/personsmodel.h
<https://git.reviewboard.kde.org/r/121639/#comment50895>

    &s
    
    The name feels like model's custom property however, maybe use contactCustomProperty? Makes the API clearer/better understandable I guess
    
    Also can we maybe use "index" in the public API rather than "idx"?



src/personsmodel.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50896>

    &s



src/plugins/akonadi/akonadidatasource.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50899>

    while touching these lines already, if (
    
    also 2x below



src/widgets/abstractfieldwidgetfactory.h
<https://git.reviewboard.kde.org/r/121639/#comment50900>

    &



src/widgets/persondetailsview.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50901>

    &



src/widgets/persondetailsview.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50902>

    & and *



src/widgets/persondetailsview.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50903>

    &



src/widgets/persondetailsview.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50904>

    &



src/widgets/persondetailsview.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50905>

    &



src/widgets/plugins/emaildetailswidget.cpp
<https://git.reviewboard.kde.org/r/121639/#comment50906>

    & and *


- Martin Klapetek


On Dec. 31, 2014, 1:16 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121639/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 1:16 p.m.)
> 
> 
> Review request for KDEPIM, Telepathy, Christian Mollekopf, David Edmundson, and Martin Klapetek.
> 
> 
> Repository: libkpeople
> 
> 
> Description
> -------
> 
> Last week we had a couple of discussions regarding KPeople relationship with KDE PIM and KContacts and we agreed that we want to keep KContacts as a framework that is good at dealing with vCards, which leaves KPeople without a good way to send around its data.
> 
> This patch proposes an internal AbstractContact class that the backends will have to re-implement. At the moment it only requires a "customProperty" that lets us fetch whatever is needed.
> Furthermore, it also includes a set of convenience methods in PersonData to have a type-safe API.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 3df5f0b 
>   autotests/CMakeLists.txt 676f170 
>   autotests/fakecontactsource.h 5852297 
>   autotests/fakecontactsource.cpp 269f94f 
>   autotests/persondatatests.cpp 47a6213 
>   examples/CMakeLists.txt 74bd188 
>   examples/duplicates.cpp 72ff53b 
>   src/CMakeLists.txt 534066b 
>   src/backends/abstractcontact.h PRE-CREATION 
>   src/backends/abstractcontact.cpp PRE-CREATION 
>   src/backends/abstractpersonaction.h 703063e 
>   src/backends/abstractpersonaction.cpp be93d90 
>   src/backends/allcontactsmonitor.h 47e5dfd 
>   src/backends/allcontactsmonitor.cpp e517f17 
>   src/backends/basepersonsdatasource.h 3c25e28 
>   src/backends/basepersonsdatasource.cpp b794917 
>   src/backends/contactmonitor.h cf25953 
>   src/backends/contactmonitor.cpp ceb5d74 
>   src/declarative/personactionsmodel.cpp 4b34e15 
>   src/defaultcontactmonitor.cpp 36c31a7 
>   src/defaultcontactmonitor_p.h 8fc2812 
>   src/duplicatesfinder.cpp c6a1014 
>   src/global.h 9540623 
>   src/global.cpp 0a3b00d 
>   src/match.cpp 3a93aba 
>   src/match_p.h 5727270 
>   src/metacontact.cpp bba6b65 
>   src/metacontact_p.h 0d738cc 
>   src/persondata.h 66b19a3 
>   src/persondata.cpp 40156a9 
>   src/personpluginmanager.cpp 59f5a48 
>   src/personsmodel.h db907d4 
>   src/personsmodel.cpp c2d97f6 
>   src/plugins/akonadi/akonadidatasource.cpp ae4ace1 
>   src/widgets/abstractfieldwidgetfactory.h 1d2d8aa 
>   src/widgets/persondetailsdialog.cpp 8ca0a99 
>   src/widgets/persondetailsview.cpp b7535bd 
>   src/widgets/plugins/emaildetailswidget.h 72a96ee 
>   src/widgets/plugins/emaildetailswidget.cpp 839c5f6 
>   src/widgets/plugins/emails.h ede1686 
>   src/widgets/plugins/emails.cpp d69101b 
> 
> Diff: https://git.reviewboard.kde.org/r/121639/diff/
> 
> 
> Testing
> -------
> 
> Builds, the test passes. ktp tests work, somewhat.
> Also there's few tests altogether.
> 
> 
> 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/

More information about the kde-pim mailing list