<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="https://git.reviewboard.kde.org/r/118686/">https://git.reviewboard.kde.org/r/118686/</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;">The code looks very nice and clean so kudos for that. I only found very minor things which I commented on below. I also love the approach that you are using.
However I don't think it's reasonable to force the user to rename files. Many times they will be using downloaded files from GHNS and they won't even know where they are. Before we apply this patch (which I would like to do very much!) we have to implement an option to steal the lock.
As a side note, this patch overlaps with jpwhiting's one where he changes KUrl to QUrl. If he merges his patch before, you will have to adapt your patch to his but that should be reasonably simple.</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/118686/diff/1/?file=280427#file280427line63" style="color: black; font-weight: bold; text-decoration: underline;">keduvocdocument/keduvocdocument.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; ">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">63</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_autosave</span> <span class="o">=</span> <span class="k">new</span> <span class="n">KAutoSaveFile</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If you are immediately creating one in the constructor then why not have it as a real member instead of a pointer? A pointer only makes sense if the pointer doesn't point to anything at some times, thereby saving memory.</pre>
</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/118686/diff/1/?file=280429#file280429line2" style="color: black; font-weight: bold; text-decoration: underline;">keduvocdocument/tests/keduvocdocumentfilelockingtest.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">2</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> Copyright 2007-2008 Frederik Gladhorn <frederik.gladhorn@kdemail.net></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Really? I don't think so... :)</pre>
</div>
<br />
<p>- Inge Wallin</p>
<br />
<p>On June 12th, 2014, 5:05 a.m. UTC, Andreas Xavier wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Edu.</div>
<div>By Andreas Xavier.</div>
<p style="color: grey;"><i>Updated June 12, 2014, 5:05 a.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=240552">240552</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
libkdeedu
</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;">Implement a lock on the language file.
The problem: When 2 or more instances of parley open that same language file,
the last instance to exit overwrites all of the previous changes.
This patch implements unit-tests that demonstrate the problem, and file locking
on the language file to fix the problem.
The unit tests are straight forward with different combinations of open, editing and saving
language files. The conditions that I anticipated would replicate the problem.
As a solution this patch uses KAutoSaveFile to implement file locking. m_autosave replaces m_url to track the file.
Second and subsequent instances of KEduVocDocument can't get a lock for the file. From a programmer perspective
it returns FileCannotWrite when file access fails. From a user perspective it says that it
can't read collection from <file name>.
I added KEduVocDocument::close() to the public interface for symmetry with open(). It is not necessary and if interface changes are inconvenient at this time, it can be removed. Destroying the KEduVocDocument, changing the Url with setUrl or saveAs, or opening another file all close the currently open file and eliminate the lock.
One foreseeable problem is that the current parley interface doesn't offer a way to remove the lock if parley crashes.
The vocabulary file would have to be renamed by hand. However, this is an improvement from data loss to data inaccessible.
The obvious solution is to add a value to the error enumeration that means the file is locked, and then offer the user to option to steal the lock. That would require changes to the KEduVocDocument interface. </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;">1. Unit tests
2. Opened 2 instances of parley and verified that only one could use the file and then after exiting
that the other could then use the file and then that both successive sets of results had been saved. </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>keduvocdocument/keduvocdocument.h <span style="color: grey">(dfd45c3)</span></li>
<li>keduvocdocument/keduvocdocument.cpp <span style="color: grey">(b73fc69)</span></li>
<li>keduvocdocument/tests/CMakeLists.txt <span style="color: grey">(f05b787)</span></li>
<li>keduvocdocument/tests/keduvocdocumentfilelockingtest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>keduvocdocument/tests/keduvocdocumentvalidatortest.cpp <span style="color: grey">(834564c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118686/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>