> 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
>
>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
