'Re: Review Request 122678: [WIP] KoResource and Krita bundles MD5 generation fix'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       calligra-devel
Subject:    Re: Review Request 122678: [WIP] KoResource and Krita bundles MD5 generation fix
From:       "Boudewijn Rempt" <boud () valdyas ! org>
Date:       2015-08-09 18:28:58
Message-ID: 20150809182858.13071.16491 () mimi ! kde ! org
[Download RAW message or body]

--===============6751833343320716007==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122678/#review83622
-----------------------------------------------------------

Ship it!

Ship It!

- Boudewijn Rempt

On Aug. 9, 2015, 5:41 p.m., Stefano Bonicatti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122678/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2015, 5:41 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> Why is needed:
> 
> 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.
> 
> 
> What it does:
> 
> The md5 will be always calculated on the file and not on the interpreted data.
> 
> KoResource::generateMD5 is not pure virtual anymore, because all the resources has \
> to share the same md5 generation. 
> 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. 
> KoResource::saveToDevice must be called from the saveToDevice functions of the \
> inheriting classes. 
> 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. 
> 
> What still needs to be done:
> 
> 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. 
> 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. 
> This commit 8a19e6a469ebe784b038b802c346a80fdbd89b0d conflicts with my patch and \
> the above reasoning, because it simply prevent bundle files md5 to be generated. 
> 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. 
> 
> Diffs
> -----
> 
> krita/image/brushengine/kis_paintop_preset.h 52caacc 
> krita/image/brushengine/kis_paintop_preset.cpp 736699d 
> krita/libbrush/kis_abr_brush.cpp ad717d4 
> krita/libbrush/kis_abr_brush_collection.cpp 554865b 
> krita/libbrush/kis_auto_brush.h c537697 
> krita/libbrush/kis_auto_brush.cpp 86cc379 
> krita/libbrush/kis_brush.h 6faf597 
> krita/libbrush/kis_brush.cpp efa78a4 
> krita/libbrush/kis_gbr_brush.cpp 37158f9 
> krita/libbrush/kis_imagepipe_brush.cpp 9cffa37 
> krita/libbrush/kis_png_brush.cpp 358f8da 
> krita/libbrush/kis_svg_brush.cpp 2934a36 
> krita/plugins/extensions/dockers/tasksetdocker/taskset_resource.h 57ad287 
> krita/plugins/extensions/dockers/tasksetdocker/taskset_resource.cpp 6afc96f 
> krita/plugins/extensions/resourcemanager/resourcebundle.h 7e3c8b9 
> krita/plugins/extensions/resourcemanager/resourcebundle.cpp ba535d3 
> krita/plugins/extensions/resourcemanager/resourcebundle_manifest.h e4de7a2 
> krita/plugins/extensions/resourcemanager/resourcebundle_manifest.cpp b47c10a 
> krita/ui/CMakeLists.txt 195d9b5 
> krita/ui/kis_factory2.cc 6e2fd9d 
> krita/ui/kis_md5_generator.h PRE-CREATION 
> krita/ui/kis_md5_generator.cpp PRE-CREATION 
> krita/ui/kis_workspace_resource.h aefc57c 
> krita/ui/kis_workspace_resource.cpp b71dc49 
> libs/pigment/CMakeLists.txt 4f54815 
> libs/pigment/resources/KoAbstractGradient.h 167be69 
> libs/pigment/resources/KoAbstractGradient.cpp 800a490 
> libs/pigment/resources/KoColorSet.h ffd81b8 
> libs/pigment/resources/KoColorSet.cpp 47d3c6b 
> libs/pigment/resources/KoHashGenerator.h PRE-CREATION 
> libs/pigment/resources/KoHashGeneratorProvider.h PRE-CREATION 
> libs/pigment/resources/KoHashGeneratorProvider.cpp PRE-CREATION 
> libs/pigment/resources/KoMD5Generator.h PRE-CREATION 
> libs/pigment/resources/KoMD5Generator.cpp PRE-CREATION 
> libs/pigment/resources/KoPattern.h 31acba4 
> libs/pigment/resources/KoPattern.cpp 1c1f521 
> libs/pigment/resources/KoResource.h 53a18e2 
> libs/pigment/resources/KoResource.cpp 2b568bf 
> libs/pigment/resources/KoSegmentGradient.h bdd6baa 
> libs/pigment/resources/KoSegmentGradient.cpp 5387743 
> libs/pigment/resources/KoStopGradient.h 2e1e3d4 
> libs/pigment/resources/KoStopGradient.cpp dc851da 
> 
> Diff: https://git.reviewboard.kde.org/r/122678/diff/
> 
> 
> Testing
> -------
> 
> Compiles.
> The md5 inside the bundle manifests are correct, even for gradients (where normally \
> the wrong one was used, since they're interpreted). 
> 
> Thanks,
> 
> Stefano Bonicatti
> 
> 

--===============6751833343320716007==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit

<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;">Ship It!</pre>  <br />

<p>- Boudewijn Rempt</p>

<br />
<p>On August 9th, 2015, 5:41 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 Aug. 9, 2015, 5:41 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">(6faf597)</span></li>

 <li>krita/libbrush/kis_brush.cpp <span style="color: grey">(efa78a4)</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">(9cffa37)</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/plugins/extensions/resourcemanager/resourcebundle_manifest.h <span \
style="color: grey">(e4de7a2)</span></li>

 <li>krita/plugins/extensions/resourcemanager/resourcebundle_manifest.cpp <span \
style="color: grey">(b47c10a)</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>

--===============6751833343320716007==--

[Attachment #3 (text/plain)]

_______________________________________________
calligra-devel mailing list
calligra-devel@kde.org
https://mail.kde.org/mailman/listinfo/calligra-devel

[prev in list] [next in list] [prev in thread] [next in thread] 