[Kde-pim] Review Request: Kmail: Do not ask in which Adressbook to add a contact when only one exist and use it instead

[Thomas McGuire]

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



/trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp
<http://reviewboard.kde.org/r/5166/#comment7347>

    Qt includes should be after KDE includes.
    The include order is:
    - own includes
    - kdepim includes (like libkdepim)
    - kdepimlibs includes
    - kdelibs incldues
    - qt includes
    - system includes (like stdlib)
    
    I don't remember the reason for the ordering though :)



/trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp
<http://reviewboard.kde.org/r/5166/#comment7350>

    if you want to be const-pedantic, this can be:
    Akonadi::CollectionFetchJob * const addressBookJob



/trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp
<http://reviewboard.kde.org/r/5166/#comment7351>

    this can be
    const Akonadi::CollectionFetchJob * const addressBookJob
    
    See http://www.parashift.com/c++-faq-lite/const-correctness.html if this seems confusing



/trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp
<http://reviewboard.kde.org/r/5166/#comment7353>

    The collectiondialog uses setAccessRights, so we need to filter out collections that don't match the access rights, otherwise the count would be different from the count of collectiondialog.
    We talked on IRC about this.



/trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp
<http://reviewboard.kde.org/r/5166/#comment7348>

    Please use KMessageBox, it provides better KDE integration.
    See http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKMessageBox.html.
    
    Easiest is to use one of the static methods, like KMessageBox::information()



/trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp
<http://reviewboard.kde.org/r/5166/#comment7349>

    Missing i18n(), this string can't be translated.



/trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp
<http://reviewboard.kde.org/r/5166/#comment7352>

    The return path here doesn't have a emitResult().
    
    Each job needs to have an emitResult() when it is finished, otherwise the job is never properly deleted.
    
    You should probably also set an error here, so that the caller knows that adding a contact didn't work.


- Thomas


On 2010-08-27 10:40:51, Nicolas Lécureuil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/5166/
> -----------------------------------------------------------
> 
> (Updated 2010-08-27 10:40:51)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> In KMail when we want to add a contact in the addressbook and if there is only one adressbook, a window will open with only one choice.
> With this patch the only adressbook is choosed directly.
> 
> If there is no adressbook available, an empty list open. With this patch a QMessageBox explain to the user what to do.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/libkdepim/addemailaddressjob.h 1128979 
>   /trunk/KDE/kdepim/libkdepim/addemailaddressjob.cpp 1128979 
> 
> Diff: http://reviewboard.kde.org/r/5166/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Nicolas
> 
>

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