> On Aug. 2, 2011, 4:22 a.m., Aaron J. Seigo wrote: > > i don't like the idea of this being public API, at least not until we know > > we need it to be. > > > > i also recommend changing the name of the class from EngineInstaller to > > ComponentInstaller, since it seems more generic than just "Engines" (which > > itself is ambiguous due to ScriptEngines and DataEngines).
Thanks for the review. > i don't like the idea of this being public API, at least not until we know we > need it to be. Well, I can make this a private class for now, but I might need it at least in kde-workspace eventually. I also think this would be a useful public API to have, but I don't feel strongly about that. > i also recommend changing the name of the class from EngineInstaller to > ComponentInstaller OK, I'll rename it (I don't care all that much about the name), but… > since it seems more generic than just "Engines" (which itself is ambiguous > due to ScriptEngines and DataEngines). … that's kinda the point, the EngineInstaller can install both data engines and script engines. :-) At this time, I'm not sure what other stuff it'd be useful for, but "ComponentInstaller" is probably more future-proof. I will update the patch based on your suggestions. > On Aug. 2, 2011, 4:22 a.m., Aaron J. Seigo wrote: > > plasma/CMakeLists.txt, line 53 > > <http://git.reviewboard.kde.org/r/102175/diff/1/?file=30596#file30596line53> > > > > mismatch with if Whoops, I renamed the variable and forgot the endif (and CMake really just ignores it, so it didn't complain). I'll fix that. > On Aug. 2, 2011, 4:22 a.m., Aaron J. Seigo wrote: > > plasma/engineinstaller.cpp, line 78 > > <http://git.reviewboard.kde.org/r/102175/diff/1/?file=30599#file30599line78> > > > > so if !force, it shouldn't be added to alreadyPrompted? > > > > it may also be worthwhile to remove items from alreadyPrompted once > > there is success (thus freeing up that memory), assuming packagekit returns > > this information. > so if !force, it shouldn't be added to alreadyPrompted? The idea is that force will be false for requests triggered by use (on demand), and true for requests triggered by installation (metadata explicitly requesting a component, this is not part of this patch, but will be in a later patch). So they can be kept quite separate in principle. However, I'll add the force requests to alreadyPrompted too for consistency. > it may also be worthwhile to remove items from alreadyPrompted once there is > success (thus freeing up that memory), assuming packagekit > returns this information. This should be possible in principle, but is the added code to handle the asynchronous replies really worth the few bytes of QSet<QString> entries it'll save? (I don't expect that small set of short strings to become a memory hog at all.) - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102175/#review5307 ----------------------------------------------------------- On Aug. 2, 2011, 2:57 a.m., Kevin Kofler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102175/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2011, 2:57 a.m.) > > > Review request for Plasma. > > > Summary > ------- > > This is part of my GSoC 2011 work. Expect more Plasma patches related to this > in the next few days. > > In particular, the currently unused "force" feature of the new API is > intended to be used if the user just installed a widget which explicitly > requires a given engine. (By default, prompts are not repeated within a > session to prevent flooding the user.) > > The implementation is based on PackageKit. It requires PackageKit >= 0.6.16 > and either Apper trunk or the backported patch from > http://pkgs.fedoraproject.org/gitweb/?p=kpackagekit.git;a=blob;f=kpackagekit06-plasma.patch;h=208427aa6cc072164b6a9ba48a30a954657ef892;hb=HEAD > to have any effect. If the requirements are not met, no change will be > noticeable at all, because the asynchronous call will simply fail and any > errors are (intentionally) discarded. (We don't want to annoy the user with a > pointless error dialog if he/she doesn't have PackageKit installed or his/her > version is too old.) > > PackageKit will also only actually find the relevant packages if the > distribution is using RPM and yum (at this time) and if the RPMs in the > repository have been built with my Provides generator. But that shouldn't be > Plasma's problem. Any work in that area needs to happen on the PackageKit or > distro side. Support for GStreamer plugins has been implemented in other > PackageKit backends too, so it should also be doable for Plasma engines. And > if a distro wants to use something entirely different from PackageKit, that's > also possible, this is what the abstract EngineInstaller API is for. > > The API is public because it might be useful outside of libplasma, and it is > quite simple and generic, thus keeping BC should not be a problem. > > > Diffs > ----- > > includes/CMakeLists.txt a967a92 > includes/Plasma/EngineInstaller PRE-CREATION > plasma/CMakeLists.txt ef411df > plasma/dataenginemanager.cpp 988fe76 > plasma/engineinstaller.h PRE-CREATION > plasma/engineinstaller.cpp PRE-CREATION > plasma/scripting/scriptengine.cpp fb8cd1a > > Diff: http://git.reviewboard.kde.org/r/102175/diff > > > Testing > ------- > > Verified that it compiles without errors and that it successfully prompts for > a missing Python script engine on Fedora 15. (The patch is against master > (4.8), but applies without changes to the kdelibs 4.6.5 in Fedora 15, which > is how I tested it.) > > > Thanks, > > Kevin > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
