[Kde-pim] Review Request: new kmail filter : add to address book

[Bruno Bigras]

> On 2009-07-16 17:35:05, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmfilteraction.cpp, line 2233
> > <http://reviewboard.kde.org/r/1038/diff/2/?file=8543#file8543line2233>
> >
> >     This looks messy? Why not use a grid layout instead of all there complicated nested layouts?

I wish I knew about QGridLayout before making that embarrassing change :)


> On 2009-07-16 17:35:05, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmfilteraction.cpp, line 57
> > <http://reviewboard.kde.org/r/1038/diff/2/?file=8543#file8543line57>
> >
> >     Please use CamelCase headers.
> >     But is this actually needed?

It's not needed.


> On 2009-07-16 17:35:05, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmfilteraction.cpp, line 2162
> > <http://reviewboard.kde.org/r/1038/diff/2/?file=8543#file8543line2162>
> >
> >     Shouldn't the maps be cleared here, so that there are no duplicate resources when calling this multiple times?

I did a little test and it don't make duplicate ressources, maybe if the key is already in there. But I think it should be safer to clear them anyways.


> On 2009-07-16 17:35:05, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmfilteraction.cpp, line 2378
> > <http://reviewboard.kde.org/r/1038/diff/2/?file=8543#file8543line2378>
> >
> >     "" -> QString()

Is .clear() ok?


> On 2009-07-16 17:35:05, Thomas McGuire wrote:
> > /trunk/KDE/kdepim/kmail/kmfilteraction.cpp, line 2223
> > <http://reviewboard.kde.org/r/1038/diff/2/?file=8543#file8543line2223>
> >
> >     Shouldn't this return ErrorButGoOn or whatever the code for that is?

I think you're right.


- Bruno


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1038/#review1626
-----------------------------------------------------------


On 2009-07-16 23:19:28, Bruno Bigras wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1038/
> -----------------------------------------------------------
> 
> (Updated 2009-07-16 23:19:28)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> This is a port of a commit by Christian Schaarschmidt https://bugs.kde.org/show_bug.cgi?id=47333#c19 from the kdepim-3.5.5+ branch.
> 
> It add an "add to address book" filter action.
> 
> I tried to make the ui look better when the window is not maximized (see the two screenshots) but the first combo box (the one to select "From", "To", "CC", "BCC") don't look so good in the center.
> 
> 
> This addresses bug 47333.
>     https://bugs.kde.org/show_bug.cgi?id=47333
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/kmfilteraction.cpp 998013 
> 
> Diff: http://reviewboard.kde.org/r/1038/diff
> 
> 
> Testing
> -------
> 
> I tested this successfully when receiving emails and when sending to multiples recipients.
> 
> 
> Screenshots
> -----------
> 
> before
>   http://reviewboard.kde.org/r/1038/s/143/
> after
>   http://reviewboard.kde.org/r/1038/s/144/
> 
> 
> Thanks,
> 
> Bruno
> 
>

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