Re: Review Request 122475: Fix bug 343906 - Unable to handle plain directory paths as QUrl
On Feb. 9, 2015, 10:01 nachm., Kevin Kofler wrote: IMHO, QUrl::fromUserInput(str, QString() QUrl::AssumeLocalFile) would be safer. Or do you really think dolphin nonexistentfile should look up nonexistentfile over DNS? Thomas Lübking wrote: +1, notably since http://nonexistenfile won't be very helpful in dolphin, but will directly open a browser. One could end up on nasty pages. Arjun AK wrote: IMHO, QUrl::fromUserInput(str, QString() QUrl::AssumeLocalFile) would be safer. Or do you really think dolphin nonexistentfile should look up nonexistentfile over DNS? [Done](http://commits.kde.org/kde-baseapps/0f91025a752b37ea4b6f2e7c02507bda5863e71f) QUrl::AssumeLocalFile looks like a good idea! Unfortunately, it seems that it's only available in Qt 5.4 and later: http://mail.kde.org/pipermail/kde-frameworks-devel/2015-February/022157.html http://mail.kde.org/pipermail/kde-frameworks-devel/2015-February/022158.html So there should either be an ifdef-version check, or the Qt version requirement should be bumped to 5.4. I'm not sure if there are any distros who will still use Qt 5.3 in their next releases - if not, then bumping the required Qt version is probably easier and less ugly. - Frank --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122475/#review75736 --- On Feb. 9, 2015, 12:48 nachm., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122475/ --- (Updated Feb. 9, 2015, 12:48 nachm.) Review request for KDE Base Apps. Bugs: 343906 http://bugs.kde.org/show_bug.cgi?id=343906 Repository: kde-baseapps Description --- URLs passed as commandline arguments should be constructed using `QUrl::fromUserInput()` Diffs - dolphin/src/main.cpp 094402f Diff: https://git.reviewboard.kde.org/r/122475/diff/ Testing --- dolphin /tmp dolphin ftp.debian.org Thanks, Arjun AK
dolphin Qt requirement
Arjun, http://mail.kde.org/pipermail/kde-frameworks-devel/2015-February/022157.html -- it seems your recent commit changed Dolphin to require Qt 5.4 since AssumeLocalFile is new in Qt 5.4. If you want to depend on Qt 5.4 please bump the requirement in the kde-baseapps/CMakeLists.txt. If we/you want it to build with Qt 5.3 some other workaround is needed. thanks, Jeremy
Re: Review Request 122475: Fix bug 343906 - Unable to handle plain directory paths as QUrl
On Feb. 9, 2015, 10:01 nachm., Kevin Kofler wrote: IMHO, QUrl::fromUserInput(str, QString() QUrl::AssumeLocalFile) would be safer. Or do you really think dolphin nonexistentfile should look up nonexistentfile over DNS? Thomas Lübking wrote: +1, notably since http://nonexistenfile won't be very helpful in dolphin, but will directly open a browser. One could end up on nasty pages. Arjun AK wrote: IMHO, QUrl::fromUserInput(str, QString() QUrl::AssumeLocalFile) would be safer. Or do you really think dolphin nonexistentfile should look up nonexistentfile over DNS? [Done](http://commits.kde.org/kde-baseapps/0f91025a752b37ea4b6f2e7c02507bda5863e71f) Frank Reininghaus wrote: QUrl::AssumeLocalFile looks like a good idea! Unfortunately, it seems that it's only available in Qt 5.4 and later: http://mail.kde.org/pipermail/kde-frameworks-devel/2015-February/022157.html http://mail.kde.org/pipermail/kde-frameworks-devel/2015-February/022158.html So there should either be an ifdef-version check, or the Qt version requirement should be bumped to 5.4. I'm not sure if there are any distros who will still use Qt 5.3 in their next releases - if not, then bumping the required Qt version is probably easier and less ugly. Oops, I didn't realize that we're still supporting 5.3. :-( Fedora ships 5.4 as an official update to all supported releases, so we'd be fine with the requirement just getting bumped. Kompare (ported in October 2014) uses this: https://projects.kde.org/projects/kde/kdesdk/kompare/repository/revisions/master/entry/libdialogpages/diffpage.cpp#L45 - Kevin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122475/#review75736 --- On Feb. 9, 2015, 12:48 nachm., Arjun AK wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122475/ --- (Updated Feb. 9, 2015, 12:48 nachm.) Review request for KDE Base Apps. Bugs: 343906 http://bugs.kde.org/show_bug.cgi?id=343906 Repository: kde-baseapps Description --- URLs passed as commandline arguments should be constructed using `QUrl::fromUserInput()` Diffs - dolphin/src/main.cpp 094402f Diff: https://git.reviewboard.kde.org/r/122475/diff/ Testing --- dolphin /tmp dolphin ftp.debian.org Thanks, Arjun AK
Re: KDiagram libs (KChart, KGantt) in KDE Review
On Monday 09 February 2015 01:50:03 Friedrich W. H. Kossebau wrote: Yes, nearly copypaste: the copies of KDChart in Calligra KMyMoney are older (2.4.1, based on Qt4) versions, while the copy of KDChart in Massif-Visualizer matches the version of the KChart lib in KDiagram. I've tried compiling the code on FreeBSD 10.1-RELEASE with Clang 3.4.1 (I'm assuming that's a supported compiler -- on techbase, searching for supported compiler doesn't give me much compatibility information newer than KDE 4.2). - I need to add /usr/local/include to the include search path; this is not kdiagram specific, but seems to be a general Qt5 issue on FreeBSD. - TestDrawIntoPainter seems to hang; after 2 min at 100% CPU I killed it. I ran it separately by hand and get output about missing OpenGL drivers (which is true, I'm building kdiagram in a restricted environment; I didn't originally expect to need to have DBUS running to be able to do the tests either) and debug output like: QDEBUG : TestDrawIntoPainter::testTest() Time for drawing pixmap :/test: 53682 ms Is that test supposed to take so much longer (minutes) than the other tests (deciseconds)? So from a it-compiles-and-the-tests-pass point of view on my platform, it looks good. [ade]
Re: KDiagram libs (KChart, KGantt) in KDE Review
On Wednesday 11 February 2015 14:59:50 Adriaan de Groot wrote: On Monday 09 February 2015 01:50:03 Friedrich W. H. Kossebau wrote: Yes, nearly copypaste: the copies of KDChart in Calligra KMyMoney are older (2.4.1, based on Qt4) versions, while the copy of KDChart in Massif-Visualizer matches the version of the KChart lib in KDiagram. I've tried compiling the code on FreeBSD 10.1-RELEASE with Clang 3.4.1 (I'm assuming that's a supported compiler -- on techbase, searching for supported compiler doesn't give me much compatibility information newer than KDE 4.2). - I need to add /usr/local/include to the include search path; this is not kdiagram specific, but seems to be a general Qt5 issue on FreeBSD. - TestDrawIntoPainter seems to hang; after 2 min at 100% CPU I killed it. I ran it separately by hand and get output about missing OpenGL drivers (which is true, I'm building kdiagram in a restricted environment; I didn't originally expect to need to have DBUS running to be able to do the tests either) and debug output like: QDEBUG : TestDrawIntoPainter::testTest() Time for drawing pixmap :/test: 53682 ms Is that test supposed to take so much longer (minutes) than the other tests (deciseconds)? I guess so. The test is commented out in the .pro file in the KDAB version - the reason is probably that it takes so long. So from a it-compiles-and-the-tests-pass point of view on my platform, it looks good. [ade]
Review Request 122534: [konq-plugins] fsview: Add missing include(ECMMarkAsTest)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122534/ --- Review request for KDE Base Apps. Repository: kde-baseapps Description --- [konq-plugins] fsview: Add missing include(ECMMarkAsTest) Diffs - konq-plugins/fsview/tests/CMakeLists.txt 4ee03006999dfb62c25a36bb643f92c3078697b6 Diff: https://git.reviewboard.kde.org/r/122534/diff/ Testing --- Thanks, Andreas Sturmlechner
Re: Review Request 122534: [konq-plugins] fsview: Add missing include(ECMMarkAsTest)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122534/#review75894 --- I'd move that line to the top CMakeLists.txt. And remove the duplicate ones in other CMakeLists.txt files. - Kevin Funk On Feb. 11, 2015, 9:43 p.m., Andreas Sturmlechner wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122534/ --- (Updated Feb. 11, 2015, 9:43 p.m.) Review request for KDE Base Apps. Repository: kde-baseapps Description --- [konq-plugins] fsview: Add missing include(ECMMarkAsTest) Diffs - konq-plugins/fsview/tests/CMakeLists.txt 4ee03006999dfb62c25a36bb643f92c3078697b6 Diff: https://git.reviewboard.kde.org/r/122534/diff/ Testing --- Thanks, Andreas Sturmlechner
Re: Review Request 122534: [konq-plugins] fsview: Add missing include(ECMMarkAsTest)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122534/ --- (Updated Feb. 11, 2015, 11:52 p.m.) Review request for KDE Base Apps. Changes --- Moved the include up into konq-plugins/CMakeLists.txt Repository: kde-baseapps Description (updated) --- [konq-plugins] fsview: Add missing include(ECMMarkAsTest) Diffs (updated) - konq-plugins/CMakeLists.txt 1b70313929a76175167e7fbc6680ee4ff8fc7026 Diff: https://git.reviewboard.kde.org/r/122534/diff/ Testing --- Thanks, Andreas Sturmlechner
