Review Request: kdelibs: "default"-values in config files should be able to handle code-generated default values

Mon Oct 4 06:40:28 CEST 2010

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5525/#review7956
-----------------------------------------------------------

beyond Marco's observation that we can't add a new virtual to that class (breaks binary compat), i'd honestly rather not support code entries unless we have some good use cases for it, since is likely to be non-trivial to get right.

i don't like mingling the ScriptEngine class in with the ConfigLoader API. they are really two completely different things (not to mention how it leads to a fairly ugly set of constructors :). at the very least it should be up the owner of the ConfigLoader class to call setScriptEngine, but even then, i'm not sure that'll work.

if we just look at the JavaScript case, the plasmoid does not really control when the config file is loaded. which means that the value can be non-deterministic unless it's a simple bit of code that is fully self contained (e.g. doesn't reference any other functions, variables, etc in the plasmoid itself). this is because when evaluate is called on QSCriptEngine the code is run in the current context. which can be almost anything, since activeConfig can be set pretty much whenever. probably the code should run in the top-level context for the package. remember that we now have addons, which means they could have their own kconfigxt files, and those should be evaluated in the context that the addon was created in. thankfully we can actually know what that context is these days since each are marked with the package as a hidden property.

not to mention issues of keeping the code that is evaluated() from accidentally stomping on, e.g., local variables. (that's easy enough to avoid though, with a new context created for it to be eval'd within.

(on a side note, i just noticed that settings files aren't kept separate for addons and the main applet, which means they can rather easily get jumbled. i'll have to come up with a solution for that one, likely by keeping each set of config objects separate for the main plasmoid and for each addon. blarg.)

this means, however, that where ConfigLoader calls evaluate, it also needs to be passing in more information that just "here, execute this script." the API will rapidly get messy.

next, from a principle-of-least-surprise perspective, i'd expect this code to be "live" when writing javascript. when it's used with C++ it isn't -> it is executated exactly once on construction of the settings class. but in those cases, the creation of the KConfigSkeleton object is controlled by the code using it. in this case, the plasmoid doesn't know when that happens and currently doesn't have a way to say "ok, put it away and re-parse it." so in the C++ case, it is really just "run it once"; it's "run it each time the settings object is created". emulating that rather useful behaviour in JS is going to tricky, if only because we will need to decide whether to unload the settings every time activeConfig is set as a way to emulate the C++ behaviour (which is partly what makes code="true" so useful in the first place), explicit rules for when the main config is parsed (though that's simple enough: before the first value is called on it), etc. the alternative is to make the code execute whenever the default is requested.

this would easy enough with QScriptEngine: each default that is code could be wrapped up in an anonymous function in the correct context and the QScriptValue that results would be held onto. whenever the default would be needed, that QScriptValue would get call()'d, and the returned value fed right back into the script env. 

as a bonus -> this would completely get rid of the problems with objects and constructors being passed back and forth since it would never actually leave the scripting env.

what might make sense it to allow configLoader to parse code="true" items (again, assuming we have good use cases for it :) and then be able to return a list of items that have code associated with them. then in the JS ScriptEngine, we could make up a small subclass of ConfigLoader which, after the ConfigLoader is done parsing, can ask for the list of all items with code for defaults and set up the anonymous functions for each. "all" that would remain is knowing when the generate the default; this could be accomplished by setting a default constructed object (e.g. QString(), QColor(), etc) as the default in such cases, and then it would need to run the anon function if property() is the default. this would mean a small amount of extra overhead on reading values -> check if it has code associated with it, if so, then get property() and see if it is the default (could be a convenience method in ConfigLoader for that; i don't think KConfigSkeletonItem makes that very easy on its own; or ConfigLoader::property() could just return QVariant() in those cases prevening another external lookup being necessary).

this would confer the following benefits:

* no new virtuals
* no appearance of ScriptEngine in ConfigLoader
* the ability to easily set the correct context (or whatever else is appropriate) for the given config file by the ScriptEngine (in this case)
* the default value would be "live" (though one could easily make it "eval only once" with a guard variable)

if my expectations of the value being "live" are incorrect, then it gets a bit easier in that the running of the code need only happen once. but then we're back to "how to reset the defaults" issue and i'd still prefer to see the middle two points in the above list met (the first item is a requirement, due to binary compat.)

- Aaron

On 2010-10-03 18:10:47, Martin Blumenstingl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5525/
> -----------------------------------------------------------
> 
> (Updated 2010-10-03 18:10:47)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> For some projects it's necessary to have code-generated default values for some of the settings.
> Just an example:
> say a plasmoid prints text somewhere. The font (including the color) is configurable.
> Now there's one problem: the developer might run into trouble when setting a hardcoded default color in the config file.
> 
> Here's my solution:
> KConfigXT already support code-generated default values, see: http://techbase.kde.org/Development/Tutorials/Using_KConfig_XT (take a look at the "Font" example).
> With my patch it's possible to add code to the default tags.
> The ConfigLoader will parse it (with the current script's script engine) if the "code" attribute is set to "true".
> 
> This patch provides the kdelibs part.
> The patch for the JavaScript engine (and probably other engines) will be part of a separate review:
> http://svn.reviewboard.kde.org/r/5526/
> 
> 
> Please note that my patch is not finished yet.
> I still need to write unit tests (but before that I need to think about how to write them..).
> 
> There is one issue which I'd like to fix soon:
> The ConfigLoader does not handle QVariants yet.
> This means: if the user specifies "type=color" then the code-generated value must return a QString (which is then passed to the ctor of QColor()).
> But a developer would expect the following: if the code-generated value returns a QString then that QString will be passed to the ctor of QColor(). If the code-generated value returns a QColor() then that one is directly used.
> 
> PS: The reason why I opened a review request for an unfinished patch is quite simple:
> Communication is the key to success.
> Thus I'd like to ask you about comments before I finish it (otherwise someone might tell me after I'm done with it: "the architecture of your patch sucks. Your patch can go to hell. kthxbye" ;)).
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/applet.cpp 1182146 
>   /trunk/KDE/kdelibs/plasma/configloader.h 1182146 
>   /trunk/KDE/kdelibs/plasma/configloader.cpp 1182146 
>   /trunk/KDE/kdelibs/plasma/private/configloader_p.h 1182146 
>   /trunk/KDE/kdelibs/plasma/scripting/scriptengine.h 1182146 
>   /trunk/KDE/kdelibs/plasma/scripting/scriptengine.cpp 1182146 
> 
> Diff: http://svn.reviewboard.kde.org/r/5525/diff
> 
> 
> Testing
> -------
> 
> The ConfigLoader unit-test did not fail :)
> I also wrote a test-plasmoid: http://filebin.ca/abcwza/codedefault.tar.bz2
> 
> 
> Thanks,
> 
> Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20101004/8e502500/attachment-0001.htm 