> On May 2, 2012, 3:20 p.m., David Faure wrote: > > kio/kfile/kpropertiesdialog.cpp, line 332 > > <http://git.reviewboard.kde.org/r/104802/diff/1/?file=61694#file61694line332> > > > > Nice one, deleting the dialog immediately after show() ! > > > > See, this is exactly why I'm against such massive changes. It's just > > too easy to slip in an untested change, which introduces a regression. > > > > So instead of a fix for a bug that did not actually happen, we have a > > regression that does happen. Not good. > > > > I'm veto'ing this for 4.8. > > > > At most, review it again to ensure it *only* changes code paths that > > call exec(), *test it*, and put it in the frameworks branch only. > > Yes it's hard to test so many code paths at once, so let's only change > > code paths one by one, after testing them.
Now this is exactly why posting patches to reviewboard is always a good idea. You are correct on both counts. And all this time I was wondering why the properties dialog quit showing up in Konqueror and Dolphin for me. *sigh*. Anyways, I withdraw the patch. Not only do I now completely agree that such changes should be done one at a time, but also they should be done only when the situation warrants it. That is the offending code must responsible for a reproducible crash. - Dawit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104802/#review13274 ----------------------------------------------------------- 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 > >
