<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/124589/">https://git.reviewboard.kde.org/r/124589/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 6th, 2015, 2:12 p.m. UTC, <b>Lamarque Souza</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/124589/diff/5/?file=389660#file389660line158" style="color: black; font-weight: bold; text-decoration: underline;">applets/diskquota/plugin/DiskQuota.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">158</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">bool</span> <span class="n">quotaFound</span> <span class="o">=</span> <span class="o">!</span> <span class="n">QStandardPaths</span><span class="o">::</span><span class="n">findExecutable</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span class="s">"quota"</span><span class="p">)).</span><span class="n">isEmpty</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;">You should search for quota and filelight programs during startup only. You can send a notification if they are not found so the user knows they are not installed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Polling filesystem every two minutes is not extreme bad but it should be prevented if it is not really necessary.</p></pre>
</blockquote>
<p>On August 8th, 2015, 1:47 p.m. UTC, <b>David Edmundson</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;">Can you think of a way we can tell if it's installed later?</p></pre>
</blockquote>
<p>On August 8th, 2015, 3:04 p.m. UTC, <b>Lamarque Souza</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;">The plasmoid will search for them at every logon. Why is that not enough?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You can also connect a slot to org.freedesktop.ScreenSaver.ActiveChanged(false) signal from ksmserver to check for them when the user unlocks the screen.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The point is that current code searches for them even when they were detected two minutes before. That's overkill. If the intention is to be over precautions then I step down here. I still insist in sending a notification to warn the user if the programs are not installed.</p></pre>
</blockquote>
<p>On August 9th, 2015, 5:04 p.m. UTC, <b>Dominik Haumann</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;">Ok, I would like to implement the following solution:
- In the constructor, I check only once if quota exists. If it exists, all is good, and the applet runs as before.
- In the constructor, if 'quota' does not exist, I will <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> launch the timer and instead add a button with the text i18n("Check Again") under the text displayed of this: http://kate-editor.org/wp-content/uploads/2015/08/diskquota-missing.png - Clicking this button would look for 'quota' again, and on success starts the timer and the applet runs as before.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Would you accept this solution?</p></pre>
</blockquote>
<p>On August 9th, 2015, 5:06 p.m. UTC, <b>Kai Uwe Broulik</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;">What about checking when you open the plasmoid?</p></pre>
</blockquote>
<p>On August 10th, 2015, 9:45 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 think the solution with the button is fine. Elegant, and exactly there where you'd expect it.</p></pre>
</blockquote>
<p>On August 10th, 2015, 11:12 a.m. UTC, <b>Lamarque Souza</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 like Kai's suggestion: if the programs were not found yet then search for then when user clicks on system tray icon. That is more automatic then using a button.</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;">I know I said the same as Lamarque in my review, but on reflection I don't think it's worth changing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Given it's not in the default set, a user has to manually add it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On finding an error message "you must install quota" the user will either:
- install quota
- remove the plasmoid</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So we're never realistically going to be in this situation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also I did a benchmark:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #008000; font-weight: bold">QBENCHMARK</span> {
QStandardPaths<span style="color: #666666">::</span>findExecutable(QStringLiteral(<span style="color: #BA2121">"quota"</span>));
}
<span style="color: #008000; font-weight: bold">0</span><span style="color: #0000FF; font-weight: bold">.017</span> <span style="color: #008000; font-weight: bold">msecs</span> <span style="color: #008000; font-weight: bold">per</span> <span style="color: #008000; font-weight: bold">iteration</span> <span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">total</span><span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">72</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">iterations</span><span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">4096</span><span style="color: #666666">)</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's one second of overhead every 81.7 days of constant use. (I did the maths)
We've got much bigger places to worry about optimising than a stat call.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm fine with you pushing as-is.</p></pre>
<br />
<p>- David</p>
<br />
<p>On August 3rd, 2015, 5:34 p.m. UTC, Dominik Haumann 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, Kai Uwe Broulik and Sebastian Kügler.</div>
<div>By Dominik Haumann.</div>
<p style="color: grey;"><i>Updated Aug. 3, 2015, 5:34 p.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 disk quota is usually used in enterprise installations where network shares are mounted locally. Typically, sysadmins want to avoid that users copy lots of data into their folders, and therefor set quotas (the quota limit has nothing to do with the physical size of a partition). Typically, once a user gets over the hard limit of the quota, the account is blocked and the user cannot login anymore. This happens from time to time, since the users are not really aware of the current quota limit and the already used disk space.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Here is where the "Disk Quota" plasmoid helps: It continusouly monitors the disk quota and warns the quota apprpriately.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A detailed description including screenshots can be found in this blog: http://kate-editor.org/?p=3591</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(I had a KDE4 hack of this plasmoid running at university, and it proved very usable over the years, so it is probably a good idea to have it by default in plasma)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Issues:
- the panel icon is larger than the others (some wrong margin?)
- an icon for the metadata.desktop is missing (the shipped quota.svg file is not available here, it seems).
- the grid units probably need some more tuning</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;">Tested combinations:
- no quota installed: A nice message is displayed telling the user that 'quota' is missing.
- quota installed, but no quota restrictions set: The applet says "No quota restrictions found"
- quota installed, quotas active: The applet continuously shows the data. The quota entries are in a QAbstractItemModel derived class, so inserting/removing quotas all works (tested).
- filelight installed: the item under mouse gets highlighted. If clicked, filelight starts with the correct location.</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/CMakeLists.txt <span style="color: grey">(c60c350)</span></li>
<li>applets/diskquota/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/Messages.sh <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/icons/quota.svg <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/package/contents/ui/ListDelegateItem.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/package/contents/ui/main.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/package/metadata.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/DiskQuota.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/DiskQuota.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/QuotaItem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/QuotaItem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/QuotaListModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/QuotaListModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/plugin.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/plugin.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/diskquota/plugin/qmldir <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124589/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>