<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/123331/">https://git.reviewboard.kde.org/r/123331/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<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, two bugs... The first, with the '0' adding path to /usr/krita, that's a copy and paste error, and the weird line in LcmsEnginePlugin was added when Srikanth added GHNS support for profiles. It's totally wrong... As long as Krita also keeps looking in data krita/profiles, everything should be fine.</p></pre>
<br />
<p>- Boudewijn Rempt</p>
<br />
<p>On April 11th, 2015, 1:38 p.m. UTC, Friedrich W. H. Kossebau 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, Cyrille Berger Skott, Boudewijn Rempt, and Srikanth Tiyyagura.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated April 11, 2015, 1:38 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;">While getting an overview over all the KStandardDirs usage in Calligra, to get an idea what is needed with KF5 where there is a bigger API/functionality breakage, I saw some issues with how ICC profiles are searched for.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Let's see if I have got things correctly:
ICC profiles are installed by different packages in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">*/share/color/icc/*</code>. E.g. on my system there are number of different folders below <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">/usr/share/color/icc/</code>.
That is why the LcmsEnginePlugin calls</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%"> <span style="color: #008000; font-weight: bold">KGlobal</span><span style="color: #666666">:</span><span style="color: #AA22FF">:mainComponent</span><span style="color: #666666">()</span><span style="color: #0000FF; font-weight: bold">.dirs</span><span style="color: #666666">()</span><span style="color: #008000; font-weight: bold">-</span><span style="color: #666666">></span><span style="color: #008000; font-weight: bold">addResourceType</span><span style="color: #666666">(</span><span style="color: #BA2121">"icc_profiles"</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">0</span><span style="color: #666666">,</span> <span style="color: #BA2121">"share/color/icc/"</span><span style="color: #666666">);</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">to get any such paths below the usual system prefixes (second argument <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">0</code> means immediate concatenation, without further grouping prefixes).
Krita itselfs installs all its own set of profiles in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">${CMAKE_INSTALL_PREFIX}/share/color/icc/krita</code>.
Krita's ColorSettingsTab::installProfile() supports installation of additional ICC profiles, just to be used by Krita. It searches for the location to save the imported file with</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%"> <span style="color: #008000; font-weight: bold">QString</span> <span style="color: #008000; font-weight: bold">saveLocation</span> <span style="color: #666666">=</span> <span style="color: #008000; font-weight: bold">KGlobal</span><span style="color: #666666">:</span><span style="color: #AA22FF">:mainComponent</span><span style="color: #666666">()</span><span style="color: #0000FF; font-weight: bold">.dirs</span><span style="color: #666666">()</span><span style="color: #008000; font-weight: bold">-</span><span style="color: #666666">></span><span style="color: #008000; font-weight: bold">saveLocation</span><span style="color: #666666">(</span><span style="color: #BA2121">"icc_profiles"</span><span style="color: #666666">);</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">which will give the first path in the list of prefixes + registered path suffix which can be written to. Which for me is <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">$KDEHOME/share/color/icc</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So far things seem in order.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But then there are two calls that confuse me:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">First is in KisFactory::componentData():</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%"> s_componentData->dirs()->addResourceType("icc_profiles", 0, "krita/profiles/");
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">which will resolve to e.g. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">/usr/krita/profiles/</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">$KDEHOME/krita/profiles/</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Two questions:
a) Is that path resolution really wanted? Or should that have been done with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">"data"</code> as second argument, so it would be resolved to e.g. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">/usr/share/apps/krita/profiles/</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">$KDEHOME/share/apps/krita/profiles/</code>?
b) why that extra path, who is expected to put ICC files also in such folders?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Second issue is in LcmsEnginePlugin constructor:</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%"> <span style="color: #008000; font-weight: bold">KGlobal</span><span style="color: #666666">:</span><span style="color: #AA22FF">:mainComponent</span><span style="color: #666666">()</span><span style="color: #0000FF; font-weight: bold">.dirs</span><span style="color: #666666">()</span><span style="color: #008000; font-weight: bold">-</span><span style="color: #666666">></span><span style="color: #008000; font-weight: bold">addResourceDir</span><span style="color: #666666">(</span><span style="color: #BA2121">"icc_profiles"</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">QDir</span><span style="color: #666666">:</span><span style="color: #AA22FF">:homePath</span><span style="color: #666666">()</span> <span style="color: #666666">+</span> <span style="color: #008000; font-weight: bold">QString</span><span style="color: #666666">(</span><span style="color: #BA2121">"/.kde/share/apps/krita/profiles/"</span><span style="color: #666666">));</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hardcoding <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.kde</code> will not work on systems where the user data is stored under a different folder, after all that is configurable and possibly even platform-specific. Also giving a full path should not be needed.
This call seems to be a workaround for the first issue, as somebody has expected that ICC files in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">$KDEHOME/share/apps/krita/profiles/</code> will be found.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So I propose to clean this up, by removing the code from LcmsEnginePlugin for registering the hardcoded path to the user's profile files.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And to either fix the registration of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">krita/profiles/</code> or perhaps to even completely remove it (which I favour, unless there is a real need).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">WRT to backwards compatibility, as <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">$KDEHOME/share/color/icc</code> seems to be picked first for saving ICC files via <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">saveLocation()</code>, removing both questionable resource path registrations should then only affect people who installed any files manually in the wrong <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">krita/profiles/</code> dirs or that hardcoded dir. Which I guess are not that many. And if they are, they surely are also able to fix it for them.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So for clean code and not collecting too much someone-on-the-earth-might-be-unhappy-to-have-to-adapt boilerplate I would remove this all, and make a comment in the release notes :)
But not my call, so asking you as maintainer :)
If you want to keep it, I will add at least some "this is broken, remove in a sane moment" comment explaining what brokeness we have here.</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>krita/ui/kis_factory2.cc <span style="color: grey">(2f5ba41)</span></li>
<li>plugins/colorengines/lcms2/LcmsEnginePlugin.cpp <span style="color: grey">(c73ecf6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123331/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>