----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68968 -----------------------------------------------------------
Thanks for the patch. Calling init() again surprised me a bit, since this method was never called twice before, but OK, anything else would be much more invasive. I would suggest to at least add a comment e.g. in the call to showPrompt(), to say "this will call init() again once the dialog is closed" or something. Some small things below. src/widgets/executablefileopendialog.h <https://git.reviewboard.kde.org/r/120666/#comment48240> Use 0 or Q_NULLPTR, for portability. src/widgets/executablefileopendialog.cpp <https://git.reviewboard.kde.org/r/120666/#comment48239> should be _p.h since it's internal src/widgets/executablefileopendialog.cpp <https://git.reviewboard.kde.org/r/120666/#comment48241> Any reason for forcing the dialog to be non-resizable? Users prefer to be able to resize them if needed... src/widgets/krun.cpp <https://git.reviewboard.kde.org/r/120666/#comment48225> Missing parent widget. KRun has that somewhere. src/widgets/krun.cpp <https://git.reviewboard.kde.org/r/120666/#comment48224> sender() feels hackish, using a member var would be better. Also because it would allow deleting the dialog if the KRun gets deleted somehow. src/widgets/krun_p.h <https://git.reviewboard.kde.org/r/120666/#comment48227> This doesn't need to be a qobject. Use Q_PRIVATE_SLOT to define slots in the private class. - David Faure On Oct. 22, 2014, 4:10 p.m., Arjun AK wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120666/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2014, 4:10 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kio > > > Description > ------- > > This patch makes KIO show a dialog box asking the user what to do (either > open it using a text editor or execute it) when he clicks on a script or a > desktop file. > > See also: https://git.reviewboard.kde.org/r/120171/ > > > Diffs > ----- > > src/widgets/CMakeLists.txt 4060cdf > src/widgets/executablefileopendialog.h PRE-CREATION > src/widgets/executablefileopendialog.cpp PRE-CREATION > src/widgets/krun.cpp 6ac42da > src/widgets/krun_p.h 69e2e98 > > Diff: https://git.reviewboard.kde.org/r/120666/diff/ > > > Testing > ------- > > > Thanks, > > Arjun AK > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
