Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Sept. 22, 2016, 8:31 p.m.) Status -- This change has been discarded. 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.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 Diff: https://git.reviewboard.kde.org/r/119575/diff/ Testing --- Compiled, installed and tested locally Thanks, Marc Deop
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/#review99442 --- Closing as this review request is more than 2 years old. If it still applies to current Plasma. Please reopen. Thanks - David Edmundson On Aug. 9, 2014, 11:55 a.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, 11:55 a.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.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b > runners/contacts/contactsrunner.cpp > 2261e3744c695d046ec95e6dd97f1ad26c800d71 > > Diff: https://git.reviewboard.kde.org/r/119575/diff/ > > > Testing > --- > > Compiled, installed and tested locally > > > Thanks, > > Marc Deop > >
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/#review64153 --- Ship it! Looks OK now, thanks for the fix! - 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
Re: Review Request 119575: Fix krunner contacts pllugin
---
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
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Aug. 9, 2014, 11:26 a.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 (updated) - runners/contacts/contactsrunner.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 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
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Aug. 9, 2014, 11:55 a.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 (updated) - 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
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/#review63733 --- runners/contacts/contactsrunner.h https://git.reviewboard.kde.org/r/119575/#comment44387 I think you can remove m_book entierly. runners/contacts/contactsrunner.cpp https://git.reviewboard.kde.org/r/119575/#comment44388 Calling exec is generally a bad idea, but I'm not sure wether you have another option with the current runner design. - Christian Mollekopf On Aug. 3, 2014, 10:40 p.m., Marc Deop wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Aug. 3, 2014, 10:40 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.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 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
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Aug. 3, 2014, 10:40 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.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 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
Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- Review request for 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.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 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
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/#review63680 --- Thanks for the patch. Please add someone from kdepim team to review. runners/contacts/contactsrunner.cpp https://git.reviewboard.kde.org/r/119575/#comment44360 Stray new line runners/contacts/contactsrunner.cpp https://git.reviewboard.kde.org/r/119575/#comment44361 Please no spaces inside parentheses Occurs multiple times runners/contacts/contactsrunner.cpp https://git.reviewboard.kde.org/r/119575/#comment44359 Wrong indentation on these two lines runners/contacts/contactsrunner.cpp https://git.reviewboard.kde.org/r/119575/#comment44358 Trailing whitespace - Christoph Feck On Aug. 2, 2014, 2:37 p.m., Marc Deop wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Aug. 2, 2014, 2:37 p.m.) Review request for 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.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 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
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Aug. 2, 2014, 7:16 p.m.) Review request for Plasma. Changes --- Fixes following Christoph Feck's suggestions 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 (updated) - runners/contacts/contactsrunner.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 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
Re: Review Request 119575: Fix krunner contacts pllugin
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119575/ --- (Updated Aug. 2, 2014, 7:18 p.m.) Review request for 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.h 9bcb40d34a40dad414ea5154b745c97c18d6d81b runners/contacts/contactsrunner.cpp 2261e3744c695d046ec95e6dd97f1ad26c800d71 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
