Re: [PATCH] fdo#35973 - [EasyHack] Remember the state of the sidebar pane in Impress

Hi Tor,

Op 28-05-12 13:23, Tor Lillqvist schreef:
> I am sorry but I still have some critique of the patch... Please don't
> take this too harshly.
it's ok.
> Is this use of "IsDisposing" state variable really necessary? Or is
> that band-aid protection against some bug (mismanagement of
> dynamically allocated objects) elsewhere?
I created it because otherwise it would save the visibility-states 
several times.
Also because for each view the bar is set to invisible before disposing 
I added a boolean if it is already saved.
> 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?
I copied this code from miscopts-class. I didn't change the workings as 
much.
I'll see what can be changed.
> 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?
I'll change it to SlideSorter-toolbar
> if (SvtSlideSorterBarOptions().GetVisibleImpressView()==sal_True)
>
> should surely be written as
>
> if (SvtSlideSorterBarOptions().GetVisibleImpressView()) , etc
ok.
> 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.
I removed all ascii-art as you asked earlier. I didn't know Doxygen.
> 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?
I'll remove that
> 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.
ok
> 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?
i think so.
> --tml