Review Request: Import Kopete logs when adding new account in KCM

[Daniele Elmo Domenichelli]

> On Nov. 9, 2012, 3:08 p.m., Daniele Elmo Domenichelli wrote:
> > src/kcm-telepathy-accounts.cpp, line 397
> > <http://git.reviewboard.kde.org/r/107246/diff/3/?file=94492#file94492line397>
> >
> >     For some reason I don't remember, I think that krazy gives a warning when you do this, you should use instead QWeakPointer<KDialog>
> >     Have a look at this: http://quickgit.kde.org/index.php?p=ktp-accounts-kcm.git&a=commitdiff&h=ddc8b8f91a0623ae11910f468b3f7ffc7dc0a27b
> 
> Dan Vrátil wrote:
>     In this case ownership of the dialog is transferred to KMessageBox::createKMessageBox() (l. 402), so the weakpointer would be of no help here.
> 
> David Edmundson wrote:
>     You're absolutely right. 
>     
>     In which case I would suggest:
>     KDialog *dialog = new KDialog(0); //ownership is passed to KMessageBox::createMessageBox();
> 
> Dan Vrátil wrote:
>     Sorry, "ownership" is not what I meant :) The dialog should still be owned by KCMTelepathyAccounts, what KMessageBox::createKMessageBox() does is that it wraps it in QPointer before calling exec(), thus making sure that it does not crash when parent is deleted:
>     
>     QPointer<KDialog> guardedDialog = dialog;
>     const int result = guardedDialog->exec();
>     ....
>     delete (KDialog *) guardedDialog;

Oh I didn't know that... But perhaps that crazy will spot that anyway. In case we can suppress crazy warning later.
Anyway a comment "KDialog *dialog = new KDialog(this); // this is deleted later by createKMessageBox" might be useful, or someone (for example me as soon as I forget about this) might add "delete dialog;" :P


- Daniele Elmo


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


On Nov. 9, 2012, 2 p.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107246/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2012, 2 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> When you add a new account in the KCM, it will search for corresponding Kopete logs and ask whether to import them (and eventually import them).
> 
> 
> Bonus points for me: when you remove an account, you will be prompted whether you want to remove logs as well.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt b4248fb 
>   src/CMakeLists.txt ee82fd0 
>   src/kcm-telepathy-accounts.h 29629eb 
>   src/kcm-telepathy-accounts.cpp 6389a67 
> 
> Diff: http://git.reviewboard.kde.org/r/107246/diff/
> 
> 
> Testing
> -------
> 
> Added account I had on Kopete to KTp, got prompted whether to import logs, confirmed, logs imported.
> Removed account, got prompted whether to clear logs, confirmed, logs gone.
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20121110/6e12d7d8/attachment.html>