<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/121448/">https://git.reviewboard.kde.org/r/121448/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 12th, 2014, 2:08 p.m. UTC, <b>Alex Merry</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="https://git.reviewboard.kde.org/r/121448/diff/1/?file=332663#file332663line15" style="color: black; font-weight: bold; text-decoration: underline;">modules/ECMAddAppIcon.cmake</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">15</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c"># example: ecm_app_app_icon(myapp_SRCS "pics/cr16-myapp.png;pics/cr32-myapp.png")</span></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Would this actually work? The code looks to me like pattern_rx would just be the filename again, so fn would be empty, and the icon would never be appended to _list.</p></pre>
</blockquote>
<p>On December 15th, 2014, 7:39 a.m. UTC, <b>Ralf Habacker</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The doc in the origin cmake macro file seems to be out of sync with the official doc located in FindKDE4Internals.cmake, which is: </p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">adds an application icon to target source list.</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Make sure you have a 128x128 icon, or the icon won't display on Mac OS X.</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Mac OSX notes : the application icon is added to a Mac OS X bundle so that Finder and friends show the right thing.</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Win32 notes: the application icon(s) are compiled into the application</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">There is some workaround in kde4_add_kdeinit_executable to make it possible for those applications as well.</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Parameters:</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">SRCS_VAR - specifies the list of source files</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">pattern - regular expression for searching application icons</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Example: KDE4_ADD_APP_ICON( myapp_SOURCES "pics/cr*-myapp.png")</h1>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Example: KDE4_ADD_APP_ICON( myapp_KDEINIT_SRCS "icons/oxygen/*/apps/myapp.png")</h1>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">normal file mode is not documentated and does not work</p></pre>
</blockquote>
<p>On December 15th, 2014, 8:16 p.m. UTC, <b>Alex Merry</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would still much prefer an API to match ecm_add_icons, as I noted at the start of this review. Would you be willing to do that? If not, I can probably find some time over Christmas to look at it.</p></pre>
</blockquote>
<p>On December 18th, 2014, 1:59 p.m. UTC, <b>Ralf Habacker</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Just one question: As it turns out that the macro used in KDE4 only supports a "simple pattern" style and no single files, what is different or has been extended with KDE5 to require this now ?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Best CMake practices include listing all source files in the CMakeLists.txt file, rather than globbing them. That way, CMake knows when a file has been added (or removed), because the CMakeLists.txt file will change, and so it will automatically reconfigure when you run <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">make</code>. The icon installation function (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_add_icons</code>) was changed to support and encourage that approach, and I think this function should also change to match it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Additionally, I would like consistency between interfaces. Having ecm_add_app_icon behave completely differently to ecm_add_icons just makes things that much harder for users of e-c-m. They should use the same file naming scheme (or compatible schemes, at least), and similar arguments, in so far as they need similar information.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll admit it makes porting a little harder, but it should be possible to create a porting script without too much difficulty.</p></pre>
<br />
<p>- Alex</p>
<br />
<p>On December 15th, 2014, 8:01 a.m. UTC, Ralf Habacker 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 Extra Cmake Modules, KDE Frameworks and Laurent Navet.</div>
<div>By Ralf Habacker.</div>
<p style="color: grey;"><i>Updated Dec. 15, 2014, 8:01 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
extra-cmake-modules
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This module, which has been migrated from the related KDE4 macto kde4_app_app_icon,
supports platform specific application icon for Windows and Mac OSX.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On Windows this function depends on the external tool png2ico, which is
provided by the kdewin-tools binary package. Sources are available at
https://projects.kde.org/projects/kdesupport/kdewin.</p></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>modules/ECMAddAppIcon.cmake <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121448/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/12/12/a05ee2b5-64e3-4e44-ae34-4e1b7110e5f1__ECMAddAppIcon.cmake">ECMAddAppIcon.cmake</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/12/15/8b3e226f-a70b-4998-983a-813730a436bf__ECMAddAppIcon.cmake">ECMAddAppIcon.cmake</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/12/15/8433995f-b88f-426d-af54-46aba635ae1e__ECMAddAppIcon.cmake">ECMAddAppIcon.cmake</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>