Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 29, 2014, 4:22 p.m.) Status -- This change has been marked as submitted. 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/krun.cpp 6ac42da src/widgets/krun_p.h 69e2e98 src/widgets/CMakeLists.txt 4060cdf src/widgets/executablefileopendialog.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h PRE-CREATION 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 1:20 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 (updated) - src/widgets/CMakeLists.txt 4060cdf src/widgets/executablefileopendialog.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review69246 --- src/widgets/executablefileopendialog.cpp https://git.reviewboard.kde.org/r/120666/#comment48410 where is this slot used? src/widgets/krun.cpp https://git.reviewboard.kde.org/r/120666/#comment48411 This should use the 4 args connect, in case the KRun gets deleted before the dialog. i.e. insert ,this as the 3rd arg. src/widgets/krun.cpp https://git.reviewboard.kde.org/r/120666/#comment48412 It's a little bit surprising to see a getter (is prompt needed) modify the KRun object. But OK, I can't think of a better solution right now. much better indeed, thanks - David Faure On Oct. 28, 2014, 7:50 a.m., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 7:50 a.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.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 28, 2014, 3:35 p.m., David Faure wrote: src/widgets/krun.cpp, line 978 https://git.reviewboard.kde.org/r/120666/diff/3/?file=322409#file322409line978 This should use the 4 args connect, in case the KRun gets deleted before the dialog. i.e. insert ,this as the 3rd arg. i.e. insert ,this as the 3rd arg Did you mean `q` ? - Arjun --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review69246 --- On Oct. 28, 2014, 1:20 p.m., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 1:20 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.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 28, 2014, 10:05 a.m., David Faure wrote: src/widgets/krun.cpp, line 978 https://git.reviewboard.kde.org/r/120666/diff/3/?file=322409#file322409line978 This should use the 4 args connect, in case the KRun gets deleted before the dialog. i.e. insert ,this as the 3rd arg. Arjun AK wrote: i.e. insert ,this as the 3rd arg Did you mean `q` ? Yes, q will be fine. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review69246 --- On Oct. 28, 2014, 7:50 a.m., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 7:50 a.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.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 28, 2014, 3:35 p.m., David Faure wrote: src/widgets/executablefileopendialog.cpp, line 64 https://git.reviewboard.kde.org/r/120666/diff/3/?file=322407#file322407line64 where is this slot used? Nowhere. The file was copy-pasted from the other patch - Arjun --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review69246 --- On Oct. 28, 2014, 1:20 p.m., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 1:20 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.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 5:13 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 (updated) - src/widgets/krun.cpp 6ac42da src/widgets/krun_p.h 69e2e98 src/widgets/CMakeLists.txt 4060cdf src/widgets/executablefileopendialog.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h PRE-CREATION 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review69260 --- Ship it! Just one last thing, then push. src/widgets/krun.cpp https://git.reviewboard.kde.org/r/120666/#comment48419 Why not pass dialog-isDontAskAgainChecked() to the slot (which is not really a slot, so the slot prefix should be removed...) Then the member var m_dialogDoNotAskAgainSet won't be needed anymore. - David Faure On Oct. 28, 2014, 11:43 a.m., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 11:43 a.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/krun.cpp 6ac42da src/widgets/krun_p.h 69e2e98 src/widgets/CMakeLists.txt 4060cdf src/widgets/executablefileopendialog.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h PRE-CREATION 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 28, 2014, 1:37 p.m.) Status -- This change has been marked as submitted. 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/krun.cpp 6ac42da src/widgets/krun_p.h 69e2e98 src/widgets/CMakeLists.txt 4060cdf src/widgets/executablefileopendialog.cpp PRE-CREATION src/widgets/executablefileopendialog_p.h PRE-CREATION 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 23, 2014, 11:14 a.m., Emmanuel Pescosta wrote: src/widgets/krun.cpp, line 1013 https://git.reviewboard.kde.org/r/120666/diff/2/?file=321616#file321616line1013 Any reason why using show instead of exec? Exec would avoid the rather complex and error-prone code path. IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. Arjun AK wrote: IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. faure: Are you okay with this? Frank Reininghaus wrote: Note that you have to be *extremely* careful when calling exec(), which runs a nested event loop. Anything can happen inside such a loop, including quitting the application. See https://git.reviewboard.kde.org/r/118858/ and the links in the comments there fore more information. If you replace show() by exec() in your patch, you might get a crash in the line m_dialogNotYetShown = false; if the application quits inside the nested event loop. Emmanuel Pescosta wrote: m_dialogNotYetShown = false; This variable won't be needed anymore. Only the result of exec() is needed. Please please keep the non-modal dialog. Modal dialogs are very annoying - and even more so when called from plasma: suddenly your whole taskbar and desktop is unusable, just because somewhere there's a modal dialog. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68990 --- 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 23, 2014, 1:14 p.m., Emmanuel Pescosta wrote: src/widgets/krun.cpp, line 1013 https://git.reviewboard.kde.org/r/120666/diff/2/?file=321616#file321616line1013 Any reason why using show instead of exec? Exec would avoid the rather complex and error-prone code path. IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. Arjun AK wrote: IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. faure: Are you okay with this? Frank Reininghaus wrote: Note that you have to be *extremely* careful when calling exec(), which runs a nested event loop. Anything can happen inside such a loop, including quitting the application. See https://git.reviewboard.kde.org/r/118858/ and the links in the comments there fore more information. If you replace show() by exec() in your patch, you might get a crash in the line m_dialogNotYetShown = false; if the application quits inside the nested event loop. Emmanuel Pescosta wrote: m_dialogNotYetShown = false; This variable won't be needed anymore. Only the result of exec() is needed. David Faure wrote: Please please keep the non-modal dialog. Modal dialogs are very annoying - and even more so when called from plasma: suddenly your whole taskbar and desktop is unusable, just because somewhere there's a modal dialog. and even more so when called from plasma: suddenly your whole taskbar and desktop is unusable This is indeed really bad. - Emmanuel --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68990 --- On Oct. 22, 2014, 6: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, 6: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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 23, 2014, 11:43 a.m., David Faure wrote: 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. Calling init() again surprised me a bit, since this method was never called twice before Why not splitting up this method? Just renaming the old init method and hide it, then create a new init function which checks if a dialog is needed or not and load the settings. The old init function can then be called in the dialog finished handler, or directly in the new init method when no dialog is needed at all. anything else would be much more invasive And I don't think this will be too invasive ;) - Emmanuel --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68968 --- On Oct. 22, 2014, 6: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, 6: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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120666/#review68987
---
src/widgets/executablefileopendialog.h
https://git.reviewboard.kde.org/r/120666/#comment48253
m_
src/widgets/krun.cpp
https://git.reviewboard.kde.org/r/120666/#comment48252
Lambda expression maybe?
connect(dialog, ExecutableFileOpenDialog::finished, [this, dialog](int
result) {
...
}
Avoids the cast hack and the need of a member variable.
- Emmanuel Pescosta
On Oct. 22, 2014, 6: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, 6: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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68990 --- src/widgets/krun.cpp https://git.reviewboard.kde.org/r/120666/#comment48256 Any reason why using show instead of exec? Exec would avoid the rather complex and error-prone code path. IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. - Emmanuel Pescosta On Oct. 22, 2014, 6: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, 6: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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 23, 2014, 4:44 p.m., Emmanuel Pescosta wrote: src/widgets/krun.cpp, line 1013 https://git.reviewboard.kde.org/r/120666/diff/2/?file=321616#file321616line1013 Any reason why using show instead of exec? Exec would avoid the rather complex and error-prone code path. IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. faure: Are you okay with this? - Arjun --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68990 --- On Oct. 22, 2014, 9:40 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, 9:40 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Okt. 23, 2014, 11:14 vorm., Emmanuel Pescosta wrote: src/widgets/krun.cpp, line 1013 https://git.reviewboard.kde.org/r/120666/diff/2/?file=321616#file321616line1013 Any reason why using show instead of exec? Exec would avoid the rather complex and error-prone code path. IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. Arjun AK wrote: IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. faure: Are you okay with this? Note that you have to be *extremely* careful when calling exec(), which runs a nested event loop. Anything can happen inside such a loop, including quitting the application. See https://git.reviewboard.kde.org/r/118858/ and the links in the comments there fore more information. If you replace show() by exec() in your patch, you might get a crash in the line m_dialogNotYetShown = false; if the application quits inside the nested event loop. - Frank --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68990 --- On Okt. 22, 2014, 4:10 nachm., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Okt. 22, 2014, 4:10 nachm.) 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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
On Oct. 23, 2014, 1:14 p.m., Emmanuel Pescosta wrote: src/widgets/krun.cpp, line 1013 https://git.reviewboard.kde.org/r/120666/diff/2/?file=321616#file321616line1013 Any reason why using show instead of exec? Exec would avoid the rather complex and error-prone code path. IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. Arjun AK wrote: IMHO we should prefer a blocking dialog in this case, because it asks the user for permission. faure: Are you okay with this? Frank Reininghaus wrote: Note that you have to be *extremely* careful when calling exec(), which runs a nested event loop. Anything can happen inside such a loop, including quitting the application. See https://git.reviewboard.kde.org/r/118858/ and the links in the comments there fore more information. If you replace show() by exec() in your patch, you might get a crash in the line m_dialogNotYetShown = false; if the application quits inside the nested event loop. m_dialogNotYetShown = false; This variable won't be needed anymore. Only the result of exec() is needed. - Emmanuel --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/#review68990 --- On Oct. 22, 2014, 6: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, 6: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
Re: Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- (Updated Oct. 22, 2014, 9:40 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 (updated) - 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
Review Request 120666: Get user's permission before executing scripts or desktop files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120666/ --- 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
