Review Request: Make KNewFileMenu working asynchronously (don't block plasma shell from folderview)

[David Faure]

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


Difficult patch to review, since all the code moved around, but it looks good overall.
One thing that bothers me is m_strategy. You got rid of the actual use of the strategy pattern (subclasses of a base strategy class) -- too bad, but I'll survive -- so why keep the strategy member and the Strategy class? They don't really serve any purpose anymore, other than minimizing the code changes a little bit. OK they keep some related variables together, so I guess it's better than just folding these vars into knewfilemenuprivate.
So I would recommend simply storing m_strategy by value rather than using new+delete. My comments below are from before I realized that, but they show the various places where it's leaked currently.


/trunk/KDE/kdelibs/kfile/knewfilemenu.cpp
<http://svn.reviewboard.kde.org/r/4964/#comment7599>

    Shouldn't this delete m_strategy, too? It seems that the strategy is leaked if executeStrategy() never gets called.



/trunk/KDE/kdelibs/kfile/knewfilemenu.cpp
<http://svn.reviewboard.kde.org/r/4964/#comment7607>

    



/trunk/KDE/kdelibs/kfile/knewfilemenu.cpp
<http://svn.reviewboard.kde.org/r/4964/#comment7601>

    Please use a static_cast<> rather than a  C cast.



/trunk/KDE/kdelibs/kfile/knewfilemenu.cpp
<http://svn.reviewboard.kde.org/r/4964/#comment7604>

    All the exit paths from this method leak m_strategy too, AFAICS.
    
    Maybe it should be put into a QScopedPointer at the beginning of such methods ... hmm, but then executeStrategy can't delete it anymore.



/trunk/KDE/kdelibs/kfile/knewfilemenu.cpp
<http://svn.reviewboard.kde.org/r/4964/#comment7605>

    Call me nitpicker, but for clean object-oriented code, it's usually much better to define a KDialog subclass rather than new KDialog + new layout + new widgets. Makes the code more reuseable etc.
    Well, not a blocker for acceptance, just my 2 cents.


- David


On 2010-08-13 17:57:09, Björn Ruberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/4964/
> -----------------------------------------------------------
> 
> (Updated 2010-08-13 17:57:09)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Summary
> -------
> 
> This patch makes the dialogs created by KNewFileMenu asynchronously. It's now possible to get non-modal dialogs with it. This is a feature that is needed for plasma folderview, because modal dialogs block the whole plasma shell. So this is the main part of a fix for bug #167243.
> By the default the dialogs will stay modal, so there should be no change for other applications. Folderview just needs to call setModal(false) on the KNewFileMenu (have a patch for that, too). But as this touches much code (and orders methods alphabetically ;) ) you can never be sure that there is no change.
> 
> Please have closer look whether the dialogs are deleted probably. I'm not doing it directly but relying on Qt::WA_DeleteOnClose. 
> 
> 
> This addresses bug #167243.
>     https://bugs.kde.org/show_bug.cgi?id=#167243
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kfile/knewfilemenu.h 1161027 
>   /trunk/KDE/kdelibs/kfile/knewfilemenu.cpp 1161027 
> 
> Diff: http://svn.reviewboard.kde.org/r/4964/diff
> 
> 
> Testing
> -------
> 
> There are five different code pathes that I have worked on:
> - create directory
> - create normal file
> - create symlink
> - create URL 
> - create .desktop file 
> 
> I tested all of them and they work for me.
> 
> 
> Thanks,
> 
> Björn
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100903/834de431/attachment.htm>