<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/119232/">https://git.reviewboard.kde.org/r/119232/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 11th, 2014, 2:48 p.m. UTC, <b>Sebastian Kügler</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It would be easier to review, if you made separate patches for the renaming and the actual changes in the code that juggle around the objects.</p></pre>
</blockquote>
<p>On July 11th, 2014, 3:08 p.m. UTC, <b>Martin Klapetek</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I did the renaming in the middle of this as I got annoyed by the confusing names.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To sum it up - all Rectangles are replaced by one Rectangle and two Repeaters at the top in DaysCalendar.qml, the rest is just renaming and sanitizing.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I understand all that, it just doesn't change the fact that now, I have to look at every changeset first, if it's just renaming or a real code change. You want others to verify that everything's correct, because if not, you can just commit it without review and then wait for complaints. We've decided that that is not how we want to work together. (There's already one thing gone unnoticed to you, so I think a good review is warranted here, especially since, in the past, you have suggested changes which are detrimental to the overall design, suggesting to me that you haven't fully internalized what the important points of the design are, or that you are making different calls than I would do.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the end, if changes aren't self-contained, you're accepting a trade-off between your time and the reviewer's.</p></pre>
<br />
<p>- Sebastian</p>
<br />
<p>On July 11th, 2014, 2:35 p.m. UTC, Martin Klapetek 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 Plasma.</div>
<div>By Martin Klapetek.</div>
<p style="color: grey;"><i>Updated July 11, 2014, 2:35 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</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;">Currently the grid itself is composed of 88 rectangles that draw all the lines in a way that two big rect draws the whole two topmost horizontal and leftmost vertical border lines and then each day rectangle is drawing small bottom and right rect.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch reduces it to 13 rects only where one rect draws the whole frame around the grid and then 1px wide/high rects draw the inner lines. Results in much cleaner & simple code.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Plus there's a small refactor on the id names so it makes more sense.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This does not require any additional changes in the applets.</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;">All applets using the Calendar component looks exactly the same and work perfectly fine with no changes in the applets.</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>src/declarativeimports/calendar/qml/MonthMenu.qml <span style="color: grey">(5209816)</span></li>
<li>src/declarativeimports/calendar/qml/MonthView.qml <span style="color: grey">(ba3fe95)</span></li>
<li>src/declarativeimports/calendar/qml/CalendarToolbar.qml <span style="color: grey">(cd28702)</span></li>
<li>src/declarativeimports/calendar/qml/DayDelegate.qml <span style="color: grey">(11f0afa)</span></li>
<li>src/declarativeimports/calendar/qml/DaysCalendar.qml <span style="color: grey">(ae9df01)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119232/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/07/11/9a4f02eb-b06c-4d13-8ea5-94276659fba8__plasma_cal4.png">digital-clock before/after</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>