<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/124048/">https://git.reviewboard.kde.org/r/124048/</a>
</td>
</tr>
</table>
<br />
<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 June 30, 2015, 5:38 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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 should be the final version, buttons have been relocated into the KoResourceItemChooser, where is now possible to add up to two custom buttons (this is hardcoded because not hardcoding it would have been tricky.. and right now this is the only use case).</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Summary (updated)</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;">Krita - Brush editor Stamp and Clipboard refactoring</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=345195">345195</a>
</div>
<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;">The Stamp and Clipboard tabs are removed and they become buttons in the Predefined tab, so that the user must add a predefined brush tip to use it first.
This solves some issues and inconsistencies with temporary brush tips by removing the need of them.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Fixes a crash that happened when selecting a temporary brush after adding the respective clone to the predefined or when deleting the cloned brush.
The crash happened only with a Stamp Animated one.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Fixes a crash when attempting to add a Clipboard brush tip when the clipboard is empty.
Now the OK button will be disabled if the clipboard is emmpty.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This review is here because of the placing of the buttons, their text and tooltips.
David Revoy suggested to put "long" (but that fit on the button) text on them instead of icons and to write a tooltip to explain what they do.
I wanted to put the buttons on the same line of the import and delete ones but given that they come from another widget and that the row is then occupied, i couldn't.
Another possible issue is that the buttons now open a dialog so the brush editor popup closes in the background and there's "no" way to keep it open or reopen it.
Or well there's a way to reopen it but it involves calling 7 nested parentWidget, and then show().</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://mail.kde.org/pipermail/kimageshop/2015-June/012772.html here a little more in depth explanation on this change.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now if the popup closing is too anoying, the only way i see is to go back to tabs, but this time the brush won't be usable until add to predefined is clicked (while previously, due to the temporary brush created, just by opening the respective tab it was possible to use it).</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;">Several adding/removing/selecting tests in the predefined list of both Stamp and Clipboard brush.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>krita/libbrush/kis_imagepipe_brush.cpp <span style="color: grey">(4d301cb)</span></li>
<li>krita/plugins/extensions/dockers/defaultdockers/kis_layer_box.cpp <span style="color: grey">(810ae21)</span></li>
<li>krita/plugins/paintops/libpaintop/forms/wdgclipboardbrush.ui <span style="color: grey">(bb1cd4a0)</span></li>
<li>krita/plugins/paintops/libpaintop/forms/wdgcustombrush.ui <span style="color: grey">(043252b)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_brush_chooser.h <span style="color: grey">(aea4cd3)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_brush_chooser.cpp <span style="color: grey">(8f46885)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_brush_selection_widget.h <span style="color: grey">(2462917)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_brush_selection_widget.cpp <span style="color: grey">(1f61266)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_clipboard_brush_widget.h <span style="color: grey">(62f6aea)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_clipboard_brush_widget.cpp <span style="color: grey">(58b18e1)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_custom_brush_widget.h <span style="color: grey">(13589d0)</span></li>
<li>krita/plugins/paintops/libpaintop/kis_custom_brush_widget.cpp <span style="color: grey">(630aff5)</span></li>
<li>libs/widgets/KoResourceItemChooser.h <span style="color: grey">(74b404c)</span></li>
<li>libs/widgets/KoResourceItemChooser.cpp <span style="color: grey">(86e3663)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124048/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>