Review Request 109886: KoGenericRegistry, KoShapeRegistry, KoInlineObjectRegistry factory adding/removing

[Commit Hook]

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


This review has been submitted with commit aeba95e21aaa98557471d66b5d1af3310d5a6659 by Sebastian Sauer to branch coffice.

- Commit Hook


On April 6, 2013, 3:09 p.m., Sebastian Sauer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109886/
> -----------------------------------------------------------
> 
> (Updated April 6, 2013, 3:09 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> === Problems ===
> 
> Some of our central KoGenericRegistry's like KoShapeRegistry and KoInlineObjectRegistry keep an internal cache-map to speed up lookup later on.
> 
> That looks like:
> 
> void KoInlineObjectRegistry::Private::init(KoInlineObjectRegistry *q) {
>     KoPluginLoader::instance()->load(QString::fromLatin1("Calligra/Text-InlineObject"),
>                                      QString::fromLatin1("[X-KoText-PluginVersion] == 27"), config);
>     foreach (KoInlineObjectFactoryBase *factory, q->values())
>         cacheMap.insert(..., factory);
> }
> 
> init() is called exact once if the static Registry::instance() function is called first.
> 
> === Problems ===
> 
> 1. All later calls to Registry::add() will not be in that specialized caches but only in the KogenericRegistry's factory lookup-hash.
> 2. Adding to the cache happens in the most early stage possible. That has the effect that only KoPluginLoader::instance()->load() is able to actually attached factories to the registries and that only thanks to the design of K_GLOBAL_STATIC which allows reentrance.
> 3. Factories removed from such a registry will not be removed from the internal caches.
> 
> All that has the effect that *only* factories defined in plugins or those explicit instanciated and created in the registry's ctor are actually working.
> Solutions like attaching them later (e.g. scripting, lazy initialization, other plugin-mechansims like the Android-one used in Coffice at Android are not working cause of that.
> 
> === Solution ===
> 
> 1. Be sure KoGenericRegistry::m_hash and caches in inheriting classes are in sync always.
> 2. Introduce two virtual methods: virtual_add and virtual_remove which are called in the KoGenericRegistry if a factory is added/removed.
> 3. In specialized classes remove all iterations over values to fill caches at construction-time and move such logic to an overwritten virutal_add.
> 4. Implement virtual_remove for all such caches so removing a factory from a registry also removes it from possible caches.
> 
> === Changed cases ===
> 
> I did manually check all Registries and we have a lot of them (half of them used in Krita btw). Good news is that Krita does well except for one case which I described in the following section.
> 
> * libs/flake/KoShapeRegistry.h
> * libs/kotext/KoInlineObjectRegistry.h
> Contain both an internal lookup-cache. Both fixed, tested, unittests run.
> 
> * libs/flake/KoInputDeviceHandlerRegistry.h
> Contains logic to KoInputDeviceHandler::start() right they got loaded.
> This was changed to do it on add() *and* stop the device again on remove() rather then only in the registry's dtor.
> I verified that the only input device handler we seem to have (for the SpaceNavigator) does handle that (stopping twice) proper. Do we have other devices outside of the tree where that (stop() when not running any longer) could lead to problems?
> 
> === Skipped cases ===
> 
> * krita/image/brushengine/kis_paintop_registry.h
> Does remove KisPaintOpFactory instances again if they are not supposed to handle the specificed KisImageSP.
> That additional condition is skipped if KisPaintOpRegistry::add() is used from outside. I think that's okay in that if somebody does explicit add a factory to the registry he/she may know better. On the other hand is there a problem if the registry contains factories that not fullfit that condition?
> 
> * sheets/FunctionModuleRegistry.h
> That's a bit more difficult in that it also fetches various other things and does stuff like documentation for the function-modules.
> I skipped that for now and will look at a later time what needs to be done there to make it proper working also for Coffice.
> 
> === Solution & Testing ===
> 
> This is actually the n-th iteration of a patch for that specific problem. In Coffice I am actually using currently the previous version of that patch but decided to rewrite it to came up with the imho most clean solution to solve that problem.
> 
> Lots of testing done. Still this patch does changes in the very core of our code affecting nearly all apps and lots of cases I wasn't able to proper test (like space navigator) and while I think it has little possibilities to break something...
> So, review/suggestions/questions/wishes/etc please :)
> 
> 
> Diffs
> -----
> 
>   interfaces/KoGenericRegistry.h 3d4c7df 
>   libs/flake/KoInputDeviceHandlerRegistry.h 44784c9 
>   libs/flake/KoInputDeviceHandlerRegistry.cpp d260595 
>   libs/flake/KoShapeRegistry.h b97eb80 
>   libs/flake/KoShapeRegistry.cpp 148e686 
>   libs/kotext/KoInlineObjectRegistry.h 2132b99 
>   libs/kotext/KoInlineObjectRegistry.cpp c59d318 
> 
> Diff: http://git.reviewboard.kde.org/r/109886/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sebastian Sauer
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130413/7b0f47b9/attachment.htm>