> On 2010-10-04 04:41:13, Aaron Seigo wrote: > > 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.) > >
"then it gets a bit easier in that the running of the code need only happen once" it should be noted that the return value should probably remain a QScriptValue for proper transparency to the script writer (who may expect crazy things like returning a different data type on default; such is the world of scripts, i've found) and to avoid the whole "initializing the C++ object properly" mess. - Aaron ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5525/#review7956 ----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
