I am sorry but I still have some critique of the patch... Please don't take this too harshly.
Is this use of "IsDisposing" state variable really necessary? Or is that band-aid protection against some bug (mismanagement of dynamically allocated objects) elsewhere? The use of the GetPropertyNames() static method seems a bit overly complicated and duplicate to me; Is it really necessary to have this method *and* local variables here and there initialised by calling it? Could this be done in some simpler way? Can't this table just be a static variable in the source file? The SvtSlideSorterBarOptions::m_nRefCount thing seems a bit odd. The use of the "reference count" term is misleading, it doesn't correspond to what is normally thought of as reference counting, does it? (And if it does, then we already have some mechanisms in LO to build reference countig upon, no need to write your own.) Is it really necessary to "hide" simple boolean fields behind public setters and getters? Couldn't the fields just be made public instead? The comments "We implement these class with a refcount mechanism! Every instance of this class increase it at create and decrease it at delete time - but all instances use the same data container! He is implemented as a static member ..." , "We use these class as internal member to support small memory requirements. You can create the container if it is neccessary. The class which use these mechanism is faster and smaller then a complete implementation!" and "impl. data container as dynamic pointer for smaller memory requirements!" make me suspect you attempt some questionable micro-optimisation. I.e. make the code a lot more complex for dubious gain in speed and size. The use of naming like _impl suffix easily misleads the code reader into thinking that it is the common Pimpl idiom that is used, while in fact it is something else, a dynamically allocated block of static variables? Or am I missing something? Anyway, what is the real gain here? Does other similar code in LO use similar constructs? The rest is just style issues, if you can improve that too, that would be great. The capitalization of the description texts added to officecfg/registry/schema/org/openoffice/Office/Impress.xcs is a bit odd. Is the user and translators supposed to understand the difference betwee "Slide Sorter" with a space and "SlideSorter" without space? if (SvtSlideSorterBarOptions().GetVisibleImpressView()==sal_True) should surely be written as if (SvtSlideSorterBarOptions().GetVisibleImpressView()) , etc The style of Doxygen (?) comments added is inconsistent; does /*@short really work, when elsewhere you seem to use /** @short etc? Sometimes you put the terminating */ right after the last word, sometimes on a line by itself. Why lots of spaces in front of semicolons in some places, to align semicolons, but not consistently, and that is not something LO code does elsewhere, is it? OUString has a constructor that takes a string literal, doesn't it, no need to use the RTL_CONSTASCII_USTRINGPARAM stuff I think. In newly written code we shouldn't use sal_Bool and sal_Int32 unless necessary; just bool and int is fine. It is not clear who "we" and "you" in comments refer to. "we" = the class in which the comment is located, "you" = code of subclasses? --tml _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
