[Kde-pim] Review Request 110591: Add vcard based ContactGroup2 format

[Kevin Krammer]

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



akonadi/contact/abstractcontactgroupformatter.h
<http://git.reviewboard.kde.org/r/110591/#comment29406>

    you need to duplicate the API docs, otherwise this will result in a krazy checker warning about an undocumened method
    also needs appropriate @since tag



akonadi/contact/contactgroupeditor.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29407>

    rest of the code is in pim coding style so if ( mItem... ) for consistency



akonadi/contact/contactgroupmodel.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29409>

    should probably be called "isEmptyMember" or "isEmpty", emptyMember sounds more like something that empties the member



akonadi/contact/contactgroupmodel.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29408>

    maybe shorter to just write
    return member.name.isEmpty() && member.email.isEmpty()



akonadi/contact/contactgroupmodel.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29410>

    this no longer looks at all the members?



akonadi/contact/contactgroupmodel_p.h
<http://git.reviewboard.kde.org/r/110591/#comment29411>

    kabc/



akonadi/contact/contactstreemodel.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29412>

    kabc/
    and move to old group's include



akonadi/contact/standardcontactactionmanager.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29414>

    this is not exactly the same check.
    the old check would have also been true for an item without payload.
    have you verified that this can not happen?



kabc/contactgroup2.h
<http://git.reviewboard.kde.org/r/110591/#comment29416>

    I think we usually use QList
    



kabc/contactgroup2.h
<http://git.reviewboard.kde.org/r/110591/#comment29418>

    argument is not called reference



kabc/contactgroup2.h
<http://git.reviewboard.kde.org/r/110591/#comment29417>

    space before )



kabc/contactgroup2.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29419>

    rest of the file seems to use spaces in parentheses



kabc/contactgroup2.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29420>

    spaces, position of &



kabc/contactgroup2.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29421>

    hadn't that been succeeded by text/directory?
    or does text/direcory only apply to single contacts?



kabc/vcardconverter.h
<http://git.reviewboard.kde.org/r/110591/#comment29422>

    @since missing



kabc/vcardconverter.h
<http://git.reviewboard.kde.org/r/110591/#comment29423>

    @since missing



kabc/vcardconverter.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29424>

    position of &



kabc/vcardconverter.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29425>

    maybe it should be >= v4_0?
    



kabc/vcardtool.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29426>

    that looks like something which we should already have elsewhere.
    even if it is not sharable right now we should reference the other copy so that we can make it sharable in the future



kabc/vcardtool.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29429>

    spaces, position of &



kabc/vcardtool.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29427>

    sounds dangerous if there is a 4.X or 5.0 in the future



kabc/vcardtool.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29428>

    might not be good to ignore version argument and hardcode a specific value



kabc/vcardtool.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29430>

    position of &



kabc/vcardtool.cpp
<http://git.reviewboard.kde.org/r/110591/#comment29431>

    while correct syntactically,I would prefer isGroup = false


- Kevin Krammer


On Sept. 11, 2013, 1:38 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110591/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2013, 1:38 p.m.)
> 
> 
> Review request for KDEPIM-Libraries, Kevin Krammer and Tobias Koenig.
> 
> 
> Description
> -------
> 
> This patch adds a vcard based replacement for ContactGroup, which uses UID's for references instead of Akonadi::Item::Id's. ContactGroup2 is meant to replace ContactGroup, but we have to wait until KDE5 until we can get rid of ContactGroup and rename ContactGroup2 (for binary compatiblity).
> 
> 
> Diffs
> -----
> 
>   akonadi/contact/abstractcontactgroupformatter.h adaac0fd2063a155509591510bab4584bec0b858 
>   akonadi/contact/abstractcontactgroupformatter.cpp 94b27905e4cc3c07d10502c5c10b68eefb3357fb 
>   akonadi/contact/contactgroupeditor.h bdef7871f760d8e13e9d1bd19a45f9161090f858 
>   akonadi/contact/contactgroupeditor.cpp 1a83b972cc9e0c30208b4cf5a00e32c784524a03 
>   akonadi/contact/contactgroupeditor_p.h 981e1d494ea4aaac295458e417847caded4d287d 
>   akonadi/contact/contactgroupeditordialog.cpp 2cbf0dee4b9efa52d7fb21b3842b4658c62d4968 
>   akonadi/contact/contactgroupexpandjob.h 48914153f71eb9ee651be0b717ab6d26332f0498 
>   akonadi/contact/contactgroupexpandjob.cpp 304b7a42ed0a04caed7cb798695a4afbabcb0641 
>   akonadi/contact/contactgroupmodel.cpp 03875819d5c61724e960c43c5a025a67a776373a 
>   akonadi/contact/contactgroupmodel_p.h 12332ced2ae9bd7d47bd2466fc27eb14c7e51da6 
>   akonadi/contact/contactgroupviewer.cpp 9c04e905d02fecc9bc9aaf0165ff6cb94e3d4370 
>   akonadi/contact/contactstreemodel.cpp b98c867a62aa68a5d67918ad8814b419a164fea9 
>   akonadi/contact/standardcontactactionmanager.cpp 9248b89f211b33a9c5ee3a5bc038140d565bfbc7 
>   includes/KABC/ContactGroup2 PRE-CREATION 
>   kabc/CMakeLists.txt b3907c69758eb4ab7973af60f1d7e4da625fc96b 
>   kabc/addressee.cpp c6c6515d0c207b5af4b264268aab1c30a3a24ad0 
>   kabc/contactgroup2.h PRE-CREATION 
>   kabc/contactgroup2.cpp PRE-CREATION 
>   kabc/tests/CMakeLists.txt 7bc46cb38380f3374bd6c5f023c0f7a5dbfe31dd 
>   kabc/tests/contactgroup2test.cpp PRE-CREATION 
>   kabc/vcardconverter.h e379f7c6248edaf0e313154362ea0ca66b5765dc 
>   kabc/vcardconverter.cpp 9d7556c5efbef18a26db661e0fac82e15124c2fe 
>   kabc/vcardparser/vcard.h 3f714c424ef71a8732f192d65388df2c46843371 
>   kabc/vcardtool.h 6f2b3707755d7889bef5074b2d5f089a9a8349f5 
>   kabc/vcardtool.cpp e99bd9f52fc1f987cf788a7670a93704a37ef123 
> 
> Diff: http://git.reviewboard.kde.org/r/110591/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

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