<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106987/">http://git.reviewboard.kde.org/r/106987/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2012, 11:25 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Great.
I'm surprised that you say that these worked before:
Qt5, Phonon 4 on Qt4*
Qt5, Phonon 5 on Qt4*
With the code in the toplevel CMakeLists.txt, there was no way to use any phonon in KF5+Qt5. So I'm glad this is going in, so that all the code can be compiled with Qt5 (until now my qt5-build didn't build everything that my qt4-build was building).
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">With those setup, kdeframeworks would compile, but of course not link to libphonon.so (as all modules doing such linking would not be included due to the KDE_NO_PHONON hack).
Without phonon, kdeframeworks would not compile at all (due to a find_package(Phonon REQUIRED) in cmake/modules/FindKDE4Internal.cmake and an #include in knotify/config/knotifyconfigactionswidget.cpp). That was what got me started on Phonon in the first place, I didn't want to mix Qt4 and Qt5 headers on the same system...</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2012, 11:25 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106987/diff/2/?file=95977#file95977line6" style="color: black; font-weight: bold; text-decoration: underline;">interfaces/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">6</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">if (NOT KDE_NO_PHONON)</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">6</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">if (PHONON_FOUND)</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Shouldn't the find_package(Phonon) be done before this line, rather than inside interfaces/kmediaplayer?
I'm not sure yet how we'll split up this directory, to be honest. But since the toplevel kdelibs goes away for sure, a find_package is definitely needed here.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I assume interfaces/CMakeLists.txt will disappear together with the top-level CMakeLists.txt. I only added find_package(Phonon REQUIRED) to interfaces/kmediaplayer/CMakeLists.txt in order to make sure whoever eventually does the splitting remember it.
Please note that both interfaces/CMakeLists.txt and interfaces/kmediaplayer/CMakeLists.txt currently only work when called from the toplevel CMakeLists.txt, so this patch changes nothing in that regard.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2012, 11:25 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106987/diff/2/?file=95980#file95980line2" style="color: black; font-weight: bold; text-decoration: underline;">knotify/config/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">include_directories(BEFORE ${KDE4_PHONON_INCLUDES})</pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">include_directories(BEFORE ${KDE4_PHONON_INCLUDES})</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This seems to use the wrong variable, and too early (before the find_package(Phonon) that you added below.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yes, I didn't clean up existing stuff, as I thought such cleanup should be done when splitting. I only added find_package(Phonon) to knotify/config/CMakeLists.txt in order to make sure whoever eventually does the splitting remember it.
That said, doing just that cleanup is not too difficult, so I'll do it now instead.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2012, 11:25 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106987/diff/2/?file=95980#file95980line6" style="color: black; font-weight: bold; text-decoration: underline;">knotify/config/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">6</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> add_definitions(-DHAVE_PHONON=1)</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This will lead to compiler warnings/errors when phonon isn't found, because #if HAVE_PHONON will say "not defined!".
The proper way to do this is a configure_file for a knotify-config.h, with #cmakedefine01 in the knotify-config.h.cmake, like in many other places in kdelibs.
Or maybe Phonon is just required for this code.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It compiled, so obviously only a warning. I thought a configure_file was overkill for a single variable, but I'll add one anyway.</pre>
<br />
<p>- Jon</p>
<br />
<p>On November 23rd, 2012, 10:49 a.m., Jon Severinsson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Jon Severinsson.</div>
<p style="color: grey;"><i>Updated Nov. 23, 2012, 10:49 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"> Previously the Phonon headers and library was always required to build,
but when building with Qt5 the build system made sure never to link
against the library. Thus the build would work (with some missing
features) even if the only available phonon was built with Qt4. (Which
was the only possibility, as Phonon previously didn't build with Qt5).
With this patch, the Phonon headers and library is optional to build,
but if they are found they will be used. Thus building with Qt5 and a
Phonon built with Qt4 will no longer work. However, this patch allows
building with Phonon on Qt5, provided Phonon was built with Qt5.
In other words, these combinations are now supported (* are new with this patch):
Qt4, No Phonon*
Qt4, Phonon 4 on Qt4
Qt4, Phonon 5 on Qt4
Qt5, No Phonon*
Qt5, Phonon 5 on Qt5*
While these combinations are not supported (* previously (sort of) worked):
Qt4, Phonon 4 on Qt5
Qt4, Phonon 5 on Qt5
Qt5, Phonon 4 on Qt4*
Qt5, Phonon 5 on Qt4*
Qt5, Phonon 4 on Qt5
Please note that Phonon 5 requires the patch from review 106970 to
build on Qt5, and that "Qt5, Phonon 4 on Qt5" is fine as far as kdelibs
is concerned, but Phonon 4 won't actually build on Qt5.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(af6c3ec)</span></li>
<li>cmake/modules/FindKDE4Internal.cmake <span style="color: grey">(f99aaee)</span></li>
<li>interfaces/CMakeLists.txt <span style="color: grey">(d496d01)</span></li>
<li>interfaces/kmediaplayer/CMakeLists.txt <span style="color: grey">(4413131)</span></li>
<li>khtml/CMakeLists.txt <span style="color: grey">(db08b27)</span></li>
<li>knotify/config/CMakeLists.txt <span style="color: grey">(ae0933d)</span></li>
<li>knotify/config/knotifyconfigactionswidget.cpp <span style="color: grey">(de892fe)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106987/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>