----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
