<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102896/">http://git.reviewboard.kde.org/r/102896/</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, 2011, 6:04 p.m., <b>Dmitry Kazakov</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;">Hi!
It's a really cool idea to use uuids here! I think the patch is very good, but i have one small concern. Is it possible to remove old code like copyFromName(), setCopyFromName() and findNodeByName()? Probably, map the name to the uuid right during the loading in KisKraLoader or KisKraLoadVisitor. Is it possible? I think keeping this old code might play a negative role in a long-term plan =)</pre>
</blockquote>
<p>On October 16th, 2011, 7:46 p.m., <b>Torio Mlshi</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;">Hi!
I guess it is impossible to do this good way because during loading clone layer in KisKraLoader all we know is a target name. We even don't know is it created or still only present in xml. I can imagine some hacks which would allow to know uuid of that node (search for existing node with the same name, if found get uuid from it; if not, write to a map that node with required name should have special uuid), but they would be definetly worse is long-term plan.</pre>
</blockquote>
<p>On October 17th, 2011, 11:54 a.m., <b>Dmitry Kazakov</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;">Well, then you could leave KisKraLoader as it is but encapsulate the clone information into a separate object like:
CloneInfo {
public:
CloneInfo(const QUuid &uuid);
CloneInfo(const QString &name);
KisNodeSP findNode(KisNodeSP rootNode);
private:
QUuid m_uuid;
QString m_name;
};
And store this object in clone layers. This would remove all the duplications from the layer's code and remove findNodeByName/ByUuid duplicated cycles from the loader. The loader would create clone information from information it has in xml. What do you think about this solution? </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;">Yes, this looks better from design point of view (doubled cycle could be resolved easier, but such solution is more beautiful). I'll implement it (probably tomorrow). Is it ok to just push in master then?</pre>
<br />
<p>- Torio</p>
<br />
<p>On October 16th, 2011, 4:59 p.m., Torio Mlshi wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra and Boudewijn Rempt.</div>
<div>By Torio Mlshi.</div>
<p style="color: grey;"><i>Updated Oct. 16, 2011, 4:59 p.m.</i></p>
<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;">This patch fixes saving and loading of clone layers. It adds uuid to KisBaseNode so that clone layers use it to store information about their targets, making sure to target required layer.
About compatibility:
All nodes get uuid on creation. So node will have uuid even if no one is provided in file (which overwrites if present). Clone layers also use old code if no copyfromuuid field. So old files should work fine.
And new files should also be loaded ok by old versions because no fields are removed and old fields (such as copyfrom) are still valid. Of course, old versions will open new files just as old ones and bug will still present there.
</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;">Old files worked fine, as expected.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=283610">283610</a>
</div>
<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/kis_base_node.h <span style="color: grey">(798bb06)</span></li>
<li>krita/image/kis_base_node.cpp <span style="color: grey">(cccbee8)</span></li>
<li>krita/image/kis_clone_layer.h <span style="color: grey">(21e766e)</span></li>
<li>krita/image/kis_clone_layer.cpp <span style="color: grey">(050ef5f)</span></li>
<li>krita/ui/kra/kis_kra_load_visitor.h <span style="color: grey">(89bf501)</span></li>
<li>krita/ui/kra/kis_kra_load_visitor.cpp <span style="color: grey">(d980ab0)</span></li>
<li>krita/ui/kra/kis_kra_loader.cpp <span style="color: grey">(5d3e5d5)</span></li>
<li>krita/ui/kra/kis_kra_savexml_visitor.cpp <span style="color: grey">(ecee329)</span></li>
<li>krita/ui/kra/kis_kra_tags.h <span style="color: grey">(a737ae3)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102896/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>