Review Request 113443: Attempt to unbreak pre-configuring QML applets via desktop scripting

[Aaron J. Seigo]

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


syncing after every change will be horrible for performance; that is why we have a timed sync() after changes to compress these sync()s. so this patch can not be merged in.

KCoreConfigSkeleton uses a KSharedConfig, so any changes made to it should get kept when the reparseConfig call is made. the shell scripting uses the config from the applet, which uses the config from Corona, which opens the config this way:

    d->config = KSharedConfig::openConfig(d->configName, KConfig::SimpleConfig);

all i can think of is that the KSharedConfigPtr's used in Corona and in the KCoreConfigSkeleton are not pointing to the same shared object, and so writes are getting lost as a result.

note that KConfig::reparseConfiguration() does this:

    if (!d->isReadOnly() && d->bDirty)
        sync();

so that sync() is already happening if it is dirty. which also really sucks imho. kconfig in general is full of such really non-optimal things.

- Aaron J. Seigo


On Oct. 26, 2013, 12:55 a.m., Eike Hein wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113443/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2013, 12:55 a.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Marco Martin.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> I've noticed that a panel layout JavaScript template file doesn't succeed in scripting the config of a pure-QML applet. I.e. this doesn't work:
> 
> foo = panel.addWidget("bar");
> foo.currentConfigGroup = new Array("General");
> foo.writeConfig("foo", "bar");
> 
> It does work for C++ applets however. Also, the config does ultimately make it to disk, and a subsequent restart of plasma-desktop reads it in just fine. Or even just opening and OK'ing the config dialog, even if the setting isn't exposed in the GUI.
> 
> So I've been looking at the code a little, and what it comes down to is that kde-workspace/libs/plasmagenericshell/scripting/applet.cpp holds an Applet::reloadConfig() that calls Plasma::Applet::configChanged(), which, if there's a d->script, calls readConfig() on its d->configLoader. ConfigLoader is a KConfigSkeleton subclass, and what KCoreConfigSkeleton::readConfig() does is explicitly read in the config from disk.
> 
> Therefore the attached patch addresses this by calling for a config sync ahead of time.
> 
> Now, I'm new to those parts of the Plasma code base, and I'm really not sure this is a kosher way to go about it. It's not clear to me whether we want to flush to disk at that time, or if there might be a prohibitive performance penalty. There may be a way to avoid this. Perhaps Plasma::Applet shouldn't clobber the modified config group by pulling in from disk -- but perhaps it needs to for other reasons. That's why someone with the bigger picture of how the design was supposed to operate needs to review this.
> 
> Debate welcome. OTOH, I am a bit pressed for time to ideally get this fixed for a release, so there'd be some value in a "good enough" too :).
> 
> 
> Diffs
> -----
> 
>   libs/plasmagenericshell/scripting/applet.cpp 3c81f77 
> 
> Diff: http://git.reviewboard.kde.org/r/113443/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Eike Hein
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20131026/26c50f3e/attachment.html>