Re: Review Request 112560: Remove KNotification dependency in KCompletion

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112560/#review39651
---

Ship it!


Ship It!

- Sune Vuorela


On Sept. 6, 2013, 3:04 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112560/
 ---
 
 (Updated Sept. 6, 2013, 3:04 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 Drop KNotifications dependency from KCompletion.
 
 Reasons:
 - Its not used in KDE4
 - It creates noise
 - It bumps KCompletion from tier2 to tier3
 
 I know I already sent an e-mail about it, but it seemed to me that a review 
 request would be more welcome.
 
 
 Diffs
 -
 
   staging/kcompletion/src/CMakeLists.txt 1300ab5 
   staging/kcompletion/src/kcompletion.cpp bcd220a 
   staging/kcompletion/src/khistorycombobox.cpp fe955a2 
 
 Diff: http://git.reviewboard.kde.org/r/112560/diff/
 
 
 Testing
 ---
 
 builds, tests pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Splitting out a KX11Extras frameworks from KWindowSystems

[Sune Vuorela]

On 2013-09-16, Martin Graesslin mgraess...@kde.org wrote:
 The KWindowSystems framework would need to become a tier2 framework and as 
 it needs to depend on NETwm (X11 implementation uses it and the defines are 
 used). It would contain:
 * KKeyServer
 * KWindowEffects (this could also stay in X11Extras)
 * KWindowInfo
 * KWindowSystem

Just to be sure, none of these have public X11 dependencies ?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112816: Do not use internal headers in kcolorutilsdemo

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112816/#review40692
---



tier1/kguiaddons/src/lib/colors/kcolorutils.cpp
http://git.reviewboard.kde.org/r/112816/#comment29951

My initial reaction is that it could return a bool wether or not things 
went okay, given there is a 'path that does nothing' in the code. Besides that, 
everything looks great.


- Sune Vuorela


On Sept. 24, 2013, 2:19 p.m., Aurélien Gâteau wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112816/
 ---
 
 (Updated Sept. 24, 2013, 2:19 p.m.)
 
 
 Review request for KDE Frameworks and kdelibs.
 
 
 Description
 ---
 
 This is a two-commit patch:
 
 1. Add a KColorUtils::getHcy() method
 
 2. In kcolorutils demo: use KColorUtils::getHcy() instead of the internal 
 KColorSpaces::KHCY
 
 This is a required first step to make kconfigwidgets build standalone
 
 
 Diffs
 -
 
   staging/kconfigwidgets/tests/kcolorutilsdemo.cpp 940569e 
   tier1/kguiaddons/src/lib/colors/kcolorutils.h c20c6f7 
   tier1/kguiaddons/src/lib/colors/kcolorutils.cpp 9212bba 
 
 Diff: http://git.reviewboard.kde.org/r/112816/diff/
 
 
 Testing
 ---
 
 kcolorutilsdemo still works.
 
 
 Thanks,
 
 Aurélien Gâteau
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112816: Do not use internal headers in kcolorutilsdemo

[Sune Vuorela]

 On Sept. 24, 2013, 2:23 p.m., Sune Vuorela wrote:
  tier1/kguiaddons/src/lib/colors/kcolorutils.cpp, line 42
  http://git.reviewboard.kde.org/r/112816/diff/1/?file=190519#file190519line42
 
  My initial reaction is that it could return a bool wether or not things 
  went okay, given there is a 'path that does nothing' in the code. Besides 
  that, everything looks great.
 
 Aurélien Gâteau wrote:
 I wrote it this way to be consistent with QColor::get*() methods. I think 
 users of such code are going to be used to QColor API, so it makes more sense 
 this way, what do you think?

I'm convinced.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112816/#review40692
---


On Sept. 24, 2013, 2:19 p.m., Aurélien Gâteau wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112816/
 ---
 
 (Updated Sept. 24, 2013, 2:19 p.m.)
 
 
 Review request for KDE Frameworks and kdelibs.
 
 
 Description
 ---
 
 This is a two-commit patch:
 
 1. Add a KColorUtils::getHcy() method
 
 2. In kcolorutils demo: use KColorUtils::getHcy() instead of the internal 
 KColorSpaces::KHCY
 
 This is a required first step to make kconfigwidgets build standalone
 
 
 Diffs
 -
 
   staging/kconfigwidgets/tests/kcolorutilsdemo.cpp 940569e 
   tier1/kguiaddons/src/lib/colors/kcolorutils.h c20c6f7 
   tier1/kguiaddons/src/lib/colors/kcolorutils.cpp 9212bba 
 
 Diff: http://git.reviewboard.kde.org/r/112816/diff/
 
 
 Testing
 ---
 
 kcolorutilsdemo still works.
 
 
 Thanks,
 
 Aurélien Gâteau
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Making KDocTools independent of KArchive

[Sune Vuorela]

 Maybe we can bundle the generated documentation?

Distributions in general don't want pregenerated anything.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113205: Make KJob::result public for the new signal/slot syntax.

[Sune Vuorela]

On Oct. 11, 2013, 9:51 p.m., Mark Gaiser wrote:
  We are here making a 'hole' for people to do  'bad things' that wasn't 
  possible in the past. I'm not sure we want that.
 
 Mark Gaiser wrote:
 Interesting.
 So that mean we simply can't use the new signal/slot syntax because of 
 it? That would seem rather strange to me..
 
 If you do a stat call, or listEntry or any of the others...
 Then you are supposed to connect to the result slot. For listEntry you 
 are supposed to connect to the finished signal. Both of those are defined as:
 signals:
 private:
 
 AKA. Private signals.
 I really don't see how you can work around this besides perhaps 
 QSignalMapper, but that would be very odd as well. I'm really curious to see 
 how that bit of magic is supposed to work. Do you have some links for me 
 there?

I'm not saying we can't use the new syntax because of it. I'm saying it needs a 
bit more work, and before a 'stable' version is needed.

There is a solution out there. It's applied to QAIM and others.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113205/#review41570
---


On Oct. 11, 2013, 5:59 p.m., Mark Gaiser wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113205/
 ---
 
 (Updated Oct. 11, 2013, 5:59 p.m.)
 
 
 Review request for KDE Frameworks and kdelibs.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 The new signal/slot connection:
 connect(job, KJob::result,...
 
 does't like result to be private and throws an compile error:
 error: 'void KJob::result(KJob*)' is private
 
 Making it public resolves the issue and makes this slot usable in the new 
 syntax. In my case i wanted to use the new syntax and directly use a lambda 
 as slot. Which isn't possible on this signal if it isn't public.
 
 
 Diffs
 -
 
   tier1/kcoreaddons/src/lib/jobs/kjob.h d663530 
 
 Diff: http://git.reviewboard.kde.org/r/113205/diff/
 
 
 Testing
 ---
 
 Works just fine.
 
 
 Thanks,
 
 Mark Gaiser
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Flaky kcodecs test depending on qt configuration

[Sune Vuorela]

Hi

Depending on the Qt configuration (built with or without ICU), the
KCharsetsTest::testEncodingNames() test fails (in the #if) block.

If Qt built with ICU, the tests succeeds and the ISO 8859-16, jis7 and
winsami2 codecs are as expected not found. 

If Qt is built without ICU, those codecs are found, and the test that
expects them to not be there fails.

There doesn't seem to be api to check wether or not ICU is available for
QTextCodec.

One solution could be to just skip finding those 3 codecs in all cases.

Other suggestions?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

KDirWatch vs QFileSystemWatcher

[Sune Vuorela]

Hi

If your KDirWatch backend is QFileSystemWatcher, one of the testcases
fails.

The last QVERIFY in testMoveTo never receives the signal dirty-signal it
is looking for.

Apparantly, the watch.removeFile apparantly somehow turns off the QFSW
to not do any further notifications for what happens in the directory
that it also is supposed to be watching.

I haven't yet fully understood what's going on here, but if someone is
up for it, fixing it would be nice. Among others, it is the only backend
available on windows.

This dirty patch seems to ensure that QFSW is used on your platform:

diff --git a/tier1/kcoreaddons/src/lib/io/kdirwatch.cpp 
b/tier1/kcoreaddons/src/lib/io/kdirwatch.cpp
index 2a89372..8bc9d9b 100644
--- a/tier1/kcoreaddons/src/lib/io/kdirwatch.cpp
+++ b/tier1/kcoreaddons/src/lib/io/kdirwatch.cpp
@@ -103,12 +103,7 @@ static KDirWatch::Method methodFromString(const QString 
method) {
   } else if (method == QLatin1String(QFSWatch)) {
 return KDirWatch::QFSWatch;
   } else {
-#ifdef Q_OS_LINUX
-// inotify supports delete+recreate+modify, which QFSWatch doesn't support
-return KDirWatch::INotify;
-#else
 return KDirWatch::QFSWatch;
-#endif
   }
 }
 

Anyone looking into it would be nice.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KDirWatch vs QFileSystemWatcher

[Sune Vuorela]

On 2013-10-27, David Faure fa...@kde.org wrote:
 testMoveTo passes most of the time (finally got a failure after 10 runs or
 so)...

 Fixed as well (9c6cc615676a5bb) (not a real bug, a unittest bug).


4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Added Dir
C:/Users/Administrator/A
ppData/Local/Temp/2/kdirwatch_unittest-1Xeaaa for  [KDirWatch-11]
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() fsWatcher-addPath
C:/Users/Admini
strator/AppData/Local/Temp/2/kdirwatch_unittest-1Xeaaa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Added File
C:/Users/Administrator/
AppData/Local/Temp/2/kdirwatch_unittest-1Xeaaa/moveTo for 
[KDirWatch-11]
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() fsWatcher-addPath
C:/Users/Admini
strator/AppData/Local/Temp/2/kdirwatch_unittest-1Xeaaa/moveTo
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Waited 0 ms so that now
2013-10-27
T22:58:58 is  2013-10-27T22:58:57
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Overwrite file1 with
tempfile
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Waited 700 ms so that now
2013-10-
27T22:58:59 is  2013-10-27T22:58:58
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Waited 0 ms so that now
2013-10-27
T22:58:59 is  2013-10-27T22:58:58
4: QWARN  : KDirWatch_UnitTest::testMoveTo() Timeout waiting for
KDirWatch signa
l dirty(QString) (
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unit
test-1Xeaaa/ )
4: FAIL!  : KDirWatch_UnitTest::testMoveTo() 'waitForOneSignal(watch,
SIGNAL(dir
ty(QString)), m_path)' returned FALSE. ()


Seems to still consistently failing here on windows :/

Yes. lines are truncated and messed up. thanks cmd.exe.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KDirWatch vs QFileSystemWatcher

[Sune Vuorela]

On 2013-10-28, David Faure fa...@kde.org wrote:
 On Sunday 27 October 2013 22:02:18 Sune Vuorela wrote:
 Seems to still consistently failing here on windows :/

 Good, consistency is good, we can debug that :)

 Can you apply this patch and try again, so I get more debug output?

 http://www.davidfaure.fr/2013/kcoreaddons_debug.diff

It definitely made it more verbose, but most of your debug changes
wasn't even reached.

4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Added Dir
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
for  [KDirWatch-11]
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() fsWatcher-addPath
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Added File
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa/moveTo
for  [KDirWatch-11]
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() fsWatcher-addPath
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa/moveTo
4: QDEBUG : KDirWatch_UnitTest::testMoveTo()
C:/Users/Administrator/AppData/Local/Temp/2/newdir-kObaaa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Waited 300 ms so that now
2013-10-28T09:29:38 is  2013-10-28T09:29:37
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Overwrite file1 with
tempfile
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() waiting for dirty signal
4: QDEBUG : KDirWatch_UnitTest::testMoveTo()
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() e-m_ctime= 1382948973
09:29:33 stat_buf.st_ctime= 1382948954 e-m_nlink= 1 stat_buf.st_nlink=
1 e-m_ino= 0 stat_buf.st_ino= 0
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() scanEntry for
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
says 1
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() 1
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
1 clients

4: QDEBUG : KDirWatch_UnitTest::testMoveTo()
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() e-m_ctime= 1382948978
09:29:38 stat_buf.st_ctime= 1382948954 e-m_nlink= 1 stat_buf.st_nlink=
1 e-m_ino= 0 stat_buf.st_ino= 0
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() scanEntry for
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
says 1
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() 1
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
1 clients

4: QDEBUG : KDirWatch_UnitTest::testMoveTo()
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() e-m_ctime= 1382948978
09:29:38 stat_buf.st_ctime= 1382948954 e-m_nlink= 1 stat_buf.st_nlink=
1 e-m_ino= 0 stat_buf.st_ino= 0
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() scanEntry for
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
says 1
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() 1
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
1 clients

4: QDEBUG : KDirWatch_UnitTest::testMoveTo()
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa/moveTo
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() e-m_ctime= 1382948977
09:29:37 stat_buf.st_ctime= 1382948977 e-m_nlink= 1 stat_buf.st_nlink=
1 e-m_ino= 0 stat_buf.st_ino= 0
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() scanEntry for
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa/moveTo
says 1
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() 1
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa/moveTo
1 clients
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() waiting for dirty signal
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() KDirWatch-11 emitting
dirty
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() KDirWatch-11 emitting
dirty
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() KDirWatch-11 emitting
dirty
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() KDirWatch-11 emitting
dirty
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa/moveTo
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() got 4 dirty signals
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() got
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() Waited 0 ms so that now
2013-10-28T09:29:39 is  2013-10-28T09:29:38
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() waiting for dirty signal
4: QDEBUG : KDirWatch_UnitTest::testMoveTo()
C:/Users/Administrator/AppData/Local/Temp/2/kdirwatch_unittest-Pa/moveTo
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() e-m_ctime= 1382948978
09:29:38 stat_buf.st_ctime= 1382948977 e-m_nlink= 1 stat_buf.st_nlink=
1 e-m_ino= 0 stat_buf.st_ino= 0
4: QDEBUG : KDirWatch_UnitTest::testMoveTo() scanEntry for
C:/Users/Administrator/AppData/Local/Temp/2

Re: Google Code-In

[Sune Vuorela]

On 2013-10-25, Kevin Ottens er...@kde.org wrote:
 Excellent! Could you get in touch with Lydia about it? I'd rather have =
 a=20
 support role, but we need someone to keep track of things and you seem =
 like a=20
 good candidate. :-)
=20

I've scrolled over kdeexamples and added 4 tasks. 
Ensure kdeexamples/kidletime worksr with KDE Framework
ensure that kdeexamples/kdeui/kmessagewidgetdemo compiles 
ensure that the kdeexamples/kdeui/kdeuiwidgets builds and runs 
and write a simple tool 'kunarchive' using karchive for kdeexamples

kdeexamples wasn't like the biggest thing out there.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Use of the WIN32 executable property

[Sune Vuorela]

On 2013-10-27, David Faure fa...@kde.org wrote:
 I withdraw my objection then, I guess. Let them have a console, for easier 
 debugging.

\o/

Let's get all windows test apps to get a console.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113479: fix kshareddatacache on windows

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113479/
---

(Updated Oct. 28, 2013, 10:07 a.m.)


Review request for KDE Frameworks, kdelibs and Michael Pyne.


Repository: kdelibs


Description
---

fix kshareddatacache on windows to at least not be a way to have a bytearray 
roundtrip.

Also, the windows implementation is currently only a in-memory one, so don't 
test on windows if there is a file written.


Diffs
-

  tier1/kcoreaddons/autotests/kshareddatacachetest.cpp 
a4484560735f9096ecdac26b3c539394602e0f31 
  tier1/kcoreaddons/src/lib/caching/kshareddatacache_win.cpp 
cdc6536b56888a615e74960bf1b55fb12cc3e70d 

Diff: http://git.reviewboard.kde.org/r/113479/diff/


Testing
---

Test suite passes


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113479: fix kshareddatacache on windows

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113479/
---

(Updated Oct. 28, 2013, 8:08 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, kdelibs and Michael Pyne.


Repository: kdelibs


Description
---

fix kshareddatacache on windows to at least not be a way to have a bytearray 
roundtrip.

Also, the windows implementation is currently only a in-memory one, so don't 
test on windows if there is a file written.


Diffs
-

  tier1/kcoreaddons/autotests/kshareddatacachetest.cpp 
a4484560735f9096ecdac26b3c539394602e0f31 
  tier1/kcoreaddons/src/lib/caching/kshareddatacache_win.cpp 
cdc6536b56888a615e74960bf1b55fb12cc3e70d 

Diff: http://git.reviewboard.kde.org/r/113479/diff/


Testing
---

Test suite passes


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113483: Copy KDE4 macro to install all icons in the current source dir

[Sune Vuorela]

On 2013-10-29, Alexander Neundorf neund...@kde.org wrote:
 Good point.
 As it is, IMO for being ECM, it needs way more documentation.
 It needs to be documented so that it can be used by people who know nothing 
 about KDE or KDE's icon scheme.

 Or should that be in the framework which deals with icon loading ?

It is not about icon loading, so no.

Installing icons in the right places is a good generic task that
everybody[tm] needs.

There is also nothing about KDE's icon scheme in it, except maybe the
two words crystal and oxygen.

The rest is (supposedly) adhering to the icon naming spec.  including
the word hicolor.

In general, apps should install icons into a app specific directory in
hicolor and then let the more specialized icon themes override them as
needed.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 113503: make dbus dependency optional in JobWidgets

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113503/
---

Review request for KDE Frameworks, kdelibs and Stephen Kelly.


Repository: kdelibs


Description
---

Make dbus dependency optional in JobWidgets

Many don't have dbus available on all platforms, especially windows, but 
JobWidgets is very much useful without it.


Diffs
-

  tier2/kjobwidgets/CMakeLists.txt ca53024 
  tier2/kjobwidgets/src/CMakeLists.txt 0a575a6 
  tier2/kjobwidgets/src/config-kjobwidgets.h.cmake 35b64a2 
  tier2/kjobwidgets/tests/kjobtrackerstest.cpp 7a61407 

Diff: http://git.reviewboard.kde.org/r/113503/diff/


Testing
---

build tested on windows without dbus. not yet tested on other platforms


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113503: make dbus dependency optional in JobWidgets

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113503/#review42696
---


for some reason the cmake magic that is supposed to ensure the files are 
properly added, always returns false, so doesn't build with dbus. needs fixing

- Sune Vuorela


On Oct. 29, 2013, 10:27 p.m., Sune Vuorela wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113503/
 ---
 
 (Updated Oct. 29, 2013, 10:27 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs and Stephen Kelly.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 Make dbus dependency optional in JobWidgets
 
 Many don't have dbus available on all platforms, especially windows, but 
 JobWidgets is very much useful without it.
 
 
 Diffs
 -
 
   tier2/kjobwidgets/CMakeLists.txt ca53024 
   tier2/kjobwidgets/src/CMakeLists.txt 0a575a6 
   tier2/kjobwidgets/src/config-kjobwidgets.h.cmake 35b64a2 
   tier2/kjobwidgets/tests/kjobtrackerstest.cpp 7a61407 
 
 Diff: http://git.reviewboard.kde.org/r/113503/diff/
 
 
 Testing
 ---
 
 build tested on windows without dbus. not yet tested on other platforms
 
 
 Thanks,
 
 Sune Vuorela
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113503: make dbus dependency optional in JobWidgets

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113503/
---

(Updated Oct. 30, 2013, 10:08 a.m.)


Review request for KDE Frameworks, kdelibs and Stephen Kelly.


Changes
---

workin on linux with dbus


Repository: kdelibs


Description
---

Make dbus dependency optional in JobWidgets

Many don't have dbus available on all platforms, especially windows, but 
JobWidgets is very much useful without it.


Diffs (updated)
-

  tier2/kjobwidgets/CMakeLists.txt ca53024 
  tier2/kjobwidgets/src/CMakeLists.txt 0a575a6 
  tier2/kjobwidgets/src/config-kjobwidgets.h.cmake 35b64a2 
  tier2/kjobwidgets/tests/kjobtrackerstest.cpp 7a61407 

Diff: http://git.reviewboard.kde.org/r/113503/diff/


Testing
---

build tested on windows without dbus. not yet tested on other platforms


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Getting ecm files from the ECM package

[Sune Vuorela]

On 2013-11-01, Kevin Ottens er...@kde.org wrote:
 So far we chose the have it in cmake/ecm route. If we had what Mirko =
 refers=20
 to, then that'd open the door to another solution.

And it would open the first door towards alienating linux distributions.

Of course, we could say and so what?. But that is our current primary
way of getting our stuff to our users - so we shouldn't put obstacles in
their way.

Maven, ruby and stuff is all communities where there seem to be a strong
tension with the linux distributions over issues like this. Let's not
try to embrace that.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Getting ecm files from the ECM package

[Sune Vuorela]

On 2013-11-01, Mirko Boehm mi...@kde.org wrote:
 Anyone up for hacking this up next week? I am available starting Monday 
 afternoon. 

Before you start hacking, please consider the following:

http vs https?  should http even be allowed?

certificate handling? 

how to do ssl? a library? will cmake accept a dependency on a ssl
library? which ones has a license that cmake will accept?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Getting ecm files from the ECM package

[Sune Vuorela]

On 2013-11-01, Alexander Neundorf neund...@kde.org wrote:
 Anyway, attached is a quick experiment, which adds the 3 KDE*.cmake files=
  from=20
 extra-cmake-modules/ to kf5umbrella/, by that turning it into tier0/, wit=
 h the=20
 optional ability (-DWITH_ECM) to download ECM and install ECM when buildi=
 ng=20
 and installing kf5umbrella itself.

gnuinstalldirs is in cmake. I'm sure that kdeinstalldirs can be in ecm
without anyone think it looks weird. I'm not sure what that extra module
buy us.


 find_package(KF5Umbrella)
 unconditionally loads KDECMakeSettings.cmake.

no extra unconditional magic please. Our files should be understandable.
I'm already lost 90% of the times I try to figure out how the cmake
stuff is fit together.

 In case we decide to go this way (i.e. the my ideal view plus optional=20
 downloading), and we should hear Stephens opinion on that, I volunteer to=
=20
 maintain extra-cmake-modules, iff the three KDE*.cmake files are moved ou=
 t of=20
 ECM, and I'll move it to github, to make contributing by others easier.

I think you mean to make contributing harder.

Free software needs free tools!

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Getting ecm files from the ECM package

[Sune Vuorela]

On 2013-11-01, Mirko Boehm mi...@kde.org wrote:
 To me, build systems should not download anything sounds like a movie
 from the 80ies.

To me, build systems fetches code and executes it sounds like a bad
horror movie.

 For example, from a developers point of view, what is the difference between 
 me 
 pulling the ECM repo and CMake doing it automatically? A brain? 

That you know it is going on.
That you know your build system is also going to work tomorrow in the
train.

And note, it is not only pulling. It is also cloning. Fetching stuff
from random places you haven't yet accepted is not good for anyones
internet security. We should not push such things forward.

 In my opinion, a central repository of community maintained find module
 packages has a chance of making a real difference.

Sure. And we have a central repository of community maintained find
modules. That's not part of the discussion.

If you want a tool that does all the magic for you, we have
kdesrc-build. And build-tool. 

oh. and btw, you want to replace e-c-m with a 'fetch-ecm'-repository
that people needs to fetch first? why not just ask people to fetch ecm
in the first place?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Getting ecm files from the ECM package

[Sune Vuorela]

On 2013-11-03, Alexander Neundorf neund...@kde.org wrote:
 This code unconditionally searches for QtCore (and sets a target property 
 where I'm not sure how many people here can understand what's going on).

It is hopefully a temporary hack that shouldn't be in that file.

Sometimes, temporary hacks in development stuff is unfortunately needed,
but I do hope that whoever introduced it has a plan to get it out.

I agree that finding ECM should just be finding ECM and not requiring a
Qt install.

It is though okay to have something bits in ECM that has a hard
dependency on Qt - maybe a ecm_do_extreme_magic_with_qt_resource_files
could be a theoretical thing that could be in ECM and require Qt to be
found to be useful for anything.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Exceptions in KDE Frameworks

[Sune Vuorela]

On 2013-11-02, Mirko Boehm mi...@kde.org wrote:
 I get from that that I can enable exceptions for threadweaver without 
 affecting the other libraries. This makes my job a lot easier.

note though that most of Qt isn't really compatiple with exceptions. see
also long threads on qt-dev list.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Having a Tier 0 for cmake and git submodules

[Sune Vuorela]

On 2013-11-10, Kevin Ottens er...@kde.org wrote:
 Since there's been several times discussions about having a kind of Ti=
 er 0=20
 for building our frameworks containing what is right now in ECM kde-mod=
 ules=20
 directory, but the idea was never really on the table because of the ex=
 tra=20
 dependency it'd bring, I looked a bit at the git submodule option to ge=
 t=20
 there.

Why move it out of e-c-m ?

 Also git archive ignores submodules when generating the archive... But =
 that's=20
 easy to fix and there's a git archive-all script available which does t=
 he=20
 recursive export.

And if a distribution need to patch whatever is in the 'submodule', they
have a enormous useless piece of work ahead for them.

No. Let us not do that.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Having a Tier 0 for cmake and git submodules

[Sune Vuorela]

On 2013-11-10, Kevin Ottens er...@kde.org wrote:

 --===3596639883239406900==
 Content-Type: multipart/signed; boundary=nextPart1424144.VkBYIHTjbs; 
 micalg=pgp-sha1; protocol=application/pgp-signature


 --nextPart1424144.VkBYIHTjbs
 Content-Transfer-Encoding: quoted-printable
 Content-Type: text/plain; charset=iso-8859-1

 On Sunday 10 November 2013 17:12:02 Sune Vuorela wrote:
 Why move it out of e-c-m ?

 To make e-c-m a neutral ground again, not something purely for KDE need=
 s. I=20
 can understand that positioning.

Let's just rename most of them to make them not look like kde specifics.
Except kdeinstalldirs. But well. cmake has gnuinstalldirs and similar,
so that could kind of be  okay to have.

Then there is the extremely dangerous KDECompilerSettings that should be
renamed to LOLPleaseAddSurprisesIntoTheCMakeSetup. srsly. I added a
KDE Framework to my application and suddenly -DCMAKE_BUILD_TYPE=Debug is
building with -O2. The other bits of the file seems to be filled with
Is this still needed? Does this work?
Should we consider sending it to the eternal bitfields?

that leaves KDECMakeSettings which kind of could be renamed to
'PleaseUseSane2013Defaults' and thus no longer be KDE specific.
We could then extend it in a couple of years if we need new changed sane
defaults for 2015.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 113805: Do not change the build types available with cmake

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113805/
---

Review request for Build System and KDE Frameworks.


Repository: extra-cmake-modules


Description
---

Do not change the build types available with cmake.


Diffs
-

  kde-modules/KDECompilerSettings.cmake 
b034751a5be8073f9628971b552faa079c64e8b6 

Diff: http://git.reviewboard.kde.org/r/113805/diff/


Testing
---

Built kdelibs on linux with gcc.


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113805: Do not change the build types available with cmake

[Sune Vuorela]

 On Nov. 12, 2013, 7:24 p.m., Alexander Neundorf wrote:
  IMO the patch as it is is not good.
  
  Several things:
  1) This file, is not mandatory at all with KDE frameworks.
  You can build applications using KDE frameworks libraries without it. You 
  (the developer of the application) simply has to not-load the file 
  KDECompilerSettings.
  If the developer has decided to load this file, it is not surprising that 
  he gets modified compiler flags, because this is what he decided to do.
  
  2) You removed e.g. the flags for the build types COVERAGE and PROFILE. 
  CMake doesn't provide these build types or flags itself, so the patch 
  removes support for these buildtypes. I don't see the need to remove 
  support for profile or coverage builds. CMake itself comes with debug, 
  minsizerel, release and relwithdebinfo.
  
  3) You remove the flags for Linux and/or gcc. Why didn't you remove them 
  for other compilers/operating systems ?
  
  
  I know that what we are doing in this file is not the cmake-recommended way.
  From cmake, the variables for the flags are cache variables which are set 
  to some default. The idea is that the person who compiles some package can 
  adjust them to his liking. This is from my experience not what we expect 
  from the average KDE contributor. He should get a working set up, with 
  known compiler flags so it is easy to fix buid bugs (or avoid them in the 
  first place).
  By simply setting the normal (non-cache) variables, the person building the 
  package can set the cache variables to whatever he wants, it will be 
  overridden to what is set in KDECompilerSettings.cmake.
  Maybe the way this is done could be improved by doing something like
  if(NOT DEFINED SOME_CXX_FLAGS_ALREADY_PRESET)
 set(SOME_CXX_FLAGS_ALREADY_PRESET TRUE CACHE BOOL docs... )
 set(SOME_CXX_FLAGS --some-flag --another-flag CACHE STRING docs... 
  FORCE)
  endif()
  
  This way it would be only on the initial cmake run forced into the cache, 
  and afterwards the user should be able to change them as he wants.
  
  
 

1) THis file is used by all kde frameworks, so it should not put in surprises 
for the developers there. ONe shouldn't be 100% familiar with all the internals 
to hack on stuff.

2) IF we want to add such things, it should be in a ECMAddProfileBuildType or 
similar.

3) For the other compilers, the build types aren't touched.

ANd note that this doesn't modify the flags that covers everything. That I'm 
saving for another patch.

And let us do thhings the cmake way, one step at a time.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113805/#review43538
---


On Nov. 11, 2013, 10:16 p.m., Sune Vuorela wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113805/
 ---
 
 (Updated Nov. 11, 2013, 10:16 p.m.)
 
 
 Review request for Build System and KDE Frameworks.
 
 
 Repository: extra-cmake-modules
 
 
 Description
 ---
 
 Do not change the build types available with cmake.
 
 
 Diffs
 -
 
   kde-modules/KDECompilerSettings.cmake 
 b034751a5be8073f9628971b552faa079c64e8b6 
 
 Diff: http://git.reviewboard.kde.org/r/113805/diff/
 
 
 Testing
 ---
 
 Built kdelibs on linux with gcc.
 
 
 Thanks,
 
 Sune Vuorela
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Getting ecm files from the ECM package

[Sune Vuorela]

On 2013-11-11, Aaron J. Seigo ase...@kde.org wrote:
 would that work for everyone?

I don't think it solves the actual hard point:

Where should the final home for the stuff in ecm/kde-modules be ?



I'll like to reiterate what I suggested should happen with it:

KDEInstallDirs.cmake :
Keep it as is, just like cmake has various FooInstallDirs, ecm can have
a it as well


KDECMakeSettings.cmake :
Be renamed to Good2013CmakeWithQtDefaults and kind of lock it to its
current content. Patches can be formed in form of
Good2014CmakeWithQtDefautls

It mostly contains stuff that I, if I_wasn't too lazy, would boilerplate
copy into all cmake+qt projects at work, for example.


KDECompilerSettings.cmake : 
Send 2/3 of it to the bitbucket. Rename the rest to
StrictQtCompilerSettings2013

Maybe split the cmake defaults and compilersettings in two one Qt
specific that includes the non-qt-specific one.

This could also be something to boilerplate include into many projects.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113805: Do not change the build types available with cmake

[Sune Vuorela]

 On Nov. 12, 2013, 7:24 p.m., Alexander Neundorf wrote:
  IMO the patch as it is is not good.
  
  Several things:
  1) This file, is not mandatory at all with KDE frameworks.
  You can build applications using KDE frameworks libraries without it. You 
  (the developer of the application) simply has to not-load the file 
  KDECompilerSettings.
  If the developer has decided to load this file, it is not surprising that 
  he gets modified compiler flags, because this is what he decided to do.
  
  2) You removed e.g. the flags for the build types COVERAGE and PROFILE. 
  CMake doesn't provide these build types or flags itself, so the patch 
  removes support for these buildtypes. I don't see the need to remove 
  support for profile or coverage builds. CMake itself comes with debug, 
  minsizerel, release and relwithdebinfo.
  
  3) You remove the flags for Linux and/or gcc. Why didn't you remove them 
  for other compilers/operating systems ?
  
  
  I know that what we are doing in this file is not the cmake-recommended way.
  From cmake, the variables for the flags are cache variables which are set 
  to some default. The idea is that the person who compiles some package can 
  adjust them to his liking. This is from my experience not what we expect 
  from the average KDE contributor. He should get a working set up, with 
  known compiler flags so it is easy to fix buid bugs (or avoid them in the 
  first place).
  By simply setting the normal (non-cache) variables, the person building the 
  package can set the cache variables to whatever he wants, it will be 
  overridden to what is set in KDECompilerSettings.cmake.
  Maybe the way this is done could be improved by doing something like
  if(NOT DEFINED SOME_CXX_FLAGS_ALREADY_PRESET)
 set(SOME_CXX_FLAGS_ALREADY_PRESET TRUE CACHE BOOL docs... )
 set(SOME_CXX_FLAGS --some-flag --another-flag CACHE STRING docs... 
  FORCE)
  endif()
  
  This way it would be only on the initial cmake run forced into the cache, 
  and afterwards the user should be able to change them as he wants.
  
  
 
 
 Sune Vuorela wrote:
 1) THis file is used by all kde frameworks, so it should not put in 
 surprises for the developers there. ONe shouldn't be 100% familiar with all 
 the internals to hack on stuff.
 
 2) IF we want to add such things, it should be in a 
 ECMAddProfileBuildType or similar.
 
 3) For the other compilers, the build types aren't touched.
 
 ANd note that this doesn't modify the flags that covers everything. That 
 I'm saving for another patch.
 
 And let us do thhings the cmake way, one step at a time.
 
 Alexander Neundorf wrote:
 1) I don't really understand. You say no surprises and at the same time 
 you say that developers shouldn't have to be familiar with all internals to 
 hack on stuff. If you want no surprises, remove the line 
 include(KDECompilerSettings) from the project. That's why it has been 
 separated out, to make it optional for users who don't want it. Then you get 
 plain cmake, and can/have to take care of the compiler settings yourself. Add 
 that line, and you get no need to care about internals.
 
 Is your plan also to remove the added defintions, linker flags, etc. ?
 I'm fine if you post a patch which removes the file completely. I just 
 doubt that the KDE community will be happy with this.
 
 This is the state as it was May 2006:
 
 http://quickgit.kde.org/?p=kdelibs.gita=blobhb=5713bc5ddd7f11ef73e63cf67c4a0ba69180ef3af=cmake%2Fmodules%2FFindKDE4Internal.cmake
 
 You'll see that it was quite different from what we have today. It is 
 much less than today. Back then I also started with a minimal set of flags to 
 get KDE built at all. But between then and now, all those changes have gone 
 in for a reason, each single one of them.
 
 
 P.S. I am not objecting, just giving comments.


1) By no surprises I mean by 3rd party users, skilled in Qt and cmake, of a KDE 
framework  - like if I end up using one at work and some of my colleagues need 
to fix a bug or add a feature, then this would be a unpleasant surprise when 
dealing with a standalone framework.

My plan isn't - yet - to remove the file completely, but first to slice it down 
to a size where one can see what happens and what side effects it has. I have 
at least concrete plans for two or three more chunks to remove, but I prefer 
slice it down chunks at a time.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113805/#review43538
---


On Nov. 11, 2013, 10:16 p.m., Sune Vuorela wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113805

Re: Review Request 113805: Do not change the build types available with cmake

[Sune Vuorela]

 On Nov. 12, 2013, 7:24 p.m., Alexander Neundorf wrote:
  IMO the patch as it is is not good.
  
  Several things:
  1) This file, is not mandatory at all with KDE frameworks.
  You can build applications using KDE frameworks libraries without it. You 
  (the developer of the application) simply has to not-load the file 
  KDECompilerSettings.
  If the developer has decided to load this file, it is not surprising that 
  he gets modified compiler flags, because this is what he decided to do.
  
  2) You removed e.g. the flags for the build types COVERAGE and PROFILE. 
  CMake doesn't provide these build types or flags itself, so the patch 
  removes support for these buildtypes. I don't see the need to remove 
  support for profile or coverage builds. CMake itself comes with debug, 
  minsizerel, release and relwithdebinfo.
  
  3) You remove the flags for Linux and/or gcc. Why didn't you remove them 
  for other compilers/operating systems ?
  
  
  I know that what we are doing in this file is not the cmake-recommended way.
  From cmake, the variables for the flags are cache variables which are set 
  to some default. The idea is that the person who compiles some package can 
  adjust them to his liking. This is from my experience not what we expect 
  from the average KDE contributor. He should get a working set up, with 
  known compiler flags so it is easy to fix buid bugs (or avoid them in the 
  first place).
  By simply setting the normal (non-cache) variables, the person building the 
  package can set the cache variables to whatever he wants, it will be 
  overridden to what is set in KDECompilerSettings.cmake.
  Maybe the way this is done could be improved by doing something like
  if(NOT DEFINED SOME_CXX_FLAGS_ALREADY_PRESET)
 set(SOME_CXX_FLAGS_ALREADY_PRESET TRUE CACHE BOOL docs... )
 set(SOME_CXX_FLAGS --some-flag --another-flag CACHE STRING docs... 
  FORCE)
  endif()
  
  This way it would be only on the initial cmake run forced into the cache, 
  and afterwards the user should be able to change them as he wants.
  
  
 
 
 Sune Vuorela wrote:
 1) THis file is used by all kde frameworks, so it should not put in 
 surprises for the developers there. ONe shouldn't be 100% familiar with all 
 the internals to hack on stuff.
 
 2) IF we want to add such things, it should be in a 
 ECMAddProfileBuildType or similar.
 
 3) For the other compilers, the build types aren't touched.
 
 ANd note that this doesn't modify the flags that covers everything. That 
 I'm saving for another patch.
 
 And let us do thhings the cmake way, one step at a time.
 
 Alexander Neundorf wrote:
 1) I don't really understand. You say no surprises and at the same time 
 you say that developers shouldn't have to be familiar with all internals to 
 hack on stuff. If you want no surprises, remove the line 
 include(KDECompilerSettings) from the project. That's why it has been 
 separated out, to make it optional for users who don't want it. Then you get 
 plain cmake, and can/have to take care of the compiler settings yourself. Add 
 that line, and you get no need to care about internals.
 
 Is your plan also to remove the added defintions, linker flags, etc. ?
 I'm fine if you post a patch which removes the file completely. I just 
 doubt that the KDE community will be happy with this.
 
 This is the state as it was May 2006:
 
 http://quickgit.kde.org/?p=kdelibs.gita=blobhb=5713bc5ddd7f11ef73e63cf67c4a0ba69180ef3af=cmake%2Fmodules%2FFindKDE4Internal.cmake
 
 You'll see that it was quite different from what we have today. It is 
 much less than today. Back then I also started with a minimal set of flags to 
 get KDE built at all. But between then and now, all those changes have gone 
 in for a reason, each single one of them.
 
 
 P.S. I am not objecting, just giving comments.

 
 Sune Vuorela wrote:
 1) By no surprises I mean by 3rd party users, skilled in Qt and cmake, of 
 a KDE framework  - like if I end up using one at work and some of my 
 colleagues need to fix a bug or add a feature, then this would be a 
 unpleasant surprise when dealing with a standalone framework.
 
 My plan isn't - yet - to remove the file completely, but first to slice 
 it down to a size where one can see what happens and what side effects it 
 has. I have at least concrete plans for two or three more chunks to remove, 
 but I prefer slice it down chunks at a time.
 
 Alexander Neundorf wrote:
 1) Let's assume karchive. You have currently at least the following 
 options
 
 find_package(KArchive REQUIRED NO_MODULE)
 
 This finds the library, KDECompilerSettings.cmake is not involved at all.
 
 
 OR
 
 find_package(KF5 REQUIRED NO_MODULE COMPONENTS KArchive)
 
 This finds the library, via tier1/kf5umbrella/, KDECompilerSettings.cmake 
 is not involved at all.
 
 OR
 
 (when

Re: Review Request 113503: make dbus dependency optional in JobWidgets

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113503/
---

(Updated Nov. 25, 2013, 1:56 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, kdelibs and Stephen Kelly.


Repository: kdelibs


Description
---

Make dbus dependency optional in JobWidgets

Many don't have dbus available on all platforms, especially windows, but 
JobWidgets is very much useful without it.


Diffs
-

  tier2/kjobwidgets/CMakeLists.txt ca53024 
  tier2/kjobwidgets/src/CMakeLists.txt 0a575a6 
  tier2/kjobwidgets/src/config-kjobwidgets.h.cmake 35b64a2 
  tier2/kjobwidgets/tests/kjobtrackerstest.cpp 7a61407 

Diff: http://git.reviewboard.kde.org/r/113503/diff/


Testing
---

build tested on windows without dbus. not yet tested on other platforms


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113805: Do not change the build types available with cmake

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/113805/
---

(Updated Jan. 9, 2014, 9:27 p.m.)


Status
--

This change has been discarded.


Review request for Build System and KDE Frameworks.


Repository: extra-cmake-modules


Description
---

Do not change the build types available with cmake.


Diffs
-

  kde-modules/KDECompilerSettings.cmake 
b034751a5be8073f9628971b552faa079c64e8b6 

Diff: https://git.reviewboard.kde.org/r/113805/diff/


Testing
---

Built kdelibs on linux with gcc.


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 115496: Rename CMakePackageConfigHelpers to ECMPackageConfigHelpers

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115496/#review50155
---

Ship it!


Looks great. Do you plan on a transition period of more than a week?

- Sune Vuorela


On Feb. 18, 2014, 3:26 p.m., Alex Merry wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/115496/
 ---
 
 (Updated Feb. 18, 2014, 3:26 p.m.)
 
 
 Review request for Build System, Extra Cmake Modules and KDE Frameworks.
 
 
 Repository: extra-cmake-modules
 
 
 Description
 ---
 
 Rename CMakePackageConfigHelpers to ECMPackageConfigHelpers
 
 Overriding a CMake package like this will just cause all sorts of
 headaches later on.  In this particular case, projects that depended on
 CMake 2.8.13 or later (more likely 3.0.0) would fail with a message
 about removing the CMakePackageConfigHelpers file, but would have no way
 to do that while still using ECM.
 
 This also renames the configure_package_config_file() macro to
 ecm_configure_package_config_file(), so that anything including
 CMakePackageConfigHelpers afterwards does not overwrite the macro
 unexpectedly.
 
 For now, we keep a CMakePackageConfigHelpers.cmake file that just wraps
 ecm_configure_package_config_file() as configure_package_config_file()
 to keep the frameworks building while they are ported.
 
 
 Diffs
 -
 
   modules/CMakePackageConfigHelpers.cmake 
 5d65e659f7a04d65aeb08fa99569e88dde89acf2 
   modules/ECMPackageConfigHelpers.cmake PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/115496/diff/
 
 
 Testing
 ---
 
 kdesrc-build has got to KIO so far with no problems.
 
 If KJS includes ECMPackageConfigHelpers and uses 
 ecm_configure_package_config_file(), both KJS and KI18n configure and build 
 properly.
 
 
 Thanks,
 
 Alex Merry
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

KPassivePopup should be in kwidgets ?

[Sune Vuorela]

Hi

I was just looking at knotification, and for some reason, KPassivePopup
is ended up here. I was quite surprised to see it there, as it is used
for 'popping up stuff' all over the place, and isn't really notification
specific.

Shouldn't it rather be in kwidgets?

/Sune
 - hoping for a widgets free knotification

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: qt5 polkit-qt-1 and kdesrc-build

[Sune Vuorela]

On 2014-06-27, David Faure fa...@kde.org wrote:
 On Saturday 01 March 2014 17:33:45 David Faure wrote:
  I don't mind adding it - but what about releasing?
  Is anyone taking care of releasing polkit-qt-?
  Or should we make it a framework so I release it with the rest of the
  stuff
  ? Cc'ing some polkit-qt contributors.

 This question is still open. We're releasing kauth as part of KF5 but
 polkit-qt-1 isn't getting released.

 Is there any maintainer for polkit-qt-1?

 For that matter, who maintains KAuth, which optionally depends on polkit-qt-1?

Isn't KAuth fully useless on linux without polkit-qt ?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Which package will provide the common KDE library version number ?

[Sune Vuorela]

On 2012-02-23, Alexander Neundorf neund...@kde.org wrote:
 I'd be fine with a find_package(KF5BuildSpecs 5.3.0), assuming the
 version number would be mandatory, as now there'd be no risk of a
 kwhatever 5.3.0 claiming to be a kwhatever 5.10.0.

 This can be done.
 Then this line has to be kept in sync in all repositories in some way.

So. We go from having a script to keep one line in sync across
repositories to another solution having another script keeping another
line in sync across repositories.

Either the set(CURRENT_LIBRARY_VERSION 5.3.10)
or the
find_package(KF5BuildSpecs 5.2.3).

And by chosing the latter we require everyone to do complete lock-step
upgrades.

Given that we promise source and binary compatibilities, requiring
lockstep upgrades is just wrong.

So really. Let's go back. Just because something can be done in advanced
ways doesn't mean we should, especially not if it is shooting our selves
and our downstreams in the feet

 And whatever we do, without a common, (possibly optional) root dependency 
 we 
 will loose the pick up install directories from kdelibs feature, AFAICT.

I'd dislike a common root dependency, as the 'kde frameworks' is also
all sorts of low level things where a added dependency on cmake is
already sometimes a blocker, so no need to introduce ardificial extra
bits that we don't really need.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Which package will provide the common KDE library version number ?

[Sune Vuorela]

On 2012-02-24, Alexander Neundorf neund...@kde.org wrote:
 * the version numbers of the projects themselves
 * the required Qt version
This might differ across frameworks. I see no reason to artificial bump
required version.

 * the required CMake version

similar.

 * the required extra-cmake-modules version

similar.

 repositories to another solution having another script keeping another
 line in sync across repositories.
 
 Either the set(CURRENT_LIBRARY_VERSION 5.3.10)
 or the
 find_package(KF5BuildSpecs 5.2.3).
 
 And by chosing the latter we require everyone to do complete lock-step
 upgrades.

 What  is a lock-step upgrade ?

A requirement to upgrade other packages to upgrade a different package.

Like FrameworkA and FrameworkB. One is related to fetching imap mails,
the other   is related to paint svg images on screen.

I could imagine to want to be able to upgrade one without the other.
Given we promise source and binary compatibilities, it should be
perfectly possible. Except if we introduce artificial limitations like
this one.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KF5 Volunteer day 2 next week?

[Sune Vuorela]

 On Sunday 18 March 2012 20:11:20 Kevin Ottens wrote:
 So, doable? Any volunteers (aha) to drive the volunteer day? :-)

 Dario indicated to me he could be available part of the afternoon, that=
 's nice=20
 but likely not enough.


I'll most likely be around, but I*m not sure of my ability to actually
drive stuff.

Mostly hoping for a quiet day to hack on the bits I've promised to do.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Notifications-future, a recap

[Sune Vuorela]

On 2012-09-17, Dario Freddi drf54...@gmail.com wrote:
 Hello everyone,

 you might or might not know by now of my intention of revamping the
 way we deal with notifications in KDE as I explained in my last blog
 post 
 http://drfav.wordpress.com/2012/09/17/the-notifications-issue-part-3-the-possible-solution/
 . I have been talking about this with many of you already but I think
 it's time to see what's the overall reception to the idea and what's
 coming up for the actual job.

I'm not sure I agree with the problems and thus, also not with the
proposed solutions.

 I know Sune already had some plans for the notification stack and I
 think that's one of the best times for discussing what's going to

yep. the plan is
1) write a small library wrapping the org.freedesktop.notification api
   wrap growl on mac
   wrap QSystemTrayIcon on windows
2) do something similar for audio notifications
3) retrofit knotification on top of those

and the more I have investigated, 1) should actually be put into Qt and
QPA.
That should also make it possible for e.g. webkit to use it to show web
notifications.  
Unfortunately, I'm currently a bit busy with stuff :/

 My personal idea is to have a new (tier1) framework consisting of a
 way for building handlers, a client API and a server API (so that the

Really? I_think we should get rid of a daemon and just let the workspace
shell handle it.

How the workspace chooses to handle it is not a discussion I'm planning
to enter. We have more skilled people for that than me.

I have a hard time figuring out why the above quite simple steps don't
solve the problems you're specifying (and even ensures that you keep all
existing applications working with their notifications)


/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Dialogs, KMessage box and job widgets

[Sune Vuorela]

On 2012-11-14, Valentin Rusu k...@rusu.info wrote:
 Hello,

 I have a question about KMessage class from kdeui/dialogs.

 According to the epics page, kdeui/dialogs will go to tier3.
 On the other hand, kdeui/jobs will go to tier2, only the class 
 KDialogJobUiDelegate is actually using KMessageBox.
 If KMessageBox will stay in dialogs, then tier2 jobs will need to call 
 tier3. I suppose that's not the desired situation.

 What would be the best solution in order to follow the defined policies? 
 Where should KMessageBox go?

I have wondered a couple of times why we split widgets and dialogs. 

and I think putting KDioalogJobUiUglyDelegate in a quite high tier is
okay, if it is what I remember it to be.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KArchive for Qt4

[Sune Vuorela]

On 2012-11-15, Alexander Neundorf neund...@kde.org wrote:
 I thought we earlier agreed on things like you should not inherit
 sonames from other modules and such.

We have apparantly a ECM_SOVERSION coming from somewhere and used.



 
 and we just have layers of added complexity that seems to be added for
 the sake of complexity.

 What do you mean exactly ?

We have a generated config file that I still haven't figured out where
it comes from (and which is quite buggy and hard to fix where you don't
know where it comes from)
We have a find_package(KF5...) that does all sorts of unexpected magic
We have magic install variables

but in all our magic, we have actually lost the
CamelCase/ForwardingHeaders

and this is just after a short while of trying to use some bits.


/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KArchive for Qt4

[Sune Vuorela]

On 2012-11-17, Alexander Neundorf neund...@kde.org wrote:
 On Thursday 15 November 2012, Sune Vuorela wrote:
 On 2012-11-15, Alexander Neundorf neund...@kde.org wrote:
  I thought we earlier agreed on things like you should not inherit
  sonames from other modules and such.
 
 We have apparantly a ECM_SOVERSION coming from somewhere and used.
 
  and we just have layers of added complexity that seems to be added for
  the sake of complexity.
  
  What do you mean exactly ?
 
 We have a generated config file that I still haven't figured out where
 it comes from (and which is quite buggy 

 What exactly is buggy in the file ?

at least the two following things (which is taken from memory, so there
might be typing issues)

threadweaver_LIBRARY is just threadweaver. I needed to figure out how
to add LINK_DIRECTORIES(${threadweaver_DIR}) the right places as well.
that was at least ... unexpected.

the variable for includes is not complete. you include most bits by
doing include threadweaver/foo, but threadweaver/foo includes
threadweaver_export.h, so the threadweaver_INCLUDE_DIR variable isn't
complete enough. I needed
include_directories(${threadweaver_INCLUDE_DIR}
${threadweaver_INCLUDE_DIR}/threadweaver) to get it to be used.

and when I was doing a installer on windows (where I need to include the
threadweaver.dll file) I had to do like
INSTALL(${CMAKE_INSTALL_PREFIX}/bin/lib${threadweaver_LIBRARY}.dll
DESTINATION bin) but I_think that's jsut a extra way of showing the
first issue.

So basically, getting the library to link and getting the include paths
was broken.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KArchive for Qt4

[Sune Vuorela]

On 2012-11-19, Stephen Kelly steve...@gmail.com wrote:
 I don't have a link to 'you should not inherit sonames from other
 modules'. but it sholud kind of be common sense as we also see in
 various areas of current KDE land where e.g. libkmailprivate from kdepim
 4.4 is not having a matching SONAME, but gets the SONAME from the
 kdelibs it is built against.

 I think I misread what you wrote before. You meant 'kde modules', not 'ecm 
 modules' AKA cmake files or macros.

No. I actually meant do not inherit SONAME from anywhere. including
another kde module or a ecm module or cmake.

 I'm not opposed to changing that (I think Alex wrote a replacement macro 
 already), but I don't think just renaming the macro makes it more 
 discoverable. If you didn't know the macro existed you'd still just be 
 looking at a '${FOO_VERSION}' that comes from somewhere and start looking 
 for a use of a macro with version in the name.

I agree that it is not a prefect fix, but definately a improvement. It
also helps to show that it originates *somewhere* inside this directory
structure and not somewhere else.

 Anyway - Yes, the ecm API is not perfect yet and not ready for consumption 
 by anyone yet. That will come though and I don't think it's something we 
 need to spend a lot of time worrying about or discussing just yet.

We do need to start to actually use it at least amongst powerusers to
ensure that we can actually commit to a nice source compatibility once
we are at a release.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kemoticons is now in staging

[Sune Vuorela]

On 2012-11-19, David Faure faure+bluesyst...@kde.org wrote:
 Let me say this differently: look at the huge mass of code in Qt: almost none 
 of it uses QSettings to let the users configure things from underneath.
 Most of the time, the apps themselves have control over the various settings 
 offered by the Qt API.

 The exception is the stuff available in `qtconfig`, but that's quite limited.

 So maybe this setting should be left to the application, if that makes 
 sense.

I was actually going to do a mail suggesting the same.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: add_library NO_PREFIX

[Sune Vuorela]

On 2012-11-29, Alexander Neundorf neund...@kde.org wrote:
 At least it is possible:

It is possible on some systems. I think it might be limited to a GNU
userland on unix-like systems, if not limited to GNU userland on linux.

It is at least according to some windows people not possible to do so on
windows.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Separating everything ?

[Sune Vuorela]

On 2013-02-08, Mirko Boehm mi...@kde.org wrote:
 As Frank said: I haven't seen any convincing argument yet why multiple 
 repositories are better. 
 +1.

There are several things here intermixed

1) is the release tarball layout
2) is the way the software is built
3) is the repository layout

all of them is different things with different arguments for and
against.

So... which ones of it is it we are talking about?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kde5 or kf5 ?

[Sune Vuorela]

On 2013-02-16, Alexander Neundorf neund...@kde.org wrote:
 _set_fancy(LIBEXEC_INSTALL_DIR  ${LIB_INSTALL_DIR}/kde5/libexec)

I still don't see a reason for a *shared* libexec directory. libexec is
implementation details of specific libraries, just like one shouldn't
mess around with each others private's in the code, one also shouldn't
mess around with other's libexec bits.

so libfoo's libexec should be lib_install_dir/foo/libexec

 _set_fancy(XDG_APPS_INSTALL_DIR ${SHARE_INSTALL_PREFIX}/applications/kde5 )

I'm also a bit unsure about this one. Sure it is nice to avoid path
conflicts with other applications hogging the same desktop file names,
but from the different end of things: if things does hog the same
desktop file name, the content of the file is probably going to be so
similar that everyone wants to know about it and ensure that the content
is different enough.

e.g. having two webbrowser.desktop files will not only give file
conflicts on the system, but also confuse users if they both say
Name=Web Browser

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kde5 or kf5 ?

[Sune Vuorela]

On 2013-02-16, Alexander Neundorf neund...@kde.org wrote:
 The tool kde4-config has also been renamed to kde5-config.

What's the purpose of this tool in a non-monolithic world?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KF5 Update Meeting Minutes 2013-w27

[Sune Vuorela]

On 2013-07-02, Alexander Neundorf neund...@kde.org wrote:
 E.g. kplotting and kwidgets have the same dependencies. I wouldn't mind 
 having 
 kplotting as part of kwidgets.

I do think that consideratiotns like these are very important, but I
also think we should wait with looking at merging them just a bit until
we are a bit further in the splitting.

And I do agree that if kplotting and kwidgets have the same dependencies
and none of them are absurdely large, they should probably be merged.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KIO progress towards tier 1 framework?

[Sune Vuorela]

On 2013-07-10, Kevin Ottens ervin+bluesyst...@kde.org wrote:
 OK thanks for the clarification. I don't know where I got that impressi=
 on that=20
 QtScript and Qml were using the same JS engine then...

it was that way in qt4 :)

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 111689: desktoptojson -- convert .desktop files to .json for plugin metadata

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111689/#review36501
---



staging/kservice/tools/desktoptojson/kconfigtojson.h
http://git.reviewboard.kde.org/r/111689/#comment26943

No need to have it inherit anything



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
http://git.reviewboard.kde.org/r/111689/#comment26946

maybe qPrintable(msg) instead



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
http://git.reviewboard.kde.org/r/111689/#comment26945

As such there is no need for a d-pointer, but it also doesn't hurt.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
http://git.reviewboard.kde.org/r/111689/#comment26944

Timer usage could be skipped with no eventloop running



staging/kservice/tools/desktoptojson/main.cpp
http://git.reviewboard.kde.org/r/111689/#comment26940

I don't think you need to 'new' it here. just let the scoping handle the 
deletion



staging/kservice/tools/desktoptojson/main.cpp
http://git.reviewboard.kde.org/r/111689/#comment26941

Just do it as a QCoreApplication first and then create a KConfigToJson 
afterwards



staging/kservice/tools/desktoptojson/main.cpp
http://git.reviewboard.kde.org/r/111689/#comment26942

and just return kconfigtojson.runMain(); here


For inspiration for doing simple command line tools, you could also take a look 
at https://codereview.qt-project.org/#change,60560,patchset=5 which has a 
simpler 'top down' structure which is how CLI-apps usually are.

- Sune Vuorela


On July 25, 2013, 4:10 p.m., Sebastian Kügler wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111689/
 ---
 
 (Updated July 25, 2013, 4:10 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Description
 ---
 
 Small program which takes a .desktop file and converts it to json. This is 
 useful to convert plugins which have their metadata in .desktop files (i.e. 
 all KDE plugins) to Qt's new plugin system.
 
 
 Diffs
 -
 
   staging/kservice/tools/CMakeLists.txt PRE-CREATION 
   staging/kservice/tools/desktoptojson/CMakeLists.txt PRE-CREATION 
   staging/kservice/tools/desktoptojson/kconfigtojson.h PRE-CREATION 
   staging/kservice/tools/desktoptojson/kconfigtojson.cpp PRE-CREATION 
   staging/kservice/tools/desktoptojson/main.cpp PRE-CREATION 
 
 Diff: http://git.reviewboard.kde.org/r/111689/diff/
 
 
 Testing
 ---
 
 Converted metadata of several plugins and used them from QPluginLoader -- 
 works.
 
 
 Thanks,
 
 Sebastian Kügler
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

install paths

[Sune Vuorela]

Hi peoples.

The following popped up in the chat channel today:

|kf5 sonnet is looking for plugins in
|$prefix/lib/x86_64-linux-gnu/plugins/sonnet_clients/ but installs them
|in $prefix/lib/x86_64-linux-gnu/plugins/kf5/plugins/sonnet_clients

or if I try to shorten it a bit

|kf5 sonnet is looking for plugins in
| LIBDIRplugins/sonnet_clients/ but installs them
|in LIBDIR/plugins/kf5/plugins/sonnet_clients


This immediately sparked discussions about especially the several layers
of /plugin/ in the install target.

My general take on stuff like that is that plugins for foo should go
somewhere under LIBDIR/foo/.

a directory like LIBDIR/plugins/ seems to imply that it is something
shared between stuff, but I don't see it as something that anyone is
using. Also I don't see how it makes sense to have a common plugin dir
for any type of plugin. So I think that that 'plugins' should go.

Next up is the '/kf5/' directory. I don't see what a shared kf5 directory
for plugins is for since plugins for e.g. libplasma is not sensibly
loadable by libsonnet or the other way around. So that one could go as
well, unless we want some branding in our filepaths.

Then there is another /plugin/ directory. see above.

and then we are down to LIBDIR/kf5/sonnet_clients if we feel we need
branding in the filepaths. or just LIBDIR/sonnet_clients if we just want
to promote our frameworks.

I don't see file paths as a thing to use in promo land.

Most of the weird bits in the paths is apparantly coming from a variable
called ${PLUGIN_INSTALL_DIR} which as a generic term doesn't really
makes sense.

Any opinions?

Love and kisses

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 111937: Port to KF5/Qt5

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111937/#review37300
---


I don't think it is yet time to *default* to building the qt5 versions. It is 
these days quite normal to have both qt4 and qt5 installed and available. 

- Sune Vuorela


On Aug. 7, 2013, 5:15 p.m., Alexander Richardson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111937/
 ---
 
 (Updated Aug. 7, 2013, 5:15 p.m.)
 
 
 Review request for KDE Frameworks and Konsole.
 
 
 Description
 ---
 
 Port to KF5/Qt5
 
 It will build for KF5 automatically if Qt5 is found.
 You can override this behaviour by using cmake -DFORCE_QT4=ON
 
 TerminalDisplayAccessible is disable for Qt5 currently since I don't
 have any experience with accessible stuff and it is more complicated
 than just changing a few includes
 
 
 Diffs
 -
 
   CMakeLists.txt 5783804588bc4b1d1687e2dd1aaff76f3a65906e 
   src/BookmarkHandler.h 76c33269e5f20cbe20c97cfa621236cf05dc0f53 
   src/BookmarkHandler.cpp 9fc1600aa5a2901549f87295894acaf88b2f55ed 
   src/CMakeLists.txt 48601a147b4ba9a48eee064d9071382d6dd683b1 
   src/ColorScheme.cpp 6945c39144d12d8faefbb5242edfa1899e278477 
   src/ColorSchemeEditor.cpp afe13c3a447197ff864639be3d6c136b146c8c66 
   src/ColorSchemeManager.cpp 802f5d7f9d3a1008a8785b27546a7ffa27fac24c 
   src/EditProfileDialog.cpp aff67b5723e2f1e08cf6aebc82f4911ffadc353e 
   src/Emulation.cpp 02ed4be48a5916c75c7731864b33b4a825c31e6e 
   src/Filter.cpp 8f40c0c598bd1f61ec2ad9765c2399d0ba011744 
   src/KeyBindingEditor.cpp 005b9e32027dd14f9965ec6e2e7dd32e94b79272 
   src/KeyboardTranslator.h 99c71743d6c295c14381a83787baa061e7703878 
   src/KeyboardTranslator.cpp 8fe2acb40d75b62595b9823a990ba99f0e6e0509 
   src/KeyboardTranslatorManager.cpp 4d46210517e901b14e379dcfe435fd8fdedaeb09 
   src/MainWindow.cpp 285d6347be6680632b912b51961ced4d797a10bc 
   src/Part.h 2806bcdb066814ca32e1905772e99a0b2de426e5 
   src/Part.cpp 33e2d3849629e415817dc9a5f241e4ac3a323a28 
   src/PrintOptions.cpp 5e802987d5872f9041438a1ee69ec8162ddb962a 
   src/Profile.cpp 14946ce69000eef00b03864e4610c6775597a51d 
   src/SessionController.h 036e0534960fe7a1bd32be04136ba162c0469413 
   src/SessionController.cpp 88fc50c6a164fed94eefc72a71e4460f15ede57e 
   src/ShellCommand.cpp 2b059a16e6232146cc9d75b1b6af25919fdcc4e7 
   src/TerminalDisplay.cpp acfddc176bc68ae9b0f3a9ea52c958e37d14dff8 
   src/TerminalDisplayAccessible.h c6964412559e880dd27cf66a9fa8759196a09980 
   src/TerminalDisplayAccessible.cpp 8d37dc5e13448fc5d8619078c50334b0813597d4 
   src/ViewManager.cpp 4b9708e4a135a3682b319351ba02ce1d310c3329 
   src/Vt102Emulation.cpp 0b6d2ed2f75630df9fc7ac5d6d2b5d9a6702f254 
   src/main.cpp 2afc62c721b0e77264d0784caf7fbbd70b13951f 
   src/tests/PartTest.cpp 72cd27de07c0e96f8986db3a40dcf9239d9b3529 
   src/tests/ShellCommandTest.cpp d03964fda57a1b37d4a02c82c3c04fd00040aaff 
 
 Diff: http://git.reviewboard.kde.org/r/111937/diff/
 
 
 Testing
 ---
 
 It compiles fine (both Qt4 and Qt5).
 After installation the terminal view of KF5 based okteta started working.
 
 
 Thanks,
 
 Alexander Richardson
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Smart D-Ptr in KCoreAddons

[Sune Vuorela]

On 2013-08-23, Ivan Čukić ivan.cu...@kde.org wrote:
 oh. and I think it also looks a bit like your d-ptr gets in the way when
 you need a d-ptr hierachy.

 As I said, this is for non-inherited privates.

That alone is  -  I think  -  a reason to not have it.

The few cases where I have needed d-ptr hierachies, I didn't know about
it in advance but was happy than I could extend my classes without having 
to use a different smartpointer (with possibly a different size).

We shouldn't make our tools so convenient that they end up being in our
way.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119564: Add define to re-enable Qt functionality we depend on.

[Sune Vuorela]

 On Aug. 2, 2014, 9:04 a.m., Alex Merry wrote:
  I would rather change the code. The Qt behaviour was changed for a reason, 
  to prevent accidental use of dangerous behaviour, and I'm not too keen on 
  undoing that move for all software that uses KDECompilerSettings.

agreed.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119564/#review63665
---


On Aug. 1, 2014, 4:06 p.m., Axel Rasmussen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119564/
 ---
 
 (Updated Aug. 1, 2014, 4:06 p.m.)
 
 
 Review request for Build System, Extra Cmake Modules and KDE Frameworks.
 
 
 Bugs: 337472
 http://bugs.kde.org/show_bug.cgi?id=337472
 
 
 Repository: extra-cmake-modules
 
 
 Description
 ---
 
 Upstream Qt commit e112c2e altered the way QExplicitlySharedDataPointer 
 behaves by default, such that it no longer uses a `static_cast` to cast 
 compatible pointers. A nontrivial amount of KDE code (I encountered the build 
 error in KService, although there are other users) depends on this 
 functionality, so this commit adds a define to the build system which 
 re-enables the code in Qt.
 
 
 Diffs
 -
 
   kde-modules/KDECompilerSettings.cmake fdc930e 
 
 Diff: https://git.reviewboard.kde.org/r/119564/diff/
 
 
 Testing
 ---
 
 I applied this patch, attempted to build KService, and the build succeeded, 
 rather than exhibiting the compile errors found in the log provided on the 
 bug report.
 
 
 Thanks,
 
 Axel Rasmussen
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119901: Fix ECM to use qmake instead of hardcoding plugin install dirs

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119901/#review65037
---


Looks good. THere is a couple of things that is kind of a matter of taste and 
will let Alex comment on it:
* should the qmake-query function be a secret underscore function
* should the setting of the qmake_exe var happen inside the function or not

- Sune Vuorela


On Aug. 22, 2014, 12:40 p.m., Rohan Garg wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119901/
 ---
 
 (Updated Aug. 22, 2014, 12:40 p.m.)
 
 
 Review request for Build System and KDE Frameworks.
 
 
 Repository: extra-cmake-modules
 
 
 Description
 ---
 
 Use qmake to query dirs for plugins and imports instead of hardcoding them in 
 ECM.
 
 
 Diffs
 -
 
   kde-modules/KDEInstallDirs.cmake 880539b 
 
 Diff: https://git.reviewboard.kde.org/r/119901/diff/
 
 
 Testing
 ---
 
 Seems to work on my system.
 
 
 Thanks,
 
 Rohan Garg
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 120139: kill warning

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120139/
---

Review request for KDE Frameworks, Andreas Hartmetz and Stephen Kelly.


Repository: kitemviews


Description
---

The warninng is even triggered by from internal KWidgetItemDelegatePrivate, so 
seems to be wrong

Try out the kwidgetitemdelegate test application


Diffs
-

  src/kwidgetitemdelegatepool.cpp d61802e 

Diff: https://git.reviewboard.kde.org/r/120139/diff/


Testing
---

Warning not printed.


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: desktoptojson man page

[Sune Vuorela]

On 2014-09-15, Alexander Richardson arichardson@gmail.com wrote:
 However, is this even possible? Building manpages seems to require KDocTools
 and kcoreaddons is a tier1 framework which would make this impossible.
 Do we really need a manpage for it? It seems to me that it is only
 used in cmake code

Or rewrite the man page in troff directly.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121712: Add install target for the KF5 Book

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121712/#review72687
---


looks right to me

- Sune Vuorela


On Dec. 28, 2014, 11:58 p.m., Andreas Cord-Landwehr wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121712/
 ---
 
 (Updated Dec. 28, 2014, 11:58 p.m.)
 
 
 Review request for KDE Frameworks, Rohan Garg, Mirko Boehm, and Valorie 
 Zimmerman.
 
 
 Repository: kf5book
 
 
 Description
 ---
 
 Install the book to /usr/share/doc/frameworks5
 
 
 Diffs
 -
 
   CMakeLists.txt 4e10120 
 
 Diff: https://git.reviewboard.kde.org/r/121712/diff/
 
 
 Testing
 ---
 
 manual testing
 
 
 Thanks,
 
 Andreas Cord-Landwehr
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Making prison a framework

[Sune Vuorela]

Hi

The KDESupport library, libprison, a bar code rendering abstraction
layer could be heading forward for the first release of a qt5, widgets
free version.

People also suggesting to make it a framework.

So please, take a look at kde:prison and let's try to get it reviewed
for frameworks

/Sune
 - developer of that thing 

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124105: Adhere a bit better to the spec

[Sune Vuorela]

 On June 15, 2015, 11:38 p.m., Christoph Feck wrote:
  Could you please describe (or code) a test case? Or point to a bug report?

It was things I stumbled upon while working on 
https://bugs.kde.org/show_bug.cgi?id=344469 - but unit test attached now.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124105/#review81499
---


On June 16, 2015, 5:59 a.m., Sune Vuorela wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124105/
 ---
 
 (Updated June 16, 2015, 5:59 a.m.)
 
 
 Review request for KDE Frameworks, David Faure and Martin Klapetek.
 
 
 Repository: kiconthemes
 
 
 Description
 ---
 
 According to table 2 in the icon theme spec, the Context per directory key is 
 optional, and the Type key defaults to Threshold if not specified. Let's make 
 the code do the same.
 
 
 Diffs
 -
 
   autotests/breeze.theme 8aef88d 
   autotests/kiconloader_unittest.cpp d1a52fc 
   src/kicontheme.cpp 4f0e522 
 
 Diff: https://git.reviewboard.kde.org/r/124105/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Sune Vuorela
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124105: Adhere a bit better to the spec

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124105/
---

(Updated June 16, 2015, 5:59 a.m.)


Review request for KDE Frameworks, David Faure and Martin Klapetek.


Changes
---

Add space after ,.
Add unit tests.


Repository: kiconthemes


Description
---

According to table 2 in the icon theme spec, the Context per directory key is 
optional, and the Type key defaults to Threshold if not specified. Let's make 
the code do the same.


Diffs (updated)
-

  autotests/breeze.theme 8aef88d 
  autotests/kiconloader_unittest.cpp d1a52fc 
  src/kicontheme.cpp 4f0e522 

Diff: https://git.reviewboard.kde.org/r/124105/diff/


Testing
---


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 124105: Adhere a bit better to the spec

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124105/
---

Review request for KDE Frameworks, David Faure and Martin Klapetek.


Repository: kiconthemes


Description
---

According to table 2 in the icon theme spec, the Context per directory key is 
optional, and the Type key defaults to Threshold if not specified. Let's make 
the code do the same.


Diffs
-

  src/kicontheme.cpp 4f0e522 

Diff: https://git.reviewboard.kde.org/r/124105/diff/


Testing
---


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124892: bug 342962: kdeclarative plugins should be built as a bundle plugin and not a shared library

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124892/#review84226
---

Ship it!


I'm not Harald, but no. it won't.

- Sune Vuorela


On Aug. 23, 2015, 3:22 p.m., Hanspeter Niederstrasser wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124892/
 ---
 
 (Updated Aug. 23, 2015, 3:22 p.m.)
 
 
 Review request for Build System, KDE Software on Mac OS X, KDE Frameworks, 
 Plasma, and Harald Sitter.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 The kdeclarative plugins (draganddropplugin, kcoreaddonsplugin, kio, 
 kquickcontrolsprivateplugin, and kquickcontrolsaddonsplugin) are being built 
 as shared libraries. They should be built as bundles (MODULE) in the 
 CMakeLists.txt file.
 
 When built as SHARED as in the current code, libdraganddropplugin.dylib gets 
 installed to $PREFIX/share/qt5/qml/org/kde/draganddrop, but is given an OS X 
 install_name of $PREFIX/lib/libdraganddropplugin.dylib. This mismatch can 
 cause problems. It is also given a compatibility_version of 0.0.0.
 
 
 Diffs
 -
 
   src/qmlcontrols/draganddrop/CMakeLists.txt e8127e4 
   src/qmlcontrols/kcoreaddons/CMakeLists.txt 3f77f2d 
   src/qmlcontrols/kioplugin/CMakeLists.txt 7b258e0 
   src/qmlcontrols/kquickcontrols/private/CMakeLists.txt da355c1 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 5b711e1 
 
 Diff: https://git.reviewboard.kde.org/r/124892/diff/
 
 
 Testing
 ---
 
 Since the plugin is not supposed to be a linkable library, it should be built 
 as MODULE in CMakeLists.txt. The physical install location remains the same 
 and plugins don't have install_names. This corrects the install_name/install 
 location mismatch. The change should not have any effect on non-OS X systems.
 
 
 Thanks,
 
 Hanspeter Niederstrasser
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124331/#review82521
---



src/kextracolumnsproxymodel.h (line 39)
https://git.reviewboard.kde.org/r/124331/#comment56929

Are there any of these not supported things we might want to support in 
the future? Can we implement e.g. adding/removing columns at runtime and still 
keep the abi/api ?
I guess for removing columns, one could just add a QSFPM on top and filter 
out the column.


- Sune Vuorela


On July 14, 2015, 10:42 a.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124331/
 ---
 
 (Updated July 14, 2015, 10:42 a.m.)
 
 
 Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
 
 
 Repository: kitemmodels
 
 
 Description
 ---
 
 REVIEW: 124331
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
   src/kextracolumnsproxymodel.h PRE-CREATION 
   src/kextracolumnsproxymodel.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/124331/diff/
 
 
 Testing
 ---
 
 Wrote autotests (as extensive as possible); used this in one real project, 
 AFAIK Jesper (blackie) did as well.
 
 
 Thanks,
 
 David Faure
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124331/#review82472
---


I like the concept, and have a whole bunch of nitpickery and a couple of more 
important bits.


src/kextracolumnsproxymodel.h (line 92)
https://git.reviewboard.kde.org/r/124331/#comment56881

where is the implementation of this? (and the unit tests ?  :)



src/kextracolumnsproxymodel.cpp (line 43)
https://git.reviewboard.kde.org/r/124331/#comment56877

Are this one of the cases where mmutz and mwolff won't hunt you down for 
using QList ?



src/kextracolumnsproxymodel.cpp (line 77)
https://git.reviewboard.kde.org/r/124331/#comment56875

Is this assert actually needed? isn't it kind of valid to pass in a nullptr 
?



src/kextracolumnsproxymodel.cpp (line 78)
https://git.reviewboard.kde.org/r/124331/#comment56876

shouldn't the old source model's about to be changed signals be 
disconnected here, or are QAPM taking care of that ?



src/kextracolumnsproxymodel.cpp (line 99)
https://git.reviewboard.kde.org/r/124331/#comment56880

isn't this kind of a normal state. is noisy output warranted here?



src/kextracolumnsproxymodel.cpp (line 115)
https://git.reviewboard.kde.org/r/124331/#comment56878

I know it is widely used outside Qt but .. Q_D is actually not public Qt 
api. Similar for Q_Q.



src/kextracolumnsproxymodel.cpp (line 170)
https://git.reviewboard.kde.org/r/124331/#comment56879

this is surprising me a bit. I'd expect that I by default would have 
emitted dataChanged inside my implementation of setExtraColumnData, especially 
if setting a piece of data influences multiple roles.



src/kextracolumnsproxymodel.cpp (line 194)
https://git.reviewboard.kde.org/r/124331/#comment56874

Wouldn't it be better to redelegate this to a set of separate functions, 
just like the actual data so that other roles are actually supported? (In my 
similar, but not as thorough implementations of this, I've often needed other 
roles like color and/or decoration)


- Sune Vuorela


On July 13, 2015, 8:31 a.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124331/
 ---
 
 (Updated July 13, 2015, 8:31 a.m.)
 
 
 Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
 
 
 Repository: kitemmodels
 
 
 Description
 ---
 
 REVIEW: 124331
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
   src/kextracolumnsproxymodel.h PRE-CREATION 
   src/kextracolumnsproxymodel.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/124331/diff/
 
 
 Testing
 ---
 
 Wrote autotests (as extensive as possible); used this in one real project, 
 AFAIK Jesper (blackie) did as well.
 
 
 Thanks,
 
 David Faure
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124281/#review82467
---


In general looks good.


src/knotificationmanager.cpp (line 29)
https://git.reviewboard.kde.org/r/124281/#comment56872

Unused ?



src/notifybypopup.cpp (line 561)
https://git.reviewboard.kde.org/r/124281/#comment56873

Isn,t QFont() the same as QApplication::font(), and then the #include 
QApplication seems unused.


- Sune Vuorela


On July 9, 2015, 1:10 p.m., Martin Klapetek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124281/
 ---
 
 (Updated July 9, 2015, 1:10 p.m.)
 
 
 Review request for KDE Frameworks and Martin Gräßlin.
 
 
 Repository: knotifications
 
 
 Description
 ---
 
 This patch reduces the dependencies of KNotifications framework and 
 effectively makes it a tier 2 framework.
 
 KService is used only for locating additional notification plugins and to my 
 knowledge,
 there are none such plugins currently existing, at least not in all around 
 KDE plus
 the class for the plugins wasn't exported until about two months ago, so this 
 should
 be safe without a legacy support.
 
 KIconThemes is used only to get KIconLoader::Small icon pixmap for 
 notifications
 using KPassivePopup. After some playing around it turns out that it puts the 
 icon into
 the KPassivePopup title and makes it as big as the text. So I've made the 
 icon size to
 be the same as the text height. So this keeps things visually the same + 
 still DPI aware,
 though I believe the KPassivePopup should receive a complete visual overhaul 
 anyway.
 
 Additionally, KCodecs dependency has again one single usage for decoding html 
 entities
 to QChars within QXmlStreamReader parser, so eventually could also be 
 removed/replaced
 with QTextDocument::toPlainText() which seems to do the same job as 
 QXmlStreamReader+KCodecs.
 
 
 Diffs
 -
 
   CMakeLists.txt 2d5437b 
   metainfo.yaml 7fc15f7 
   src/CMakeLists.txt 1cebece 
   src/knotificationmanager.cpp 8d4f953 
   src/knotificationplugin.cpp 7315c17 
   src/notifybypopup.cpp e377051 
   tests/kpassivepopuptest.h bc0dedc 
   tests/kpassivepopuptest.cpp 2486fd8 
 
 Diff: https://git.reviewboard.kde.org/r/124281/diff/
 
 
 Testing
 ---
 
 Everything still compiles + I added a test for KPassivePopup with an icon.
 
 
 Thanks,
 
 Martin Klapetek
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: phonon dep in knotifications

[Sune Vuorela]

On 2015-10-12, Christoph Cullmann  wrote:
> Hi,
>
> I would like to make this dependency optional.
> Would that be ok? Is not that much work, but if it is not wanted, don't want 
> to waste my time.
> E.g. on Win/Mac its a real hassle to build and most applications don't need 
> it anyway but
> might depend on knotifications.

I think it makes sense to make it optional. Added points if it can be
optional in a api/abi compliant way.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Policy for Dependencies

[Sune Vuorela]

On 2015-10-13, Martin Graesslin  wrote:
> I'm not sure whether it's the best solution. The problem you try to fix with 
> it is distros breaking packaging. I agree with Martin K that this is a huge 
> problem and that it will happen - since the automation of packages I also 
> experienced that nobody looks at the output of optional dependencies and the 
> packaging breaks.

I do think that such packagers should be slapped around with a large
trout. Or something.

> But I'm not sure how this could be done. Anyway, long story short: I think we 
> need the other way around. It should be optional by default, but should be 
> turned into stricter requirements on the linux distro case.

There is also another angle to the dependencies. What dependencies can
be enabled/disabled without requiring changes to users of the library.

Or put it another way. Is the enabling/disabling of a given feature ABI
and API compatible.

I do think that for features that doesn't impact the API/ABI we should
make it very easy to enable/disable them. Like based on having things
present on the system or not.

But for things that affects the API/ABI of the library, people should be
explicit about it.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125615: Make phonon dependency optional

[Sune Vuorela]

> On Oct. 13, 2015, 2:26 a.m., Martin Klapetek wrote:
> > As a maintainer, I would be much much more happy if it actually had a check 
> > for win/mac rather than making audio notifications optional always. Also 
> > speaking as someone who deals with knotifications bug reports.
> 
> Martin Klapetek wrote:
> ...and I'm sure there will be one about no sound in notifications in 
> Plasma sooner or later, which I would like to avoid.
> 
> (pressed enter too soon)

I like that things that aren't actually specific to an OS aren't marked as 
such, though one could also argue that it should be required unless passed 
-DDISABLE_PHONON or similar to cmake.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125615/#review86771
---


On Oct. 12, 2015, 9:01 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125615/
> ---
> 
> (Updated Oct. 12, 2015, 9:01 p.m.)
> 
> 
> Review request for KDE Frameworks and Sune Vuorela.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Make phonon dependency optional, purely internal change, like it is done for 
> speech.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 5a4ec83 
>   src/CMakeLists.txt 2a88b5a 
>   src/knotificationmanager.cpp 1c392b4 
> 
> Diff: https://git.reviewboard.kde.org/r/125615/diff/
> 
> 
> Testing
> ---
> 
> Still builds with (and now even without) phonon here.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124963: New widget: KNewPasswordWidget

[Sune Vuorela]

 On Aug. 28, 2015, 11:30 a.m., Lamarque Souza wrote:
  src/knewpasswordwidget.cpp, line 75
  https://git.reviewboard.kde.org/r/124963/diff/1/?file=399453#file399453line75
 
  i18n instead of tr

Aren't we in a tier1 framework where i18n and friends aren't available?


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124963/#review84525
---


On Aug. 28, 2015, 3:52 p.m., Elvis Angelaccio wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124963/
 ---
 
 (Updated Aug. 28, 2015, 3:52 p.m.)
 
 
 Review request for KDE Frameworks and Christoph Feck.
 
 
 Repository: kwidgetsaddons
 
 
 Description
 ---
 
 This widget is a stripped-down version of KNewPasswordDialog, without any 
 dialog-specific stuff.
 
 ### Why a new widget?
 
 This widget is meant to be easily embedded in any custom password dialog, 
 including KNewPasswordDialog. It's the least common denominator of features 
 that any password dialog should offer to the user.
 
 ### Features
 
 * Password visibility action (same as RR 124698). The password verification 
 field is hidden if the user shows the password.
 * Background warning colour. The password verification field gets a coloured 
 background whenever the password and its verification don't match. (feature 
 borrowed from Keepass)
 * Password status signal. This allows the upper level dialogs to update their 
 stuff (enable/disable OK button, show warnings for low password strength, 
 etc.)
 * Password strength bar can be hidden.
 * Unit test.
 
 ### Use cases
 At least the following:
 
 * Ark (new archive dialog)
 * KNewPasswordDialog refactoring (my next RR if this one gets accepted)
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt ac708ef33e3be89db85d43911f96e536c52f741d 
   autotests/knewpasswordwidgettest.h PRE-CREATION 
   autotests/knewpasswordwidgettest.cpp PRE-CREATION 
   src/CMakeLists.txt e03e9bbd6d73811873b0a465f86da269f4295138 
   src/knewpasswordwidget.h PRE-CREATION 
   src/knewpasswordwidget.cpp PRE-CREATION 
   src/knewpasswordwidget.ui PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/124963/diff/
 
 
 Testing
 ---
 
 
 File Attachments
 
 
 knewpasswordwidget1.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/5796469c-28b7-4151-b9cb-6a327631db75__knewpasswordwidget1.png
 knewpasswordwidget2.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/b3bfc9ac-ab8e-404c-8091-b5ad9e3e054f__knewpasswordwidget2.png
 knewpasswordwidget3.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/dfaeea4e-65da-4961-b266-986a21f54ca7__knewpasswordwidget3.png
 
 
 Thanks,
 
 Elvis Angelaccio
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124963: New widget: KNewPasswordWidget

[Sune Vuorela]

 On Aug. 28, 2015, 11:30 a.m., Lamarque Souza wrote:
  src/knewpasswordwidget.cpp, line 75
  https://git.reviewboard.kde.org/r/124963/diff/1/?file=399453#file399453line75
 
  i18n instead of tr
 
 Sune Vuorela wrote:
 Aren't we in a tier1 framework where i18n and friends aren't available?
 
 Lamarque Souza wrote:
 Yeah, you're right. Please revert the i18n change and sorry for not 
 noticing that before.
 
 Martin Klapetek wrote:
 Fwiw, kwidgetsaddons has i18n usage already.

Not in 77e030112909e218aa85f851b289d298dc68a9f2 at least. There is some 
examples that promotes usage of i18n, but none of the actual code uses it.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124963/#review84525
---


On Aug. 28, 2015, 3:52 p.m., Elvis Angelaccio wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124963/
 ---
 
 (Updated Aug. 28, 2015, 3:52 p.m.)
 
 
 Review request for KDE Frameworks and Christoph Feck.
 
 
 Repository: kwidgetsaddons
 
 
 Description
 ---
 
 This widget is a stripped-down version of KNewPasswordDialog, without any 
 dialog-specific stuff.
 
 ### Why a new widget?
 
 This widget is meant to be easily embedded in any custom password dialog, 
 including KNewPasswordDialog. It's the least common denominator of features 
 that any password dialog should offer to the user.
 
 ### Features
 
 * Password visibility action (same as RR 124698). The password verification 
 field is hidden if the user shows the password.
 * Background warning colour. The password verification field gets a coloured 
 background whenever the password and its verification don't match. (feature 
 borrowed from Keepass)
 * Password status signal. This allows the upper level dialogs to update their 
 stuff (enable/disable OK button, show warnings for low password strength, 
 etc.)
 * Password strength bar can be hidden.
 * Unit test.
 
 ### Use cases
 At least the following:
 
 * Ark (new archive dialog)
 * KNewPasswordDialog refactoring (my next RR if this one gets accepted)
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt ac708ef33e3be89db85d43911f96e536c52f741d 
   autotests/knewpasswordwidgettest.h PRE-CREATION 
   autotests/knewpasswordwidgettest.cpp PRE-CREATION 
   src/CMakeLists.txt e03e9bbd6d73811873b0a465f86da269f4295138 
   src/knewpasswordwidget.h PRE-CREATION 
   src/knewpasswordwidget.cpp PRE-CREATION 
   src/knewpasswordwidget.ui PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/124963/diff/
 
 
 Testing
 ---
 
 
 File Attachments
 
 
 knewpasswordwidget1.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/5796469c-28b7-4151-b9cb-6a327631db75__knewpasswordwidget1.png
 knewpasswordwidget2.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/b3bfc9ac-ab8e-404c-8091-b5ad9e3e054f__knewpasswordwidget2.png
 knewpasswordwidget3.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/dfaeea4e-65da-4961-b266-986a21f54ca7__knewpasswordwidget3.png
 
 
 Thanks,
 
 Elvis Angelaccio
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125436: Add an interface which allow plugin to show custom overlay icons (in KIO)

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125436/#review86063
---



src/widgets/koverlayiconplugin.h (line 55)
<https://git.reviewboard.kde.org/r/125436/#comment59376>

Is a plugin expected to return multiple overlays, and if so are consumers 
of this interface expected to stack them all together? In what order?

How is multiple oevrlay plugins to be handled? all of them stacked together 
? In what order?

I am wondering if either a QList - or let the plugin do the 
squashing - just QIcon - is a better return value.

the naming "getOverlays" feels a bit java-esque.
oh. and should it be const?


I think I like the concept, and it feels like kio is the right home for it.

- Sune Vuorela


On Sept. 28, 2015, 9:14 a.m., Olivier Goffart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125436/
> ---
> 
> (Updated Sept. 28, 2015, 9:14 a.m.)
> 
> 
> Review request for Dolphin and KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> The interface from https://git.reviewboard.kde.org/r/125136/ moved into KIO.
> 
> 
> Diffs
> -
> 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/koverlayiconplugin.cpp PRE-CREATION 
>   src/widgets/koverlayiconplugin.desktop PRE-CREATION 
>   src/widgets/koverlayiconplugin.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125436/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Olivier Goffart
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125418: Add method to revert knotifyconfigwidget to default notifications

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125418/#review86002
---



src/knotifyconfigwidget.h (line 73)
<https://git.reviewboard.kde.org/r/125418/#comment59352>

This function name for a setter looks weird to me.
setDefaults()
reset()
restoreDefaults()

would be better names in my opinion. 

defaults() doesn't look like an action, so it must be a getter.


- Sune Vuorela


On Sept. 27, 2015, 12:42 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125418/
> ---
> 
> (Updated Sept. 27, 2015, 12:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifyconfig
> 
> 
> Description
> ---
> 
> Add method to revert knotifyconfigwidget to defaults
> 
> 
> Diffs
> -
> 
>   src/knotifyconfigwidget.h dd336bc996008809678a6f44cbefe4bf0761908d 
>   src/knotifyconfigwidget.cpp 72c826de3d4c6984b3daac351e5a9a5a6a5bb1eb 
>   src/knotifyeventlist.h 110e3932fb94528a72cec1da911c734c174e1ba8 
>   src/knotifyeventlist.cpp dd6b6e9d5557fde0b46d980e4078921dd978f13f 
> 
> Diff: https://git.reviewboard.kde.org/r/125418/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125318: KBuildSycocaProgressDialog: use Qt's builtin busy indicator.

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125318/#review85681
---


If we can't determine the time, then yes, let's use the 'infinite spinner' 
instead of fake calculations.

I'm not sure I'm enough into the kservice code to actually give a shipit, so 
I'll refrain from that.

- Sune Vuorela


On Sept. 19, 2015, 10:58 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125318/
> ---
> 
> (Updated Sept. 19, 2015, 10:58 p.m.)
> 
> 
> Review request for KDE Frameworks and Albert Astals Cid.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This makes the code much simpler and looks better to the user
> than a fake progress.
> 
> Also removed the 1s delay before closing the dialog.
> Users like fast, not slow ;)
> 
> BUG: 158672
> Change-Id: I4a5cc975239d5c2998cdc3079890834c23ae677d
> REVIEW: 125318
> FIXED-IN: 5.15
> 
> 
> Diffs
> -
> 
>   src/widgets/kbuildsycocaprogressdialog.h 
> 88c5087990c01725e065fc650e63de246b3032e1 
>   src/widgets/kbuildsycocaprogressdialog.cpp 
> 78d23dfda174f195e4c7fdfc1e128e83194326f3 
> 
> Diff: https://git.reviewboard.kde.org/r/125318/diff/
> 
> 
> Testing
> ---
> 
> kio/tests/ksycocaupdatetest, after deleting ksycoca so that it doesn't end 
> too fast ;)
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125279: KSycoca: change DB filename to include language and sha1 of the dirs it's built from.

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125279/#review85682
---


What happens on file systems that doesn't support hardlinks ?

- Sune Vuorela


On Sept. 19, 2015, 11:29 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125279/
> ---
> 
> (Updated Sept. 19, 2015, 11:29 p.m.)
> 
> 
> Review request for KDE Frameworks and Albert Astals Cid.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> This will prevent sycoca-rebuild ping-pong if two apps with different settings
> would share the same file (and keep finding that it's wrong for them),
> and it fixes Albert's bug that "LANG=de kcmshell5 --list" doesn't show German
> translations for the strings coming from desktop files.
> 
> To avoid migration issues, the old filename ksycoca5 is still provided, as a 
> hardlink
> so that mmap works.
> 
> REVIEW: 125279
> BUG: 340731
> FIXED-IN: 5.15
> 
> 
> Diffs
> -
> 
>   autotests/ksycocatest.cpp 7c2d91e056726540b8a3a5c679d9b2a93f023c50 
>   docs/kbuildsycoca5/man-kbuildsycoca5.8.docbook 
> d7f85813b1956b578600b4fbd147b3ac649b470c 
>   src/sycoca/kbuildsycoca.cpp d3328f05bf761979bdffaa19f771e949759d6b76 
>   src/sycoca/ksycoca.h 34a2f6375148440aa6dc130395ed0e13b11d16e0 
>   src/sycoca/ksycoca.cpp a2b70407db3778196216969932c995c11f44fcf6 
> 
> Diff: https://git.reviewboard.kde.org/r/125279/diff/
> 
> 
> Testing
> ---
> 
> unittests, after adjusting ksycocatest which was checking for the old 
> behavior (same file).
> 
> Albert: there we are, finally ;) Your bug should be fixed (I don't have 
> translations installed to test it, though). Thanks for all the reviews!
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125308: KSycoca: make ensureCacheValid() part of the public API.

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125308/#review85684
---

Ship it!


Ship It!

- Sune Vuorela


On Sept. 19, 2015, 8:24 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125308/
> ---
> 
> (Updated Sept. 19, 2015, 8:24 p.m.)
> 
> 
> Review request for KDE Frameworks and Albert Astals Cid.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Application code which needs a newly installed desktop file to be visible,
> can either call this directly or use KBuildSycocaProgressDialog to get a 
> progress dialog.
> 
> REVIEW: 125308
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycoca.h a78e90876ed758093070989ab166959e0ea6c0e3 
> 
> Diff: https://git.reviewboard.kde.org/r/125308/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125325: New widget KCollapsibleGroupBox

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125325/#review85688
---


I like the concept.


src/kcollapsiblegroupbox.h (line 4)
<https://git.reviewboard.kde.org/r/125325/#comment59201>

trailing whitespace



src/kcollapsiblegroupbox.h (line 6)
<https://git.reviewboard.kde.org/r/125325/#comment59205>

widgetaddons is lgpl, not gpl



src/kcollapsiblegroupbox.h (line 12)
<https://git.reviewboard.kde.org/r/125325/#comment59202>

whitespace



src/kcollapsiblegroupbox.h (line 17)
<https://git.reviewboard.kde.org/r/125325/#comment59203>

whitespace



src/kcollapsiblegroupbox.h (line 20)
<https://git.reviewboard.kde.org/r/125325/#comment59204>

whitespace



src/kcollapsiblegroupbox.h (line 34)
<https://git.reviewboard.kde.org/r/125325/#comment59208>

Does it handle 1000 items, or is it up to the user to ensure to limit it or 
add it in a scrollarea?
Should it be documented?



src/kcollapsiblegroupbox.h (line 65)
<https://git.reviewboard.kde.org/r/125325/#comment59207>

Missing words? Or too many?



src/kcollapsiblegroupbox.h (line 100)
<https://git.reviewboard.kde.org/r/125325/#comment59206>

This construct actually EXPORT's the private class as well. The easy way is 
to move the class outside of the public class as KCollapsibleGroupBoxPrivate. 
The harder way is to ... build the right NOT_EXPORT macro


- Sune Vuorela


On Sept. 20, 2015, 1:48 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125325/
> ---
> 
> (Updated Sept. 20, 2015, 1:48 p.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> A groupbox featuring a clickable header and arrow indicator that can be
> expanded and collapsed to reveal the box content
> 
> Widget features a close and collapse animation and works as expected in
> QtDesigner.
> 
> --
> Screenshot explains what I mean better than the description above.
> 
> I've been given at least 3 mockups from the VDG mockup which feature using 
> this widget, clearly there's a demand for it. 
> 
> It's a bit like QToolBox, except no-one uses it because QToolbox has a weird 
> way of only expanding one at a time, and looks a bit weird so no-one uses it.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e03e9bbd6d73811873b0a465f86da269f4295138 
>   src/kcollapsiblegroupbox.h PRE-CREATION 
>   src/kcollapsiblegroupbox.cpp PRE-CREATION 
>   tests/CMakeLists.txt 180b0ef1f8900247da59112612ee87dd3164c8af 
>   tests/kcollapsiblegroupboxtest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125325/diff/
> 
> 
> Testing
> ---
> 
> Made little test app (see screenshot)
> 
> Wrote QtDesigner plugin, and played with it there.
> 
> 
> File Attachments
> 
> 
> kcollapsiblegroupbox.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/09/20/f9758e22-0043-4876-b462-364c3b2854dc__kcollapsiblegroupbox.png
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kcrash when started from terminal

[Sune Vuorela]

On 2015-09-24, Harald Sitter  wrote:
> Uh, ah, but, CMake is too smart :P
> If you add a target_link_library that isn't actually used it won't be
> linked. So what every application would have to do is add the target

It is not cmake that is too smart, but the linker when passed
--as-needed (which many distributions does)

$ cat foo.cpp 
int main(int, char**) { };

$ g++ -lQt5Core foo.cpp
$ objdump -x a.out | grep Qt5Core
  NEEDED   libQt5Core.so.5

$ g++ -Wl,--as-needed -lQt5Core foo.cpp
$ objdump -x a.out | grep Qt5Core


But bottom line, just adding some extra linkage doesn't get kept for
many compiled versions.


/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126610/#review90477
---


Looks much better than my attempt over in 
https://git.reviewboard.kde.org/r/120139

- Sune Vuorela


On Jan. 2, 2016, 7:24 p.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126610/
> ---
> 
> (Updated Jan. 2, 2016, 7:24 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kitemviews
> 
> 
> Description
> ---
> 
> When indexes are removed and their widgets deleted, the event filter on each 
> widget is not removed, leading to the "you should not delete widgets 
> manually"-alike warning.
> 
> Add an helper forgetAbout() function which performs all the actions done 
> per-widget before deleting each, additionally removing also the event filter.
> 
> 
> Diffs
> -
> 
>   src/kwidgetitemdelegate.cpp 779dc2a8a57148fb37f1f5a7194bc9656cb305a4 
>   src/kwidgetitemdelegatepool.cpp e916dddad8be56bb803e241da43d8cbe7a171ec3 
>   src/kwidgetitemdelegatepool_p.h 401fe193b0954d6c7c721503d4657b7f08e9fd2e 
> 
> Diff: https://git.reviewboard.kde.org/r/126610/diff/
> 
> 
> Testing
> ---
> 
> A sample application with widgets for items in the model, removing indexes: 
> no more warning at removal time.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120139: kill warning

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120139/
---

(Updated Jan. 2, 2016, 7:32 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, Andreas Hartmetz and Stephen Kelly.


Repository: kitemviews


Description
---

The warninng is even triggered by from internal KWidgetItemDelegatePrivate, so 
seems to be wrong

Try out the kwidgetitemdelegate test application


Diffs
-

  src/kwidgetitemdelegatepool.cpp d61802e 

Diff: https://git.reviewboard.kde.org/r/120139/diff/


Testing
---

Warning not printed.


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88556
---


without actualy knowing the code in question, a brief look over gives me the 
impression that the error handling could be improved and that would make this 
patch not needed.


src/engine/database.cpp (line 104)
<https://git.reviewboard.kde.org/r/126105/#comment60724>

SHouldn't there be a if(rc == 0) return false here ?



src/engine/database.cpp (line 137)
<https://git.reviewboard.kde.org/r/126105/#comment60725>

if(rc == 0) return false



src/engine/database.cpp (line 167)
<https://git.reviewboard.kde.org/r/126105/#comment60726>

if(rc == 0) return false


- Sune Vuorela


On Nov. 18, 2015, 7:40 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

[Sune Vuorela]

> On March 28, 2016, 5:14 p.m., Stephen Kelly wrote:
> > Do you still have the sample application you made to test/verify this? I'd 
> > like to try it and it should probably be committed too.
> 
> Pino Toscano wrote:
> No I don't :-/ I remember it was just removing indexes with associated 
> widgets.
> 
> Stephen Kelly wrote:
> Maybe you could try to make it again. I'm not familiar with this code but 
> I don't know any reason for adding this complexity.
> 
> I also don't consider myself the maintainer of this repo. I'm just 
> commenting that it doesn't look right to me.
> 
> David Edmundson wrote:
> sample application.
> 
> build system settings with branch davidedmundson/newdesign

a sample applicattion is the kwidgetiemdelegatetest application


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126610/#review94076
---


On March 28, 2016, 1:41 p.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126610/
> ---
> 
> (Updated March 28, 2016, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks, David Edmundson and Stephen Kelly.
> 
> 
> Repository: kitemviews
> 
> 
> Description
> ---
> 
> When indexes are removed and their widgets deleted, the event filter on each 
> widget is not removed, leading to the "you should not delete widgets 
> manually"-alike warning.
> 
> Add an helper forgetAbout() function which performs all the actions done 
> per-widget before deleting each, additionally removing also the event filter.
> 
> 
> Diffs
> -
> 
>   src/kwidgetitemdelegate.cpp 779dc2a8a57148fb37f1f5a7194bc9656cb305a4 
>   src/kwidgetitemdelegatepool.cpp e916dddad8be56bb803e241da43d8cbe7a171ec3 
>   src/kwidgetitemdelegatepool_p.h 401fe193b0954d6c7c721503d4657b7f08e9fd2e 
> 
> Diff: https://git.reviewboard.kde.org/r/126610/diff/
> 
> 
> Testing
> ---
> 
> A sample application with widgets for items in the model, removing indexes: 
> no more warning at removal time.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 128233: Don't trust files claiming to be created on unix more than other files

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128233/
---

Review request for KDE Frameworks and David Faure.


Repository: karchive


Description
---

Don't trust files claiming to be created on unix more than other files

For some historical reasons, we special case zip files claiming to be
created on unix and trust the content regarding file rights a bit better.

Zip files in the wild have shown to violate this, so don't trust them.

Thanks to Jonathan Marten for the test case

BUG: 364071


Diffs
-

  autotests/data/unusual_but_valid_364071.zip PRE-CREATION 
  autotests/karchivetest.h 4b7ecff 
  autotests/karchivetest.cpp c8abddf 
  src/kzip.cpp e7e8477 

Diff: https://git.reviewboard.kde.org/r/128233/diff/


Testing
---


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Releasing prison with next frameworks

[Sune Vuorela]

Hi peoples

After a final round of api changes in libprison, I think it is just a
version number bump away from being release d with next round of
framework releases, and thus going from the previous kdesupport area and
into a real framework.

What are the exact steps needed?

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 128237: add failing unit test likely showing off bug 232843

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128237/
---

Review request for KDE Frameworks and David Faure.


Repository: karchive


Description
---

Create unit test with expected failure to likely handle bug 232843


Diffs
-

  autotests/kfiltertest.h e03c0e1 
  autotests/kfiltertest.cpp a54eb4a 

Diff: https://git.reviewboard.kde.org/r/128237/diff/


Testing
---

test data missing because binary data. but can easily be recreated


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Is it able to make kservice's weightedOffers public access?

[Sune Vuorela]

On 2016-06-28, Leslie Zhai  wrote:
> Hi KDE developers,
>
> It might needs to use static KServiceTypeTrader::weightedOffers(const 
> QString ) to get KServiceOfferList offers, for example, 

I'll let David Faure (kservice maintainer) to comment on if this makes
sense or not. But if it does, then at least the KServiceOffer
documentation should also be made ready for public consumption.

/Sune

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128513: Unittest for content test of kdescendantsproxymodel

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128513/
---

(Updated July 24, 2016, 8:44 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure, Friedrich W. H. Kossebau, and 
Stephen Kelly.


Changes
---

Submitted with commit 3244f2ff34959f3e5c0a2c864a08737f47f96922 by Sune Vuorela 
to branch master.


Repository: kitemmodels


Description
---

A skipped unit test for https://git.reviewboard.kde.org/r/128398/

Note. the fix in that review does not seem to fix this test for me.


Diffs
-

  autotests/CMakeLists.txt dc8d779 
  autotests/kdescendantsproxymodeltest.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/128513/diff/


Testing
---


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128398: Fix KDescendantsProxyModel::setSourceModel(...) to reset internal data

[Sune Vuorela]

> On July 21, 2016, 5:43 a.m., Sune Vuorela wrote:
> > I tried writing a small test for it, and it seemed like I could reproduce 
> > the issue. But my test didn't pass when I added this changeset.
> > 
> > Maybe I should publish my test case somewhere...

https://git.reviewboard.kde.org/r/128513/ - published there


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128398/#review97685
---


On July 8, 2016, 2:58 a.m., Friedrich W. H. Kossebau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128398/
> ---
> 
> (Updated July 8, 2016, 2:58 a.m.)
> 
> 
> Review request for KDE Frameworks, Stephen  Kelly and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> KDescendantsProxyModel currently does not reset its internal data when a 
> (new) source model is set.
> 
> Not sure the provided patch is the most correct one, but it works with the 
> current unit tests and for the use case where this bug was hit.
> I am still confused why 
> `KDescendantsProxyModelPrivate::synchronousMappingRefresh()` loops over 
> `while (!m_pendingParents.isEmpty())` on calling `processPendingParents();` 
> while `KDescendantsProxyModelPrivate::scheduleProcessPendingParents()` does 
> not.
> Especially when the `KDescendantsProxyModelPrivate::sourceModelReset()` 
> handler also only calls the latter.
> The sourceModelReset handler should be surely similar to what is done on 
> setting a new source model, so the patch for now copies that code. But from 
> what I understood by reading the code both of them should rather do a full 
> loop perhaps?
> 
> And `m_relayouting` should get a better name now, but no idea yet what would 
> be nice.
> 
> I have yet to grasp the proxymodeltest system to also write a matching unit 
> test, any proposal where I should start?
> 
> 
> Diffs
> -
> 
>   src/kdescendantsproxymodel.cpp 477cd96 
> 
> Diff: https://git.reviewboard.kde.org/r/128398/diff/
> 
> 
> Testing
> ---
> 
> Existing kitemmodels unit tests still pass.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 128513: Unittest for content test of kdescendantsproxymodel

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128513/
---

Review request for KDE Frameworks, David Faure, Friedrich W. H. Kossebau, and 
Stephen Kelly.


Repository: kitemmodels


Description
---

A skipped unit test for https://git.reviewboard.kde.org/r/128398/

Note. the fix in that review does not seem to fix this test for me.


Diffs
-

  autotests/CMakeLists.txt dc8d779 
  autotests/kdescendantsproxymodeltest.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/128513/diff/


Testing
---


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128515: Fix KDescendantsProxyModel::setSourceModel() not clearing internal caches

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128515/#review97915
---


Ship it!




Ship It!

- Sune Vuorela


On July 24, 2016, 9:14 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128515/
> ---
> 
> (Updated July 24, 2016, 9:14 p.m.)
> 
> 
> Review request for KDE Frameworks, Friedrich W. H. Kossebau, Stephen Kelly, 
> and Sune Vuorela.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> This fixes Sune's unittest.
> 
> 
> Diffs
> -
> 
>   autotests/kdescendantsproxymodeltest.cpp 
> 67c0fba5bdcf700659889731f80043911af211fb 
>   src/kdescendantsproxymodel.cpp 477cd961e57bd8d8863f543aac1c7ac806bff24c 
> 
> Diff: https://git.reviewboard.kde.org/r/128515/diff/
> 
> 
> Testing
> ---
> 
> Just the unittest.
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128398: Fix KDescendantsProxyModel::setSourceModel(...) to reset internal data

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128398/#review97685
---



I tried writing a small test for it, and it seemed like I could reproduce the 
issue. But my test didn't pass when I added this changeset.

Maybe I should publish my test case somewhere...

- Sune Vuorela


On July 8, 2016, 2:58 a.m., Friedrich W. H. Kossebau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128398/
> ---
> 
> (Updated July 8, 2016, 2:58 a.m.)
> 
> 
> Review request for KDE Frameworks, Stephen  Kelly and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> KDescendantsProxyModel currently does not reset its internal data when a 
> (new) source model is set.
> 
> Not sure the provided patch is the most correct one, but it works with the 
> current unit tests and for the use case where this bug was hit.
> I am still confused why 
> `KDescendantsProxyModelPrivate::synchronousMappingRefresh()` loops over 
> `while (!m_pendingParents.isEmpty())` on calling `processPendingParents();` 
> while `KDescendantsProxyModelPrivate::scheduleProcessPendingParents()` does 
> not.
> Especially when the `KDescendantsProxyModelPrivate::sourceModelReset()` 
> handler also only calls the latter.
> The sourceModelReset handler should be surely similar to what is done on 
> setting a new source model, so the patch for now copies that code. But from 
> what I understood by reading the code both of them should rather do a full 
> loop perhaps?
> 
> And `m_relayouting` should get a better name now, but no idea yet what would 
> be nice.
> 
> I have yet to grasp the proxymodeltest system to also write a matching unit 
> test, any proposal where I should start?
> 
> 
> Diffs
> -
> 
>   src/kdescendantsproxymodel.cpp 477cd96 
> 
> Diff: https://git.reviewboard.kde.org/r/128398/diff/
> 
> 
> Testing
> ---
> 
> Existing kitemmodels unit tests still pass.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128464: Allow scrolling in config windows for small screens

[Sune Vuorela]

> On July 16, 2016, 1:06 a.m., Olivier Churlaud wrote:
> > Same patch should be applied to Konversation.

I think you need to submit i to konversation as well, then.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128464/#review97455
---


On July 16, 2016, 12:52 a.m., Olivier Churlaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128464/
> ---
> 
> (Updated July 16, 2016, 12:52 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 360260 and 362234
> https://bugs.kde.org/show_bug.cgi?id=360260
> https://bugs.kde.org/show_bug.cgi?id=362234
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Often I cannot reach the validation buttons of config windows. With this fix 
> I can.
> 
> The config page has now scroll bars when needed.
> 
> 
> Diffs
> -
> 
>   src/kconfigdialog.cpp 83c96b6 
> 
> Diff: https://git.reviewboard.kde.org/r/128464/diff/
> 
> 
> Testing
> ---
> 
> Compile and tested: Good behavior
> 
> 
> Thanks,
> 
> Olivier Churlaud
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128464: Allow scrolling in config windows for small screens

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128464/#review97467
---



looks good.

- Sune Vuorela


On July 16, 2016, 12:52 a.m., Olivier Churlaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128464/
> ---
> 
> (Updated July 16, 2016, 12:52 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 360260 and 362234
> https://bugs.kde.org/show_bug.cgi?id=360260
> https://bugs.kde.org/show_bug.cgi?id=362234
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Often I cannot reach the validation buttons of config windows. With this fix 
> I can.
> 
> The config page has now scroll bars when needed.
> 
> 
> Diffs
> -
> 
>   src/kconfigdialog.cpp 83c96b6 
> 
> Diff: https://git.reviewboard.kde.org/r/128464/diff/
> 
> 
> Testing
> ---
> 
> Compile and tested: Good behavior
> 
> 
> Thanks,
> 
> Olivier Churlaud
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128464: Allow scrolling in config windows for small screens

[Sune Vuorela]

> On July 16, 2016, 9:53 a.m., Sune Vuorela wrote:
> > looks good.
> 
> Olivier Churlaud wrote:
> Who should provide the ship it flag? I don't know who is the maintainer...

if no one else gives a more formal shipit by monday, consider this a shipit.


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128464/#review97467
---


On July 16, 2016, 10:02 a.m., Olivier Churlaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128464/
> ---
> 
> (Updated July 16, 2016, 10:02 a.m.)
> 
> 
> Review request for KDE Frameworks and Eike Hein.
> 
> 
> Bugs: 360260 and 362234
> https://bugs.kde.org/show_bug.cgi?id=360260
> https://bugs.kde.org/show_bug.cgi?id=362234
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Often I cannot reach the validation buttons of config windows. With this fix 
> I can.
> 
> The config page has now scroll bars when needed.
> 
> 
> Diffs
> -
> 
>   src/kconfigdialog.cpp 83c96b6 
> 
> Diff: https://git.reviewboard.kde.org/r/128464/diff/
> 
> 
> Testing
> ---
> 
> Compile and tested: Good behavior
> 
> 
> Thanks,
> 
> Olivier Churlaud
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128420: Name supported platforms in YAML file.

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128420/#review97259
---


Ship it!




Ship It!

- Sune Vuorela


On July 10, 2016, 10:30 a.m., Andreas Cord-Landwehr wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128420/
> ---
> 
> (Updated July 10, 2016, 10:30 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> There are ifdefs for the platforms Windows and MacOSX, besides Linux.
> Hence, it seems logical that all these three platforms are supported.
> 
> 
> Diffs
> -
> 
>   metainfo.yaml 397b3c168d274c0608efc8ba32409ac86d183472 
> 
> Diff: https://git.reviewboard.kde.org/r/128420/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Cord-Landwehr
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128353: Unit tests of directory permissions in zip files

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128353/
---

(Updated July 4, 2016, 6:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit dda62267aab8893db0ff695391d0fd8b37ace1f6 by Sune Vuorela 
to branch master.


Repository: karchive


Description
---

Add unit test to prevent later changes to abandon directory permissions in zip 
files

Test file created with
mkdir ziptest
cd ziptest
mkdir 700 750 755
chmod 700 700
chmod 750 750
chmod 755 755
zip -r my.zip 7*


Diffs
-

  autotests/karchivetest.h 5a6375c 
  autotests/karchivetest.cpp dd03dac 

Diff: https://git.reviewboard.kde.org/r/128353/diff/


Testing
---


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128353: Unit tests of directory permissions in zip files

[Sune Vuorela]

> On July 3, 2016, 8:25 p.m., David Faure wrote:
> > autotests/karchivetest.cpp, line 1118
> > <https://git.reviewboard.kde.org/r/128353/diff/1/?file=470950#file470950line1118>
> >
> > Why QLatin1String("") and not just QString() ?

Previous unit test does the same


- Sune


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128353/#review97066
-------


On July 4, 2016, 6:27 p.m., Sune Vuorela wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128353/
> ---
> 
> (Updated July 4, 2016, 6:27 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> Add unit test to prevent later changes to abandon directory permissions in 
> zip files
> 
> Test file created with
> mkdir ziptest
> cd ziptest
> mkdir 700 750 755
> chmod 700 700
> chmod 750 750
> chmod 755 755
> zip -r my.zip 7*
> 
> 
> Diffs
> -
> 
>   autotests/karchivetest.h 5a6375c 
>   autotests/karchivetest.cpp dd03dac 
> 
> Diff: https://git.reviewboard.kde.org/r/128353/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sune Vuorela
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128233: Don't trust files claiming to be created on unix more than other files

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128233/
---

(Updated July 4, 2016, 6:38 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit b7d8fce0a7167f4cea7127c6142ea10c92ff546d by Sune Vuorela 
to branch master.


Repository: karchive


Description
---

Don't trust files claiming to be created on unix more than other files

For some historical reasons, we special case zip files claiming to be
created on unix and trust the content regarding file rights a bit better.

Zip files in the wild have shown to violate this, so don't trust them.

Thanks to Jonathan Marten for the test case

BUG: 364071


Diffs
-

  autotests/data/unusual_but_valid_364071.zip PRE-CREATION 
  autotests/karchivetest.h 5a6375c 
  autotests/karchivetest.cpp dd03dac 
  src/kzip.cpp e7e8477 

Diff: https://git.reviewboard.kde.org/r/128233/diff/


Testing
---


Thanks,

Sune Vuorela

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel