<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="http://svn.reviewboard.kde.org/r/5312/">http://svn.reviewboard.kde.org/r/5312/</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 10th, 2010, 11:56 p.m., <b>Aaron Seigo</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;">if there is nothing connected to it, why would it want to fill that data? if nothing is connected, then there is precisely no reason to deliver that data. it sounds like the system monitor engine is doing something wrong.</pre>
</blockquote>
<p>On September 11th, 2010, 12:19 a.m., <b>Alex Merry</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;">System monitor maintains its list of sources (ie: things that can be monitored) by maintaining a bunch of DataContainers. Ordinarily, these just have some meta-information about the thing being monitored, such as a human-readable name and the units. Only when a source is connected to are the values updated.
This is not an unusual approach to take, as far as I'm aware - that's kind of the point of DataContainer, right? It's just that systemmonitor doesn't bother subclassing DataContainer, but instead uses it directly.
Note that the way KSysGuard works means that if it fetches the list of things that can be monitored, it has to fetch the meta-info as well, so it may as well store it. The issue is simply that it has to fetch this asynchonously, and visualizations may request a source before it has done so.</pre>
</blockquote>
<p>On September 11th, 2010, 1:20 a.m., <b>Aaron Seigo</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;">"This is not an unusual approach to take"
no, it is quite unusual. a DataContainer, once added to an engine due to a sourceRequestEvents not only represents that source but the source has the lifespan of the request.
instead, what it ought to do is add all the DataContainers using DataEngine::addSource(DataContainer *) and then update them in updateRequestEvent. this behaviour can be triggered with setMinimumPollingInterval(0). this is documented in the apidox and is the way to get the desired behavior.
so, yes, SystemMonitor is at fault here :)</pre>
</blockquote>
<p>On September 19th, 2010, 11:32 a.m., <b>Alex Merry</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;">I don't see how that would help.
The issue systemmonitor is trying to deal with is that it finds out what sources it should add *after* one of them is requested. So it has the option of refusing to deal with the request at all (in which case visualizations would need to listen to sourceAdded() - not a problem in itself, but this make break existing visualizations using systemmonitor) or adding it in the hope that it will be created (the current behaviour, and the cause of the problems).
With the latter approach, when it finds out what sources are available, it has the choice of populating the existing source (in which case it may well be removed by DataEngine at some point) or removing it and re-adding it (disconnecting the visualization in the process, defeating the purpose of creating the dummy source in the first place).</pre>
</blockquote>
<p>On September 19th, 2010, 9:28 p.m., <b>Aaron Seigo</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;">an easier way around this is to disconnect the DataContainer's becameUnused signal:
DataContainer *s = containerForSource(source);
if (s) {
disconnect(s, SIGNAL(becameUnused(QString))), this, SLOT(removeSource(QString)));
}</pre>
</blockquote>
<p>On September 20th, 2010, 12:33 a.m., <b>Alex Merry</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;">Sure, but this is awful hackish.
Or do we put this down to "we have to be hacky to maintain backwards compatibility with something that was doing it wrong"?
Well, that's what I've gone for, anyway. Along with warning comments not to copy the pattern.</pre>
</blockquote>
<p>On September 20th, 2010, 4:47 p.m., <b>Aaron Seigo</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;">"Sure, but this is awful hackish."
considerably less so than adding API to DataEngine that breaks the pattern of usage, and doesn't take into consideration issues such as only wanting a specific source to not be autoremoved. the patch proposed creates several new corner cases and, when the new feature is used, makes writing and maintaining a DataEngine more complex.
if your complaint is that disconnecting the signal requires knowing that the signal is connected in the first place, it could be replaced by a method added to DataContainer that does similarly, hiding the "how it is done"
however, it remains that the system monitor DataEngine is _doing is wrong_, which is why i have very little motivation to work around its quirks. if nothing is listening to the source, then the source does not need to be updated, and the DataContainer associated with that source can be removed. if the system monitor DataEngine is using that DataContainer to cache some data that should outlive the DataContainer, then it should be caching the information elsewhere. the alternative is to not create the source on-demand.
looking at the system monitor engine, however, i see absolutely no reason why the data must be set or updated at all unless something is connected to it. it shouldn't be creating all the sources in the id == -1 branch of SystemMonitorEngine::answerReceived; it should simply populate m_sensors as it already does and populate entries that already have been requested. it already has an updateSourceEvent, so it should all "just work" if it doesn't try to fight the DataEngine pattern :)
</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;">*shrugs* It's not my work, I just wanted to fix a bug that appeared in bubblemon.
I might fix the trunk version to work sensibly at some point, though, if I get time and motivation.</pre>
<br />
<p>- Alex</p>
<br />
<p>On September 10th, 2010, 11:30 p.m., Alex Merry wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma.</div>
<div>By Alex Merry.</div>
<p style="color: grey;"><i>Updated 2010-09-10 23:30:32</i></p>
<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;">Allow DataEngine implementations to prevent DataEngine from automatically deleting sources create in sourceRequestEvent.
Rationale: engines that fetch a list of sources asynchronously at creation time (such as systemmonitor) may reasonably want to create dummy data sources when they are requested before the list has been fetched. However, they probably don't want these to be removed again when disconnected from.</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;">It makes the systemmonitor engine work properly. Specifically, it solves the bug in bubblemon where if you had the bubblemon displaying, say, CPU Total Load when plasma started and then changed it to, say, CPU Idle Load, the CPU Total Load source would be removed and there would be an empty place where it should be in the list in the config dialog.</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>/trunk/KDE/kdelibs/plasma/dataengine.cpp <span style="color: grey">(1173953)</span></li>
<li>/trunk/KDE/kdelibs/plasma/private/dataengine_p.h <span style="color: grey">(1173953)</span></li>
<li>/trunk/KDE/kdelibs/plasma/dataengine.h <span style="color: grey">(1173953)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5312/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>