[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: DataEngine: Add methods to allow disabling
From: "Alex Merry" <kde () randomguy3 ! me ! uk>
Date: 2010-09-20 17:15:55
Message-ID: 20100920171555.8306.85266 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On 2010-09-10 23:56:04, Aaron Seigo wrote:
> > 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.
>
> Alex Merry wrote:
> 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.
> Aaron Seigo wrote:
> "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 :)
>
> Alex Merry wrote:
> 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).
> Aaron Seigo wrote:
> 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)));
> }
>
> Alex Merry wrote:
> 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.
> Aaron Seigo wrote:
> "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 :)
*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.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5312/#review7530
-----------------------------------------------------------
On 2010-09-10 23:30:32, Alex Merry wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5312/
> -----------------------------------------------------------
>
> (Updated 2010-09-10 23:30:32)
>
>
> Review request for Plasma.
>
>
> Summary
> -------
>
> 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.
>
> Diffs
> -----
>
> /trunk/KDE/kdelibs/plasma/dataengine.cpp 1173953
> /trunk/KDE/kdelibs/plasma/private/dataengine_p.h 1173953
> /trunk/KDE/kdelibs/plasma/dataengine.h 1173953
>
> Diff: http://svn.reviewboard.kde.org/r/5312/diff
>
>
> Testing
> -------
>
> 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.
>
> Thanks,
>
> Alex
>
>
[Attachment #5 (text/html)]
<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>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic