[Kde-pim] Review Request 111101: Use QWeakPointer to store move|copy and selectfolder dialogs

[Kevin Krammer]

> On June 18, 2013, 4:15 p.m., Kevin Krammer wrote:
> > kmail/kmmainwidget.h, line 631
> > <http://git.reviewboard.kde.org/r/111101/diff/1/?file=164245#file164245line631>
> >
> >     not QPointer?
> 
> Aurélien Gâteau wrote:
>     According to the doc [1], QWeakPointer is supposed to be the new QPointer, faster, more explicit and... more painful to use (because it requires data()). I don't think it makes a lot of difference here though, so I am happy to use QPointer if you prefer.
>     
>     [1]: http://qt-project.org/doc/qt-4.8/qweakpointer.html

The thing is, QWeakPointer got QObject tracking because QPointer's implementation was deemed to slow (which hardly matters in this case), but that was fixed in Qt5. So in Qt5 QPointer is again the preferred way of tracking QObjects. Using QWeakPointer now will just add a porting step that would otherwise not be required.


> On June 18, 2013, 4:15 p.m., Kevin Krammer wrote:
> > kmail/kmmainwidget.cpp, line 2069
> > <http://git.reviewboard.kde.org/r/111101/diff/1/?file=164246#file164246line2069>
> >
> >     is that necessary?
> >     I thought the smart pointers had an overloaded operator->() to make them appear like normal pointers
> 
> Sergio Luis Martins wrote:
>     It's necessary for QWP (not QP though)

I see, one more reason to use QPointer


> On June 18, 2013, 4:15 p.m., Kevin Krammer wrote:
> > kmail/kmmainwidget.cpp, line 2090
> > <http://git.reviewboard.kde.org/r/111101/diff/1/?file=164246#file164246line2090>
> >
> >     can't you keep the condition conceptually and thus keep the body unchanged?
> >     something like
> >     if ( dialog->exec() && dialog )
> >     ?
> 
> Aurélien Gâteau wrote:
>     I felt it was more readable by breaking the condition in two parts: it is more explicit to me that the dialog might have gone away after the call to exec(). No strong opinion on this though, I can revert if you prefer.

I don't care either way, was just wondering about the code change when it would have been possible to just extend the condition. So I'd say it's Laurent's decision


- Kevin


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


On June 18, 2013, 7:30 p.m., Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111101/
> -----------------------------------------------------------
> 
> (Updated June 18, 2013, 7:30 p.m.)
> 
> 
> Review request for KDEPIM and David Faure.
> 
> 
> Description
> -------
> 
> The code showing the move|copy and select folder dialogs tries to check the dialogs are not deleted while they are visible, but it actually recreates them instead of checking their existence. The patch changes this to use QWeakPointer to track the dialogs instead.
> 
> 
> Diffs
> -----
> 
>   kmail/kmmainwidget.h 977e1e13a85d703f71c234697d5cf7fe72577843 
>   kmail/kmmainwidget.cpp 36e7164a58d59cd35f296b9ab6885f1de8c44572 
> 
> Diff: http://git.reviewboard.kde.org/r/111101/diff/
> 
> 
> Testing
> -------
> 
> Dialogs still show and work correctly.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

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