<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118763/">https://git.reviewboard.kde.org/r/118763/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 16th, 2014, 6:57 a.m. UTC, <b>Martin Gräßlin</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;">looks good to me, +1. Please add Hugo Pereira Da Costa to the Review Request, though.
The review request made me wonder whether we still need to find XLib in Oxygen, though. The parts shown only use XCB, so maybe we could just go for finding only XCB?</pre>
</blockquote>
<p>On June 26th, 2014, 11:49 a.m. UTC, <b>Hugo Pereira Da Costa</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;">@Martin,
yes you are probably right. X11 should not be necessary any more.
I'll double check and commit. (have other stuff pending).
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Martin, I think you are correct.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
However, in the top CMakeLists I see:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">find_package(X11)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
if(X11_FOUND)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
find_package(XCB REQUIRED COMPONENTS XCB)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
set_package_properties(XCB PROPERTIES TYPE REQUIRED)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">find_package(Qt5 REQUIRED CONFIG COMPONENTS X11Extras)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
else()<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
set(X11_FOUND 0)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
endif()</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How would I deal with that ? (the X11_FOUND part), if I don't look for X11 in the first place ? <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Is there a XCB_FOUND set by find_packgage ? <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Should I make XCB optional (as X11 is) ? and then replace all instances of X11_FOUND by XCB_FOUND ? <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
If this is the right way, I'll do and commit</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks in advance</p></pre>
<br />
<p>- Hugo</p>
<br />
<p>On August 22nd, 2014, 3:31 p.m. UTC, Bernd Steinhauser wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for kde-workspace, Plasma and Hugo Pereira Da Costa.</div>
<div>By Bernd Steinhauser.</div>
<p style="color: grey;"><i>Updated Aug. 22, 2014, 3:31 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
oxygen
</div>
<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;">No idea if kde-workspace is still the right group, if not, please change.
find_package(XCB) is called without specifying the required components. This leads to linking to unused dependencies in case they are installed.
Since XCB is searched for in the top level cmake file in the repository, there is no need to search for it again. The component required there (only base XCB) is sufficient.
Although, this should be sufficient to fix the deps problem, it makes sense to link to XCB::XCB instead of ${XCB_LIBRARIES}, since the former is what is actually needed.</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>kstyle/CMakeLists.txt <span style="color: grey">(165b62a)</span></li>
<li>liboxygen/CMakeLists.txt <span style="color: grey">(0d1dd94)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118763/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>