On 01/13/2012 04:14 PM, Michael Meeks wrote:
On Fri, 2012-01-13 at 14:15 +0100, Stephan Bergmann wrote:
UNO *is* the tool to make functionality available to different
languages, to extensions, and to scripting facilities.
Sure - of course. This API, on the other hand, is a C++ syntactic
sugar, so surely we can do things better in our native language
perhaps ? at least I think taking an efficiency hit to allow the
possibility of a later non-C++ configmgr implementation is unlikely to
pay dividends.
For me, its more of an "avoid an additional interface" than "fail to
improve efficiency of C++ code" thing. Also, its not entirely a C++
coating only, parts of it can benefit other UNO bindings too (and at
least in principle the type safe wrappings around the configuration
items could also be moved to plain UNOIDL).
I'd not seen any of this, so I'm just looking for the first time; I
have a number of queries (given that this will/should be used
everywhere).
* reading:
workdir/unxlngi6.pro/CustomTarget/officecfg/registry/Office/Common.hxx
I love the:
namespace officecfg { namespace Office { namespace Common {
ie. we skipped the org and the OpenOffice - which is cool; good
to get over the over-namespacing legacy. My question would be -
do we even need the 'Office' ? ;-) but ... it's prolly fine for
now.
It strictly skips only the never-varying prefix "org/openoffice" (there
are components like "Inet" and "Setup" next to "Office").
* string construction:
struct UseOldExport: public unotools::ConfigurationProperty<UseOldExport, bool> {
static rtl::OUString path() { return
rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("/org.openoffice.Office.Common/InternalMSExport/UseOldExport"));
}
Since we're going to get a lot of these inserted, it might be
rather a good plan to split a path() method from a key()
method so the compiler can share the:
/org.openoffice.Office.Common/.../
for all the keys; that makes the call site slightly larger, but
the resulting binary potentially rather smaller :-)
it'd of course also be rather nice to keep the strings
as const char *'s until we get them into some non-inlined code
hiding that construction stuff inside some
static ConfigurationWrapper::getPropertyValue(...) type method ?
That way we get a single string (buffer) construction to build
the path instead of one per call-site.
There's room for improvement, for sure. I'd save that for when larger
chunks of client code have been adapted to actually use it. (The nice
thing is that optimizations like the above can be introduced completely
transparently and without the need to worry about any compatibility.)
* XComponentContext-age ...
static ConfigurationWrapper const& get(
com::sun::star::uno::Reference< com::sun::star::uno::XComponentContext>
const& context);
Do we really need to pass this parameter to these methods ?
We have a single configmr instance, it seems unlikely that we
really need anything to help us find it surely ? either
configmgr is there, or the world goes bang :-)
If we're desparate to have it (it'd be nice to know what for),
then we could we have a default-to-NULL pointer to a reference
there ?
The UNO concept is to thread the (one and only) component context
instance through the code, and in this world view
comphelper::getProcessComponentContext is considered a hack to be
cleaned up. Which one is more practical is surely debatable. (For the
case at hand, calling comphelper::getProcessComponentContext internally
in get() rather than passing in XComponentContext would evidently be
less typing at the typical client side -- but then again, think about
unit tests and the virtues of reducing global variables.) I did
hesitate before making the parameter explicit, but then decided "in
dubio for clean concepts."
Anyhow - we should clearly create an EasyHack to drive this goodness
all through the code-base, I suspect it's highly susceptible to
wide-spread volunteer effort. Are you happy enough with it yet to do
that ?
As I already wrote: "I haven't announced this new C++ API yet, as some
issues about change-notification are not yet completely thought out for
it. Shame on me, should really do that soon."
Stephan
Context
Privacy Policy |
Impressum (Legal Info) |
Copyright information: Unless otherwise specified, all text and images
on this website are licensed under the
Creative Commons Attribution-Share Alike 3.0 License.
This does not include the source code of LibreOffice, which is
licensed under the Mozilla Public License (
MPLv2).
"LibreOffice" and "The Document Foundation" are
registered trademarks of their corresponding registered owners or are
in actual use as trademarks in one or more countries. Their respective
logos and icons are also subject to international copyright laws. Use
thereof is explained in our
trademark policy.