Re: KDiagram libs (KChart, KGantt) in KDE Review
On Monday 09 February 2015 01:50:03 Friedrich W. H. Kossebau wrote: > Yes, nearly copy&paste: 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 copy&paste: 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]
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 > >
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
