<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/120036/">https://git.reviewboard.kde.org/r/120036/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 2nd, 2014, 11:20 a.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;">I don't fully understand how the "write to a different property" mechanism works. Could you expand on that?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The code overall looks quite good already, but without understanding the above, I don't dare giving it a shipit.</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 just meant I don't make noteText have a WRITE attribute and instead have an explicit save() because otherwise we get people writing to the property from two different directions. <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I was anticipating a review comment and was trying to explain myself before it happened; clearly I didn't help much :)</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 2nd, 2014, 11:20 a.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120036/diff/1/?file=309277#file309277line103" style="color: black; font-weight: bold; text-decoration: underline;">applets/notes/plugin/filesystemnoteloader.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">103</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">file</span><span class="p">.</span><span class="n">write</span><span class="p">(</span><span class="n">text</span><span class="p">.</span><span class="n">toLatin1</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<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;">Why the toLatin1() here? Why not save it as UTF8?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">ooh, good spot.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I just expect people to speak English :D</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 2nd, 2014, 11:20 a.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120036/diff/1/?file=309279#file309279line50" style="color: black; font-weight: bold; text-decoration: underline;">applets/notes/plugin/note.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">50</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_UNUSED</span><span class="p">(</span><span class="n">text</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<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;">perhaps a warning here could help developers in the future, telling them that something is wrong. In esssence, if this one gets called, data may be lost, so a warning might be useful?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">good idea, I might make it pure virtual so they get told sooner.</p></pre>
<br />
<p>- David</p>
<br />
<p>On September 2nd, 2014, 11:02 a.m. UTC, David Edmundson 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 David Edmundson.</div>
<p style="color: grey;"><i>Updated Sept. 2, 2014, 11:02 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdeplasma-addons
</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 design:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
There's a NoteLoader class, currently we just hardcode loading the FileNoteLoader. The backend is refcounted between plasmoids.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
NoteLoader acts as a factory class for creating Notes, which can be subclassed and do whatever that needs to do (like loading async-ly)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The QML side saves data on loss of focus and exit. We use explicitly different loading and saving to different properties to avoid the mess of two classes trying to write to the same property.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To explain the design, here are my long term goals:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">never lose data if we wipe the relevant plasmarc</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">the same notes editable in plasmawindowed as the desktop</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">have the same note data on two containments</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">easily make an Akonadi backend for notes so we share between knotes (hence the abstract layer ATM). </li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">allows the user to be creative (store in git, sync to owncloud, whatever). Not stuff we should add in the UI, but we enable the user to do whatever.</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I envision the settings module listing available notes. By default it just creates a new note when you make a new plasmoid so 4.x behaviour is still exactly the same as before /unless/ you go hunting for more things.</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>applets/notes/plugin/note.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/notemanager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/filesystemnoteloader.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/filesystemnoteloader.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/note.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/abstractnoteloader.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/documenthandler.h <span style="color: grey">(fd3bdd0)</span></li>
<li>applets/notes/plugin/documenthandler.cpp <span style="color: grey">(b0a603a)</span></li>
<li>applets/notes/CMakeLists.txt <span style="color: grey">(c980cb0)</span></li>
<li>applets/notes/package/contents/config/main.xml <span style="color: grey">(a614f7e)</span></li>
<li>applets/notes/package/contents/ui/main.qml <span style="color: grey">(1213f48)</span></li>
<li>applets/notes/plugin/abstractnoteloader.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/notemanager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/notes/plugin/notesplugin.cpp <span style="color: grey">(890caeb)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120036/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>