<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/125649/">https://git.reviewboard.kde.org/r/125649/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 16th, 2015, 12:30 a.m. CEST, <b>Friedrich W. H. Kossebau</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;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Q_WS_MAC</code> is not a cmake var, or? Seems someone accidentally used the c++ macro/define here, and noone ever noticed? :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In Qt5/KF5/ECM worlds ecm_add_app_icon does the respective installation, right? So only the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Q_WS_MAC</code> fix will need porting to the master branch?</p></pre>
</blockquote>
<p>On October 16th, 2015, 1:37 a.m. CEST, <b>René J.V. Bertin</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;">No, Q_WS_MAC is not something that cmake provides by default, so I guess you're right. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do calligragemini and calligraauthor get icons on Linux? For the former it seems that that should only be the case if the icon .png file(s) is/are already installed, but if that's the intended behaviour shouldn't my patch be Mac-specific? Calligraauthor's <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kde4_add_app_icon</code> call is commented out, was that intended or should that patch <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> be Mac-specific?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have no idea at the moment what has changed in KF5/ECM. If ECM provides a function <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_add_app_icon</code> then it would make sense that it does all the required work.
Note however that the sole use of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kde4_add_app_icon</code> isn't enough. It generates the icon file, and it adds the corresponding entry in the application's <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Info.plist</code>, but that's not always enough. I haven't yet figured out why certain applications always got an icon, and why others require explicit installation of the icon resource.</p></pre>
</blockquote>
<p>On October 16th, 2015, 2 a.m. CEST, <b>Friedrich W. H. Kossebau</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;">Yes, calligragemini has icons on linux, they are installed by the CMakeLists.txt in gemini/pics. And Author has icons as well.
For both it was "just" those kde4_add_app_icon that were broken or not active (they do nothing on linux IIRC, that's why not noone noticed so far. And Windows builds are only done for CalligraGemini, Krita & Kexi).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_add_app_icon</code> see here for the sources, ll. 223-224:
https://quickgit.kde.org/?p=extra-cmake-modules.git&a=blob&f=modules%2FECMAddAppIcon.cmake</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"> # Install the icon into the Resources dir in the bundle
set_source_files_properties(<span style="color: #BC7A00">${</span>_outfilename<span style="color: #BC7A00">}</span>.icns PROPERTIES MACOSX_PACKAGE_LOCATION Resources)
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No idea what effect that has and if that is better than what is there in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kde4_add_app_icon</code>, but you might :)</p></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;">No, sorry I don't. Not without delving into the cmake code. I can guess though: it installs <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">${_outfilename}.icns</code> into the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Resources</code> directory of the app bundle created at <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MACOSX_PACKAGE_LOCATION</code>. It's a bit suspicious that that variable (or token) uses the term <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">package</code> instead of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">bundle</code> (but I've learned it's useless to argue with cmake developers about such things).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Two things are required for providing an app bundle with an icon. The icon resource itself (.icns) which is expected in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">foo.app/Contents/Resources</code>, and an entry in the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Info.plist</code> which declares the name of that resource. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kde4_add_app_icon</code> appears to guarantee only the former requirement; <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_add_app_icon</code> may be better behaved. However, looking at the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kde4_add_app_icon</code> source:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"> set(MACOSX_BUNDLE_ICON_FILE <span style="color: #BC7A00">${</span>appsources<span style="color: #BC7A00">}</span>.icns)
# Append the icns file to the sources list so it will be a dependency to the
# main target
list(APPEND <span style="color: #BC7A00">${</span>appsources<span style="color: #BC7A00">}</span> <span style="color: #BC7A00">${</span>_outfilename<span style="color: #BC7A00">}</span>.icns)
# Install the icon into the Resources dir in the bundle
set_source_files_properties(<span style="color: #BC7A00">${</span>_outfilename<span style="color: #BC7A00">}</span>.icns PROPERTIES MACOSX_PACKAGE_LOCATION Resources)
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The first line tackles the Info.plist entry, and the last line is identical to the one from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_add_app_icon</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I was going to suggest that someone more intimate with KDE's cmake build system could port the 2.9x CMake files to use ECM (ECM doesn't depend on KF5 and AFAIK it can be used with KDE4 too). However, it seems this won't change anything.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I do think that some applications didn't get icons because they were declared as kdeinit applications, but that doesn't appear to apply to all of them.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In short, it seems that for calligra 3x
- my modifications to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kde4_add_app_icon</code> calls can be applied to the corresponding <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_add_app_icon</code> calls
- the explicit install instructions can be copied to the corresponding locations, with syntax or name corrections where required, and then commented out so that the fixes I made here won't get lost or forgotten.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Re: <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Q_WS_MAC</code> : it appears that this token is defined by <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">FindQt4.cmake</code> (file provided by kdelibs). To token is deprecated in Qt, so it's best not to use it.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On October 15th, 2015, 10:55 p.m. CEST, René J.V. Bertin 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 Calligra and KDE Software on Mac OS X.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Oct. 15, 2015, 10:55 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</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;">Builds on OS X currently generate icons for most Calligra applications, but those are installed only for a happy few (Krita, Braindump and Kexi). The other applications require an explicit install command of the generated <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.icns</code> file into the app bundle's Resources directory.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The attached patch takes care of that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In addition, it corrects the picture source directories for calligragemini and calligraauthor so those applications can have icons on other platforms too, and replaces the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Q_WS_MACOS</code> token with the (IMHO) more appropriate <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">APPLE</code> token.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">On OS X 10.9 with KDELibs 4.14.12 and MacPorts 2.3.4 .</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>flow/part/CMakeLists.txt <span style="color: grey">(58882f1)</span></li>
<li>gemini/CMakeLists.txt <span style="color: grey">(85123fa)</span></li>
<li>karbon/CMakeLists.txt <span style="color: grey">(b574779)</span></li>
<li>plan/CMakeLists.txt <span style="color: grey">(ad39f57)</span></li>
<li>sheets/CMakeLists.txt <span style="color: grey">(b0cc134)</span></li>
<li>stage/app/CMakeLists.txt <span style="color: grey">(079bece)</span></li>
<li>words/app/CMakeLists.txt <span style="color: grey">(1e73971)</span></li>
<li>words/part/CMakeLists.txt <span style="color: grey">(9143176)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125649/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>