<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/122678/">https://git.reviewboard.kde.org/r/122678/</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;">I think we should ship this, immediately after the next 2.9 release so there's plenty of testing time.</p></pre>
<br />
<p>- Boudewijn Rempt</p>
<br />
<p>On July 29th, 2015, 6:20 p.m. UTC, Stefano Bonicatti 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.</div>
<div>By Stefano Bonicatti.</div>
<p style="color: grey;"><i>Updated July 29, 2015, 6:20 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;">Why is needed:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The hash of a resource is sometimes calculated on the file, sometimes on the interpreted (loaded) data which can vary between users or due to approximations, resulting in the md5 inside the bundle
to be different from the recalculated one.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What it does:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The md5 will be always calculated on the file and not on the interpreted data.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KoResource::generateMD5 is not pure virtual anymore, because all the resources has to share the same md5 generation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KoResource::saveToDevice is not pure virtual anymore, because we need to set the md5 to empty each time the resource is saved, so that the next access to the md5 forces a recalculation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KoResource::saveToDevice must be called from the saveToDevice functions of the inheriting classes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">saveResourceToStore has been modified so that it doesn't try to save a resource when a copy cannot be done, when creating a bundle.
Also if the file is writable it should be still copied.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What still needs to be done:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unfortunately i discovered too late that this fixes only a small portion of issues, simply because when trying to generate a md5 from a file in a bundle, that filename or filepath isn't one that points to an existing file on the disk, but it's a path that has to be interpreted and the bundle has to be opened.
The idea to fix this is to override the generateMD5 function of all the bundle resource types and have special code to deal with the path (like KisPaintOpPreset::load function does), so that a temporary resource store is created to read the bundle file, otherwise just call KoResource::generateMD5().
The problem is that there are resource types as KoColorSet which i'm not completely sure they should know anything about bundles.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Even with that fixed there are resource types that cannot go into a bundle, but where i still removed their generateMD5 implementation, so that the base KoResource implementation is used. These needs to be tested, to see if i didn't introduce a regression.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This commit 8a19e6a469ebe784b038b802c346a80fdbd89b0d conflicts with my patch and the above reasoning, because it simply prevent bundle files md5 to be generated.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've changed a bit how saveToDevice works, so that it empties the resource md5 everytime it's saved. This needs to be tested (if the md5, when acquired, is regenerated) and it should be checked if every possible resource has the call to KoResource::saveToDevice, otherwise it should be added.</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;">Compiles.
The md5 inside the bundle manifests are correct, even for gradients (where normally the wrong one was used, since they're interpreted).</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/image/brushengine/kis_paintop_preset.h <span style="color: grey">(52caacc)</span></li>
<li>krita/image/brushengine/kis_paintop_preset.cpp <span style="color: grey">(736699d)</span></li>
<li>krita/libbrush/kis_abr_brush.cpp <span style="color: grey">(ad717d4)</span></li>
<li>krita/libbrush/kis_abr_brush_collection.cpp <span style="color: grey">(554865b)</span></li>
<li>krita/libbrush/kis_auto_brush.h <span style="color: grey">(c537697)</span></li>
<li>krita/libbrush/kis_auto_brush.cpp <span style="color: grey">(86cc379)</span></li>
<li>krita/libbrush/kis_brush.h <span style="color: grey">(1a1bf5c)</span></li>
<li>krita/libbrush/kis_brush.cpp <span style="color: grey">(ea389ee)</span></li>
<li>krita/libbrush/kis_gbr_brush.cpp <span style="color: grey">(37158f9)</span></li>
<li>krita/libbrush/kis_imagepipe_brush.cpp <span style="color: grey">(4d301cb)</span></li>
<li>krita/libbrush/kis_png_brush.cpp <span style="color: grey">(358f8da)</span></li>
<li>krita/libbrush/kis_svg_brush.cpp <span style="color: grey">(2934a36)</span></li>
<li>krita/plugins/extensions/dockers/tasksetdocker/taskset_resource.h <span style="color: grey">(57ad287)</span></li>
<li>krita/plugins/extensions/dockers/tasksetdocker/taskset_resource.cpp <span style="color: grey">(6afc96f)</span></li>
<li>krita/plugins/extensions/resourcemanager/resourcebundle.h <span style="color: grey">(7e3c8b9)</span></li>
<li>krita/plugins/extensions/resourcemanager/resourcebundle.cpp <span style="color: grey">(ba535d3)</span></li>
<li>krita/ui/CMakeLists.txt <span style="color: grey">(195d9b5)</span></li>
<li>krita/ui/kis_factory2.cc <span style="color: grey">(6e2fd9d)</span></li>
<li>krita/ui/kis_md5_generator.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>krita/ui/kis_md5_generator.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>krita/ui/kis_workspace_resource.h <span style="color: grey">(aefc57c)</span></li>
<li>krita/ui/kis_workspace_resource.cpp <span style="color: grey">(b71dc49)</span></li>
<li>libs/pigment/CMakeLists.txt <span style="color: grey">(4f54815)</span></li>
<li>libs/pigment/resources/KoAbstractGradient.h <span style="color: grey">(167be69)</span></li>
<li>libs/pigment/resources/KoAbstractGradient.cpp <span style="color: grey">(800a490)</span></li>
<li>libs/pigment/resources/KoColorSet.h <span style="color: grey">(ffd81b8)</span></li>
<li>libs/pigment/resources/KoColorSet.cpp <span style="color: grey">(47d3c6b)</span></li>
<li>libs/pigment/resources/KoHashGenerator.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/pigment/resources/KoHashGeneratorProvider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/pigment/resources/KoHashGeneratorProvider.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/pigment/resources/KoMD5Generator.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/pigment/resources/KoMD5Generator.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/pigment/resources/KoPattern.h <span style="color: grey">(31acba4)</span></li>
<li>libs/pigment/resources/KoPattern.cpp <span style="color: grey">(1c1f521)</span></li>
<li>libs/pigment/resources/KoResource.h <span style="color: grey">(53a18e2)</span></li>
<li>libs/pigment/resources/KoResource.cpp <span style="color: grey">(2b568bf)</span></li>
<li>libs/pigment/resources/KoSegmentGradient.h <span style="color: grey">(bdd6baa)</span></li>
<li>libs/pigment/resources/KoSegmentGradient.cpp <span style="color: grey">(5387743)</span></li>
<li>libs/pigment/resources/KoStopGradient.h <span style="color: grey">(2e1e3d4)</span></li>
<li>libs/pigment/resources/KoStopGradient.cpp <span style="color: grey">(dc851da)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122678/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>