[Kde-pim] Review Request 115846: kontact summary view

[Eugenio Accorsi]

> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/apptsummarywidget.cpp, line 160
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244664#file244664line160>
> >
> >     Are you sure this needs to be translated?
> >     All uppercase strings look like identifiers to me, but I am not familiar with the underlying data type

it seems that they need to be translated since that command match the translated categories by name. I tested locally using the traslated string and it works.


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/apptsummarywidget.cpp, line 244
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244664#file244664line244>
> >
> >     I guess you can remove that line then and use item.id() in places currently using id

fixed


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/apptsummarywidget.cpp, line 252
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244664#file244664line252>
> >
> >     please use block braces {} around each branch's body

fixed


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/apptsummarywidget.cpp, line 270
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244664#file244664line270>
> >
> >     please use block braces {} around each branch's body

fixed


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/kcmapptsummary.cpp, line 42
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244666#file244666line42>
> >
> >     is this an unrelated change that crept into this diff?

i deleted that because they are not used, these variables are defined in the ui file.


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/kcmapptsummary.cpp, line 162
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244666#file244666line162>
> >
> >     what is the reason for the value change here?

thats an unrelated change, i reverted it.


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/summaryeventinfo.cpp, line 209
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244668#file244668line209>
> >
> >     please use block braces {} around branch's body

fixed


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/korganizer/summaryeventinfo.cpp, line 324
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244668#file244668line324>
> >
> >     I am not sure how expensive the getNextDateTime() call is but sinde the result is needed again later it might make sense to assign it to a local variable

fixed.


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/specialdates/sdsummarywidget.cpp, line 368
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244670#file244670line368>
> >
> >     does the comment no longer apply?

i think this check is to prevent the following case: one event from the KABC-BIRTHADY/ANNIVERSARY and one from the contact. but since i check it above i think that it's not needed anymore


> On Feb. 18, 2014, 4:49 p.m., Kevin Krammer wrote:
> > kontact/plugins/specialdates/sdsummarywidget.cpp, line 667
> > <https://git.reviewboard.kde.org/r/115846/diff/1/?file=244670#file244670line667>
> >
> >     Hmm. That will go through all contacts. I think the special job was to prevent that, i.e. do not require tons of contacts to be loaded that then do not contains birthday information

i changed it to fecth all contact now using a different approach and it's faster (but i have only ~100 contacts and so cannot test with more).
the problem with the previous job is that i does not work every time for me (and also it is deprecated now).


- Eugenio


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


On Feb. 19, 2014, 10:43 a.m., Eugenio Accorsi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115846/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2014, 10:43 a.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Bugs: 278956
>     http://bugs.kde.org/show_bug.cgi?id=278956
> 
> 
> Repository: kdepim
> 
> 
> Description
> -------
> 
> since i use kontact very often i tried to make the summary view work as expected.
> the only problem with this patch is the special event plugin.
> maybe someone with better experience with akonadi can steer me in the right direction..
> i changed the old implementation because it doesn't work for every contact.
> 
> 
> Diffs
> -----
> 
>   kontact/plugins/korganizer/apptsummaryconfig_base.ui 93e72bd 
>   kontact/plugins/korganizer/apptsummarywidget.h 95bfe3b 
>   kontact/plugins/korganizer/apptsummarywidget.cpp 33fff04 
>   kontact/plugins/korganizer/kcmapptsummary.h bddab14 
>   kontact/plugins/korganizer/kcmapptsummary.cpp 6a661a4 
>   kontact/plugins/korganizer/summaryeventinfo.h a9bb775 
>   kontact/plugins/korganizer/summaryeventinfo.cpp 00c9906 
>   kontact/plugins/specialdates/kcmsdsummary.h 778c389 
>   kontact/plugins/specialdates/kcmsdsummary.cpp 3b9c5a4 
>   kontact/plugins/specialdates/sdsummarywidget.h 9076c79 
>   kontact/plugins/specialdates/sdsummarywidget.cpp 67990c3 
> 
> Diff: https://git.reviewboard.kde.org/r/115846/diff/
> 
> 
> Testing
> -------
> 
> in the special-occasion plugin enabling the search for special events in contacts will result in high cpu usage by nepomuk.
> i tried also with master but it does not find any contacts.
> the summary event plugin works as expected.
> 
> 
> Thanks,
> 
> Eugenio Accorsi
> 
>

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