Review Request: Mitigate potential crashes associated with the use of QDialog::exec in kdelibs

[Dawit Alemayehu]

> On May 1, 2012, 6:15 p.m., Lamarque Vieira Souza wrote:
> > kdeui/colors/kcolordialog.cpp, line 1484
> > <http://git.reviewboard.kde.org/r/104802/diff/1/?file=61677#file61677line1484>
> >
> >     You should use QWeakPointer instead of QPointer.

There really is no point in doing that here except to introduce changes in more lines of code. These are not performance critical code paths and as such I fail to see what is gained by using QWeakPointer which requires more work (read: more lines to change).


> On May 1, 2012, 6:15 p.m., Lamarque Vieira Souza wrote:
> > kio/kfile/kfiledialog.cpp, line 561
> > <http://git.reviewboard.kde.org/r/104802/diff/1/?file=61692#file61692line561>
> >
> >     stick to the code style please.

huh ? You do know that is exactly how it was written before right ? Can we please review the changes on their merits ? I do not care about styling issues that already existed in the document being modified. It is up to the original author to change those or for kdelibs to enforce it using some mechanism upon commit. If I write new code from scratch I will follow whatever guideline is set, otherwise butchering the style used in existing code only serves to make it more difficult to read the code.


> On May 1, 2012, 6:15 p.m., Lamarque Vieira Souza wrote:
> > kio/kfile/kurlrequesterdialog.cpp, line 133
> > <http://git.reviewboard.kde.org/r/104802/diff/1/?file=61695#file61695line133>
> >
> >     replace tab to spaces.

For whomever wants to review these patches. Please review on the merit of the code. I do not care about styling issues that already existed in the document that was modified. It is a pointless waste of time and effort to review those unless they are completely outrageous. Thank you.


- Dawit


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


On May 1, 2012, 4:37 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104802/
> -----------------------------------------------------------
> 
> (Updated May 1, 2012, 4:37 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> This patch attempts to mitigate the unintended crashes that might result from using QDialog::exec in kdelibs. Since nested event loops are potential sources of inadvertent crashes, this patch attempts to prevent that by changing how dialogs are created in kdelibs. All blocking dialog calls, i.e. those that invoke QDialog.exec(), are wrapped with QPointer and the QPointer is checked once QDialog.exec returns. See http://www.kdedevelopers.org/node/3919 for more details.
> 
> Note that I am aware of other classes that create nested event loops (e.g. QProcess), but this fix is only applicable to QDialog usage.
> 
> 
> Diffs
> -----
> 
>   kdeui/colors/kcolordialog.cpp 95bb7f5 
>   kdeui/dialogs/kedittoolbar.cpp bb80952 
>   kdeui/dialogs/kinputdialog.cpp 2801c00 
>   kdeui/dialogs/kpixmapregionselectordialog.cpp 11d964b 
>   kdeui/dialogs/kshortcutsdialog.cpp a73f8f2 
>   kdeui/dialogs/kshortcutseditor.cpp 5984a9d 
>   kdeui/findreplace/kfinddialog.cpp de2dd90 
>   kdeui/fonts/kfontdialog.cpp 9bea490 
>   kdeui/widgets/ktextedit.cpp 1e58706 
>   kdeui/xmlgui/kmenumenuhandler_p.cpp d8c82b6 
>   kfile/kdiroperator.cpp 18ffc34 
>   kfile/kdirselectdialog.cpp e0dcafa 
>   kfile/kfileplaceeditdialog.cpp 5537551 
>   kio/kfile/kacleditwidget.cpp d89429f 
>   kio/kfile/kencodingfiledialog.cpp 4686065 
>   kio/kfile/kfiledialog.cpp d121e4d 
>   kio/kfile/kicondialog.cpp b7d646f 
>   kio/kfile/kpropertiesdialog.cpp feb0c9e 
>   kio/kfile/kurlrequesterdialog.cpp 8ee29e1 
>   kio/kio/jobuidelegate.cpp 85679c2 
>   kio/kio/kbuildsycocaprogressdialog.cpp fba30ec 
>   kio/kio/passworddialog.cpp faf0c77 
>   kio/kio/paste.cpp ca451fb 
>   kio/kssl/kcm/cacertificatespage.cpp 0a269a3 
>   knewstuff/knewstuff2/ui/downloaddialog.cpp b4d2dcd 
>   knewstuff/knewstuff2/ui/kdxsbutton.cpp e8f8c83 
>   knewstuff/knewstuff3/knewstuffbutton.cpp 9c14e99 
>   kparts/browserrun.cpp c89829d 
>   kutils/kpluginselector.cpp 505e53f 
>   nepomuk/ui/tagwidget.cpp 7c59922 
>   nepomuk/utils/searchwidget.cpp f46e72a 
> 
> Diff: http://git.reviewboard.kde.org/r/104802/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

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