Re: Review Request 120720: Open X-DocPath entries from Help button kcmshell5 in KHelpcenter
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120720/ --- (Updated Oct. 28, 2014, 7:37 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Repository: kcmutils Description --- For X-DocPath entries like foo/index.html or kcontrol/foo/index.html QUrl::fromUserInput() returns http://foo/index.html; or http://kcontrol/foo/index.html; Therefore without the patch always a Browser is launched with http://foo/index.html; Diffs - src/kcmultidialog.cpp 19d312d Diff: https://git.reviewboard.kde.org/r/120720/diff/ Testing --- Started a few kcms via kcmshell5, clicked the Help button and the docbooks are displayed in KHelpcenter now. Thanks, Burkhard Lück ___ 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 120794: Implement Unique Mode Properly in Rekonq
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/#review69243 --- src/application.h https://git.reviewboard.kde.org/r/120794/#comment48403 should be const QStringList arguments src/application.cpp https://git.reviewboard.kde.org/r/120794/#comment48404 that's definitely in the wrong place, if it ever gets activated. You want session restoration only in main, not every time activateRequested is called. src/application.cpp https://git.reviewboard.kde.org/r/120794/#comment48405 It's a bit confusing because QCLP::process() is not the same, and also because of the other processCmdLine method here. Maybe call this one handleCmdLine()? Sorry for nitpicking, but if we are going to replace this in most other apps, might as well be perfect :-) parse = just the parsing process = parse and handle the builtin options (in QLCP) handle = handle the app options As far as the slot is concerned, it's ok to say that Application::processCmdLine is parse + handle the app options, it's builtin to Application. So I would rename this method, but leave the other one (processCmdLine(arguments)) as it is. src/application.cpp https://git.reviewboard.kde.org/r/120794/#comment48409 Note that you might want to use the cwd in order to resolve urls with it, so that rekonq localfile.html works. This requires QUrl::fromUserInput(two args) from Qt 5.4 though, so probably too early to do right now, but you can at least add a comment, if not a #if QT_VERSION check. src/main.cpp https://git.reviewboard.kde.org/r/120794/#comment48406 why not move this to the Application constructor too? i.e. do all the setup of the parser in a single place? I guess only the interaction with about stays here. Alternatively (and because not all apps derive from QApplication), I would just leave it all in main (where it is expected, in most apps), and do app.setParser(parser). In addition to having all the setup in one place, the thing I'm trying to solve here is the getCmdLineParser method, which doesn't look so great in terms of encapsulation. getters/setters are always better than returning a ref to an internal member variable. src/main.cpp https://git.reviewboard.kde.org/r/120794/#comment48407 app.connect -- QObject::connect. You're using the 4-args connect, which is a static method. - David Faure On Oct. 28, 2014, 5:44 a.m., David Narváez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/ --- (Updated Oct. 28, 2014, 5:44 a.m.) Review request for KDE Frameworks and rekonq. Repository: rekonq Description --- This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not. The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that. Diffs - src/application.h 7ccd60d src/application.cpp c7c297d src/main.cpp 7592f7a Diff: https://git.reviewboard.kde.org/r/120794/diff/ Testing --- 1. Open one Rekonq window 2. Try opening Rekonq again 3. Try opeining Rekonq from a command line with some URLs 4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in) 5. Open rekonq from the console using rekonq --incognito 6. Open rekonq from the console using reknoq --webapp twitter.com Before this patch, nothing happens in steps 2 - 6. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes
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
Jenkins build is back to stable : kservice_master_qt5 #195
See http://build.kde.org/job/kservice_master_qt5/195/changes ___ 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 120794: Implement Unique Mode Properly in Rekonq
On Oct. 28, 2014, 9:32 a.m., David Faure wrote: src/application.cpp, line 785 https://git.reviewboard.kde.org/r/120794/diff/2/?file=322403#file322403line785 It's a bit confusing because QCLP::process() is not the same, and also because of the other processCmdLine method here. Maybe call this one handleCmdLine()? Sorry for nitpicking, but if we are going to replace this in most other apps, might as well be perfect :-) parse = just the parsing process = parse and handle the builtin options (in QLCP) handle = handle the app options As far as the slot is concerned, it's ok to say that Application::processCmdLine is parse + handle the app options, it's builtin to Application. So I would rename this method, but leave the other one (processCmdLine(arguments)) as it is. This is up for review precisely to get it nitpicked :) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/#review69243 --- On Oct. 28, 2014, 5:44 a.m., David Narváez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/ --- (Updated Oct. 28, 2014, 5:44 a.m.) Review request for KDE Frameworks and rekonq. Repository: rekonq Description --- This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not. The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that. Diffs - src/application.h 7ccd60d src/application.cpp c7c297d src/main.cpp 7592f7a Diff: https://git.reviewboard.kde.org/r/120794/diff/ Testing --- 1. Open one Rekonq window 2. Try opening Rekonq again 3. Try opeining Rekonq from a command line with some URLs 4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in) 5. Open rekonq from the console using rekonq --incognito 6. Open rekonq from the console using reknoq --webapp twitter.com Before this patch, nothing happens in steps 2 - 6. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Thanks, David Narváez ___ 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 120794: Implement Unique Mode Properly in Rekonq
On Oct. 28, 2014, 9:32 a.m., David Faure wrote: src/application.cpp, line 789 https://git.reviewboard.kde.org/r/120794/diff/2/?file=322403#file322403line789 Note that you might want to use the cwd in order to resolve urls with it, so that rekonq localfile.html works. This requires QUrl::fromUserInput(two args) from Qt 5.4 though, so probably too early to do right now, but you can at least add a comment, if not a #if QT_VERSION check. Local URL handling is indeed broken, I added a TODO comment to work on it later, but added parameters that would be missing if I wanted to implement this properly in the handleCmdLine method - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/#review69243 --- On Oct. 28, 2014, 2 p.m., David Narváez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/ --- (Updated Oct. 28, 2014, 2 p.m.) Review request for KDE Frameworks and rekonq. Repository: rekonq Description --- This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not. The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that. Diffs - src/application.h 7ccd60d src/application.cpp c7c297d src/main.cpp 7592f7a Diff: https://git.reviewboard.kde.org/r/120794/diff/ Testing --- 1. Open one Rekonq window 2. Try opening Rekonq again 3. Try opeining Rekonq from a command line with some URLs 4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in) 5. Open rekonq from the console using rekonq --incognito 6. Open rekonq from the console using reknoq --webapp twitter.com 7. Open rekonq from the console pointing it to some local HTML file Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken. Thanks, David Narváez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120794: Implement Unique Mode Properly in Rekonq
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/ --- (Updated Oct. 28, 2014, 2 p.m.) Review request for KDE Frameworks and rekonq. Changes --- Addressed most of the issues raised by dfaure, except local URL handling which is still broken. Repository: rekonq Description --- This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not. The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that. Diffs (updated) - src/application.h 7ccd60d src/application.cpp c7c297d src/main.cpp 7592f7a Diff: https://git.reviewboard.kde.org/r/120794/diff/ Testing (updated) --- 1. Open one Rekonq window 2. Try opening Rekonq again 3. Try opeining Rekonq from a command line with some URLs 4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in) 5. Open rekonq from the console using rekonq --incognito 6. Open rekonq from the console using reknoq --webapp twitter.com 7. Open rekonq from the console pointing it to some local HTML file Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken. Thanks, David Narváez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 120854: KPassivePopup - Set default hide delay
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120854/ --- Review request for KDE Frameworks. Bugs: 340238 https://bugs.kde.org/show_bug.cgi?id=340238 Repository: knotifications Description --- If delay -1 is passed, it means server default, but in KPassivePopup the default was never set. Fixes bug 340238. Diffs - src/kpassivepopup.cpp d253898 Diff: https://git.reviewboard.kde.org/r/120854/diff/ Testing --- Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120854: KPassivePopup - Set default hide delay
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120854/#review69303 --- Should maybe be delay 0? Either way, +1. - Aleix Pol Gonzalez On Oct. 28, 2014, 4:14 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120854/ --- (Updated Oct. 28, 2014, 4:14 p.m.) Review request for KDE Frameworks. Bugs: 340238 https://bugs.kde.org/show_bug.cgi?id=340238 Repository: knotifications Description --- If delay -1 is passed, it means server default, but in KPassivePopup the default was never set. Fixes bug 340238. Diffs - src/kpassivepopup.cpp d253898 Diff: https://git.reviewboard.kde.org/r/120854/diff/ Testing --- Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120794: Implement Unique Mode Properly in Rekonq
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/#review69317 --- src/application.cpp https://git.reviewboard.kde.org/r/120794/#comment48461 I guess cwd should be Q_UNUSED until it will be 'used' :) - Andrea Diamantini On Oct. 28, 2014, 2 p.m., David Narváez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/ --- (Updated Oct. 28, 2014, 2 p.m.) Review request for KDE Frameworks and rekonq. Repository: rekonq Description --- This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not. The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that. Diffs - src/application.h 7ccd60d src/application.cpp c7c297d src/main.cpp 7592f7a Diff: https://git.reviewboard.kde.org/r/120794/diff/ Testing --- 1. Open one Rekonq window 2. Try opening Rekonq again 3. Try opeining Rekonq from a command line with some URLs 4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in) 5. Open rekonq from the console using rekonq --incognito 6. Open rekonq from the console using reknoq --webapp twitter.com 7. Open rekonq from the console pointing it to some local HTML file Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken. Thanks, David Narváez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120794: Implement Unique Mode Properly in Rekonq
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/#review69318 --- src/application.cpp https://git.reviewboard.kde.org/r/120794/#comment48462 It's not clear to me why you chose parse() instead of process() here. Can you please argument this? - Andrea Diamantini On Oct. 28, 2014, 2 p.m., David Narváez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120794/ --- (Updated Oct. 28, 2014, 2 p.m.) Review request for KDE Frameworks and rekonq. Repository: rekonq Description --- This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not. The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that. Diffs - src/application.h 7ccd60d src/application.cpp c7c297d src/main.cpp 7592f7a Diff: https://git.reviewboard.kde.org/r/120794/diff/ Testing --- 1. Open one Rekonq window 2. Try opening Rekonq again 3. Try opeining Rekonq from a command line with some URLs 4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in) 5. Open rekonq from the console using rekonq --incognito 6. Open rekonq from the console using reknoq --webapp twitter.com 7. Open rekonq from the console pointing it to some local HTML file Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken. Thanks, David Narváez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120833: Handle absolute paths without extension in KPluginLoader::findPlugin()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120833/#review69357 --- This should be done in Qt, IMHO. The docu (which I added in 418890e07) says We recommend omitting the file's suffix in the file name without specifying that this would only work for relative paths. - David Faure On Oct. 27, 2014, 9:51 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120833/ --- (Updated Oct. 27, 2014, 9:51 p.m.) Review request for KDE Frameworks, David Faure and Sebastian Kügler. Repository: kcoreaddons Description --- This means that e.g. KPluginLoader::findPlugin(/usr/lib/plugins/foo) can now return /usr/lib/plugins/foo.so instead of an empty string. Should this be done in Qt instead? Diffs - autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 src/lib/plugin/kpluginloader.cpp 802ef843bca4526cc9a4ee6780e8125453786b12 Diff: https://git.reviewboard.kde.org/r/120833/diff/ Testing --- Unit test passes Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120833: Handle absolute paths without extension in KPluginLoader::findPlugin()
On Oct. 28, 2014, 9:59 p.m., David Faure wrote: This should be done in Qt, IMHO. The docu (which I added in 418890e07) says We recommend omitting the file's suffix in the file name without specifying that this would only work for relative paths. I can look into adding that somewhen, but if you feel like making a Qt submission, go ahead of course. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120833/#review69357 --- On Oct. 27, 2014, 9:51 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120833/ --- (Updated Oct. 27, 2014, 9:51 p.m.) Review request for KDE Frameworks, David Faure and Sebastian Kügler. Repository: kcoreaddons Description --- This means that e.g. KPluginLoader::findPlugin(/usr/lib/plugins/foo) can now return /usr/lib/plugins/foo.so instead of an empty string. Should this be done in Qt instead? Diffs - autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 src/lib/plugin/kpluginloader.cpp 802ef843bca4526cc9a4ee6780e8125453786b12 Diff: https://git.reviewboard.kde.org/r/120833/diff/ Testing --- Unit test passes Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120833: Handle absolute paths without extension in KPluginLoader::findPlugin()
On Okt. 28, 2014, 10:59 nachm., David Faure wrote: This should be done in Qt, IMHO. The docu (which I added in 418890e07) says We recommend omitting the file's suffix in the file name without specifying that this would only work for relative paths. David Faure wrote: I can look into adding that somewhen, but if you feel like making a Qt submission, go ahead of course. I can add it to Qt, just have to figure out how to use Gerrit - Alexander --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120833/#review69357 --- On Okt. 27, 2014, 10:51 nachm., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120833/ --- (Updated Okt. 27, 2014, 10:51 nachm.) Review request for KDE Frameworks, David Faure and Sebastian Kügler. Repository: kcoreaddons Description --- This means that e.g. KPluginLoader::findPlugin(/usr/lib/plugins/foo) can now return /usr/lib/plugins/foo.so instead of an empty string. Should this be done in Qt instead? Diffs - autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 src/lib/plugin/kpluginloader.cpp 802ef843bca4526cc9a4ee6780e8125453786b12 Diff: https://git.reviewboard.kde.org/r/120833/diff/ Testing --- Unit test passes Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120813: Set shortcuts via invoked meta method
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120813/ --- (Updated Oct. 28, 2014, 11:14 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, David Faure and Daniel Laidig. Repository: kwidgetsaddons Description --- While porting KCharSelect application to KF5, I noticed that KCharSelect widget class still uses QAction::setShortcuts() instead of KActionCollection::setDefaultShortcuts(). This adds code to invoke setDefaultShortcuts() via QMetaObject - same as for addAction() - so that KCharSelect widget class does not need to depend on kxmlgui. For the KActionCollection patch see https://git.reviewboard.kde.org/r/120812/ Diffs - src/kcharselect.cpp 0d345a4 Diff: https://git.reviewboard.kde.org/r/120813/diff/ Testing --- None. Thanks, Christoph Feck ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Replacement for KMimeType::isBinaryData?
Heya, I didn't find a suitable replacement for KMimeType::isBinaryData in KF5. Is there some? http://lxr.kde.org/ident?v=kf5-qt5_i=isBinaryData_remember=1 shows exactly two users of this function. Worth considering upstreaming to Qt? -- Kevin Funk | kf...@kde.org | http://kfunk.org ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 120878: Remove unused header
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120878/ --- Review request for KDE Frameworks, kdewin and Plasma. Repository: krunner Description --- Since scripting/runnerscript.cpp is not compiled, we get a linking error by exporting but not defining RunnerScript. Diffs - src/abstractrunner.cpp 4da0394 Diff: https://git.reviewboard.kde.org/r/120878/diff/ Testing --- Tested building using MSVC 64bit Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
