<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 />
<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>
<br />
<div>
<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="margin-left: 2em; 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>
</div>
<br />
<div>
<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="margin-left: 2em; 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>
</div>
<br />
<div>
<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="margin-left: 2em; 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>
</div>
<br />
<p>- David</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>