<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/119905/">https://git.reviewboard.kde.org/r/119905/</a>
</td>
</tr>
</table>
<br />
<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;">Patch looks good in general except for some bad naming of a function. It was good that you created a slot for FileNew int he view, that's where it belongs. I'm sure this can be pushed in one more iteration.</p></pre>
<br />
<div>
<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/119905/diff/1/?file=307130#file307130line112" style="color: black; font-weight: bold; text-decoration: underline;">src/parleydocument.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ParleyDocument::~ParleyDocument()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">112</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_parleyApp</span><span class="o">-></span><span class="n">slotUpdateWindowCaption</span><span class="p">();</span> <span class="c1">// FIXME: This is what signals are for. The document should *not* directly call it's owner. What if there are >1 window, for instance?</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">You removed the call, but why not add a signal? That seems the natural thing to do.</p></pre>
</div>
</div>
<br />
<div>
<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/119905/diff/1/?file=307131#file307131line116" style="color: black; font-weight: bold; text-decoration: underline;">src/parleymainwindow.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<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">116</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/** Updates connections when the ParleyDocument signals it changed @p doc**/</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">If the function updates connections, then it shouldn't be called update...Document(). I suggest documentUpdated() instead which indicates that that it should be called when the document is updated to take care of any bookkeeping (in this case updating the connections, but that is not relevant from the outside).</p></pre>
</div>
</div>
<br />
<div>
<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/119905/diff/1/?file=307131#file307131line119" style="color: black; font-weight: bold; text-decoration: underline;">src/parleymainwindow.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<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">119</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/** Calls the ParleyDocument slotFileNew and then fixes connections**/</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Generally a comment like this should say what the function does, not how it does it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A better description would be something along the lines of "Creates a new document". Another comment is that in general in KDE names like slot...() are discouraged. Slots are normal functions that happen to be callable through signals in addition to the normal way. (But there are other examples of this case in Parley already so I'll let it slide.)</p></pre>
</div>
</div>
<br />
<div>
<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/119905/diff/1/?file=307132#file307132line133" style="color: black; font-weight: bold; text-decoration: underline;">src/parleymainwindow.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">133</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">m_document</span><span class="o">-></span><span class="n">document</span><span class="p">(),</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">docModified</span><span class="p">(</span><span class="kt">bool</span><span class="p">))</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">This m_document->document() shows poor data hiding. But that's historical so it's not your fault.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But I am sure that there is a missing disconnect somewhere (not necessarily in this function). At least I couldn't find any. If a previous ParleyDocument is deleted there needs to be a disconnect of the signal.</p></pre>
</div>
</div>
<br />
<p>- Inge Wallin</p>
<br />
<p>On August 22nd, 2014, 6:23 p.m. UTC, Andreas Xavier 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 KDE Edu.</div>
<div>By Andreas Xavier.</div>
<p style="color: grey;"><i>Updated Aug. 22, 2014, 6:23 p.m.</i></p>
<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=338302">338302</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=338417">338417</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
parley
</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;">Symptoms:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The [modified] in the title bar was inconsistent.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Problems:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
1. The title bar was only connected to the dirty bit in the doc from the Editor. Going directly welcome -> stats did not connect the title bar to the dirty bit.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
2. Whenever a new document was created, new collection, welcome screen etc. the title bar was disconnected.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
3. In several places setModified was called on the kvocdoc when either nothing changed or the kvocdoc should know better.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Solution:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The title bar is ParleyMainWindow's title bar. ParleyMainWindow also know when the document is updated. I created a updateParleyDocument so that ParleyMainWindow always knows when the doc changes and slotFileNew so that anyone can ask ParleyMainWindow to update its own title bar.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not fixed: Parley still directly calls the setModified in kvocdoc when it starts practice. kvocdoc needs to be fixed before that is removed.</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;">Clicked from welcomescreen -> stats -> practice -> exit stats<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Clicked from welcomscreen -> stats -> editor (edited) -> practice<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Clicked from welcomscreen -> stats -> editor (no edit) -> practice<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Created a new collection from welcome screen<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Changed doc properties from stats->File->Properties.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All of these cases worked correctly</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/editor/editor.cpp <span style="color: grey">(4fac862)</span></li>
<li>src/parleydocument.cpp <span style="color: grey">(0ebeca1)</span></li>
<li>src/parleymainwindow.h <span style="color: grey">(98a94ad)</span></li>
<li>src/parleymainwindow.cpp <span style="color: grey">(9723ee1)</span></li>
<li>src/practice/practicemainwindow.cpp <span style="color: grey">(3008501)</span></li>
<li>src/settings/documentproperties.cpp <span style="color: grey">(02cbf7b)</span></li>
<li>src/settings/languageproperties.cpp <span style="color: grey">(e9b171f)</span></li>
<li>src/vocabulary/vocabularymodel.cpp <span style="color: grey">(abf0db9)</span></li>
<li>src/welcomescreen/welcomescreen.cpp <span style="color: grey">(d24f444)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119905/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>