Review Request 113659: system-monitor plasmoid: respect settings from js code

[Eike Hein]

> On Nov. 5, 2013, 7:39 p.m., Eike Hein wrote:
> > This is a promising first start. Indeed, the System Monitor applet didn't implement configChanged(), causing scripting its configuration not to work, as we talked about on IRC.
> > 
> > However, the implementation in the proposed patch is not yet complete, either:
> > - What if one of the child applets was *removed* from the config since the last configChanged() event?
> > - What if one of the child config groups changed, not the applets list?
> > 
> > Conceptually, configChanged() is supposed to re-read and apply the config, which means it has to adapt whatever the *current* state is to whatever the config prescribes.
> > 
> > In the case of the System Monitor applet, which maintains a set of child applets, that means applets have to be instanciated (as seen in your patch) or *destroyed* as necessary.
> > 
> > Further, for whatever child applets remain, it should forward configChanged() to them, so they, too, are poked to reload their configs.
> 
> Sergei Lopatin wrote:
>     For now my code removes all child applets and re-adds them according to config. I think that it is the best way to save order of applets and re-read config of child applets. But I agree that if changes not belong to applets list, such behaviour is not necessary.

Ah right, I overread that toggled() calls removeApplet() indiscriminately, sorry.

Still means it's a bit heavy-handed :). But an improvement over not supporting configChanged() I guess, yeah.


- Eike


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113659/#review43119
-----------------------------------------------------------


On Nov. 5, 2013, 7:44 p.m., Sergei Lopatin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113659/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2013, 7:44 p.m.)
> 
> 
> Review request for Plasma and Eike Hein.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> Added ability to system-monitor plasmoid to set configuration from js code. Something like this:
> a = activityForScreen(0)
> if (a) {
> var applet = a.addWidget("system-monitor_applet")
> applet.writeConfig("applets", "sm_ram,sm_cpu")
> }
> 
> 
> Diffs
> -----
> 
>   plasma/generic/applets/system-monitor/system-monitor.h f8a2975 
>   plasma/generic/applets/system-monitor/system-monitor.cpp 1705a99 
> 
> Diff: http://git.reviewboard.kde.org/r/113659/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergei Lopatin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20131105/65ee7de4/attachment.html>