[Kde-pim] Review Request 117834: kaddressbook: add a category (tag) display filter

Fri May 2 08:58:24 BST 2014

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryfilterproxymodel.cpp, line 56
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270605#file270605line56>
> >
> >     directly in constructor:
> >     Category..Private::Category..Private(..)
> >      : QObject(),
> >        filterEnabled(false),
> >        qptr(parent) 
> >     {
> >     }

Done

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryfilterproxymodel.cpp, line 81
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270605#file270605line81>
> >
> >     return true; 
> >     not necessary to add parentheses
> >     
> >     same for other

Done throughout

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryfilterproxymodel.cpp, line 96
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270605#file270605line96>
> >
> >     const ?

Done

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryfilterproxymodel.cpp, line 98
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270605#file270605line98>
> >
> >     const here too

Fixed - not sure whether consts such as this and the previous make much difference to the code, but they do indicate the programmer's intention.

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryfilterproxymodel.cpp, line 129
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270605#file270605line129>
> >
> >     if (idList != d->filterIdList) {
> >       ...
> >     }

Fixed.  Not convinced here that this is a worthwhile optimisation, as in most cases the list will indeed have changed and the comparison is not trivial (it's OK for POD as below).

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryfilterproxymodel.cpp, line 140
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270605#file270605line140>
> >
> >     if (d->filterEnabled != enable) {
> >     
> >     }

Done

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryselectwidget.h, line 31
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270606#file270606line31>
> >
> >     4.14

Done

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryselectwidget.h, line 35
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270606#file270606line35>
> >
> >     Please don't use KHBox it's deprecated in KF5.
> >     Use a QWidget + QHboxLayout

Done

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryselectwidget.h, line 58
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270606#file270606line58>
> >
> >     Why -2 ? -3 ? -4 ?

Valid Tag::Id values start from 0 upwards, -1 is used to indicate an invalid tag.

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/categoryselectwidget.cpp, line 35
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270607#file270607line35>
> >
> >     Not necessary to use ../libkdepim
> >     if it doesn't find this includes adapt cmakelists.txt

Done -> <widgets/kcheckcombobox.h>

> On May 1, 2014, 3:48 p.m., Laurent Montel wrote:
> > kaddressbook/mainwidget.h, line 32
> > <https://git.reviewboard.kde.org/r/117834/diff/2/?file=270609#file270609line32>
> >
> >     Why ? you don't add new KToggleAction

Left in by mistake, removed (but still in diff r3, will fix next time).

- Jonathan

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

On May 2, 2014, 7:58 a.m., Jonathan Marten wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117834/
> -----------------------------------------------------------
> 
> (Updated May 2, 2014, 7:58 a.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Bugs: 332103
>     http://bugs.kde.org/show_bug.cgi?id=332103
> 
> 
> Repository: kdepim
> 
> 
> Description
> -------
> 
> There is currently no GUI to select or filter contacts by categories (tags).  Apart from the unituitive workaround of typing the tag name in the quick filter box (which doesn't work well with post-Baloo contacts which store the contact's categories as an Akonadi URL instead of the name).
> 
> This change adds a list of currently defined categories to the main layout, with additional entries for "all" and special categories.  The contacts list is filtered by checked categories in this list.  To avoid confusion, there is no filtering in simple (1-column) mode or if the category list is hidden.  The context menu over this list has quick options to check or clear all of the categories.
> 
> 
> Diffs
> -----
> 
>   kaddressbook/categoryselectwidget.h PRE-CREATION 
>   kaddressbook/categoryfilterproxymodel.cpp PRE-CREATION 
>   kaddressbook/categoryfilterproxymodel.h PRE-CREATION 
>   kaddressbook/CMakeLists.txt a22e731 
>   kaddressbook/categoryselectwidget.cpp PRE-CREATION 
>   kaddressbook/kaddressbookui.rc 6f4a774 
>   kaddressbook/mainwidget.h 0075edf 
>   kaddressbook/mainwidget.cpp 804a5f0 
>   kaddressbook/settings/kaddressbook.kcfg f3a3e38 
> 
> Diff: https://git.reviewboard.kde.org/r/117834/diff/
> 
> 
> Testing
> -------
> 
> Built kaddressbook with this change, tested on my personal address book (several hundred entries and approximately 20 category tags).
> 
> 
> File Attachments
> ----------------
> 
> Screen shot 1
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/04/28/37775637-78da-425f-85a4-a22b5ca52d9c__kaddressbook-categoryfilter_1.png
> Screen shot 2
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/04/28/0189e057-9250-40f6-aefd-d0ed2ea5be65__kaddressbook-categoryfilter_2.png
>  Screen shot for r2 - filter in quick search bar 
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/01/34908029-098a-40d9-a1b9-eadfbf71a734__kaddressbook-categoryfilter_3.png
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

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