Instead of a singleton, CommandInfoProvider (svtools/source/misc/commandinfoprovider.cxx) should just be a set of static methods. The task is to refactor that class accordingly and update all usages: http://opengrok.libreoffice.org/search?q=CommandInfoProvider .
Update: CommandInfoProvider is now in vcl/source/helper/commandinfoprovider.cxx.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner, SkippCpp -> SkillCpp, TopicCleanup) [NinjaEdit]
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit]
I am working on it.
Is this task only about making it static instead of singleton, keeping the same API, or we also should do something with SetFrame and dispose methods?
(In reply to Aleksas Pantechovskis from comment #5) > Is this task only about making it static instead of singleton, keeping the > same API, or we also should do something with SetFrame and dispose methods? I would just make all members of that class static and keep the SetFrame. We do not need the dispose method anymore. Or do we also want to remove all the caching from here? Not sure what would be the performance impact. @Maxim, any comments?
(In reply to Samuel Mehrbrodt (CIB) from comment #6) > I would just make all members of that class static and keep the SetFrame. We > do not need the dispose method anymore. Why is that? As long as the frame reference is static, we do need the dispose thing to ensure destructing at the right moment during shutdown. > Or do we also want to remove all the caching from here? Caching is apparently the reason of it being a singleton. As long as caching is needed, we should keep the singleton as well, so that frames can down to refcount of 0 and destructed when needed. > Not sure what would be the performance impact. But we pass the frame in each call anyway, so what's the point of caching it? That was the reason of why I suggested this refactoring in the first place. (Moreover the frame is mostly used to get the module id, so why not just pass the module id instead? It can be an optional arg. and empty one would mean "get from the current frame". That would make it possible to use in the new db wizard too.) Unless you can shed some light on the purpose of caching here? (I noticed now that accelerator configurations are also cached - maybe that's the reason? - surely those can be also refactored in some way to (optionally) get from the caller?)
Aleksis@ still working on this patch (otherwise please unsaying yourself) ?
Hi Samuel, I've submitted a patch corresponding to this issue: https://gerrit.libreoffice.org/#/c/31861/ . But it seems to be breaking a test case(CppunitTest_vcl_app_test). Could you please have a look at it?
(In reply to Rohan Kumar from comment #9) > Hi Samuel, > > I've submitted a patch corresponding to this issue: > https://gerrit.libreoffice.org/#/c/31861/ . But it seems to be breaking a > test case(CppunitTest_vcl_app_test). Could you please have a look at it? You have merge conflict in that patch, and a comment.
Rohan Kumar committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=33c51e8002f58847ba43bc111dabc49278e36daf tdf#95844 Refactor CommandInfoProvider It will be available in 5.4.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
@Samuel, Maxim : Should we move to Resolved now?
(In reply to Rohan Kumar from comment #12) > @Samuel, Maxim : Should we move to Resolved now? Absolutely. Thank you so much for your work on this bug!