[Kde-pim] Review Request 119575: Fix krunner contacts pllugin

[Dan Vrátil]

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


Oops, don't ship it! Sorry, I haven't checked the rest of the code - your patch breaks the "contacts" query (that should return *all* available contacts), and the foreach loop is now doing some unnecessary comparisions (that have already been done by Baloo).


runners/contacts/contactsrunner.cpp
<https://git.reviewboard.kde.org/r/119575/#comment44808>

    This can go completely away, since the list already contains results that match name or email.



runners/contacts/contactsrunner.cpp
<https://git.reviewboard.kde.org/r/119575/#comment44805>

    Since the foreach loop is now not iterating over all the contacts in addressbook, but only those that match the given term, this will never be true.
    
    The point of this condition is to ensure, that when users write "contacts" into krunner, they will get list of *all* contacts. So I think this should be moved before the foreach(...) so that we have something like
    
    ```
    if (term.compare(..."contacts" ... ) {
       fetchAllContacts();
    } else {
       // Perform the search on Akonadi
       Akonadi::ContactSearchJob *job = new Akonadi::ContactSearchJob(this);
       ....
       ....
    }
    ```
    
    and then you can iterate over the list of results.



runners/contacts/contactsrunner.cpp
<https://git.reviewboard.kde.org/r/119575/#comment44810>

    This should be kept, so that we know if the result was matched by email or by name - you can however move this to the line 76.



runners/contacts/contactsrunner.cpp
<https://git.reviewboard.kde.org/r/119575/#comment44807>

    This will always be "true" (since the list is already filtered), so you can remove the condition and unindent the block



runners/contacts/contactsrunner.cpp
<https://git.reviewboard.kde.org/r/119575/#comment44809>

    This should stay here


- Dan Vrátil


On Aug. 9, 2014, 1:55 p.m., Marc Deop wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119575/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2014, 1:55 p.m.)
> 
> 
> Review request for KDEPIM and Plasma.
> 
> 
> Bugs: 282567
>     http://bugs.kde.org/show_bug.cgi?id=282567
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> Fix krunner contacts plugin to lookup contacts through Akonadi
> 
> 
> Diffs
> -----
> 
>   runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 
>   runners/contacts/contactsrunner.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b 
> 
> Diff: https://git.reviewboard.kde.org/r/119575/diff/
> 
> 
> Testing
> -------
> 
> Compiled, installed and tested locally
> 
> 
> Thanks,
> 
> Marc Deop
> 
>

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