Re: [Libreoffice-commits] core.git: new uno sidebar api tdf#91806

Hi Stephan

Thanks a lot for your review

> I'm sorry this is a little late.  But I think we're still fine, as this
> should until now only have hit master towards LO 5.1, and not any
> libreoffice-5-0 or earlier.

right

I'll correct all of your points
with (@since LibreOffice 5.1)

remarks/questions below
feel free if anything not clear

Thanks again Stephan, i'll submit a new patch and let you know

Laurent

> > diff --git a/offapi/com/sun/star/ui/XDeck.idl
> > b/offapi/com/sun/star/ui/XDeck.idl
...
> > + module com {  module sun {  module star {  module ui {
> > +
> > +/** provides access to Desk */
>
> What does the above mean?  And please add a @since tag here.

--> probably typo, it was meant Decks. Is it clearer ?


> > +    */
> > +    void setTitle( [in] string newTitle );
>
> Is setTitle necessary and/or useful?  (At least, none of the code in
> this commit appears to use it.)

--> it allow changing the Deck title (as named)
--> in UnoDeck.hxx 
http://opengrok.libreoffice.org/xref/core/include/sfx2/sidebar/UnoDeck.hxx#40
--> do i miss somehing ?


> > +    /** Activate the deck and isplays its content
>
> Typo "isplays".


> > +    void setOrderIndex( [in] long newOrderIndex );
>
> Is setOrderIndex necessary and/or useful? (At least, none of the code in
> this commit appears to use it.)  Is setOrderIndex(0) the same as
> moveFirst()?

first, have to say that only rely of existing implementation.

unfortunatelly, the Decks and panels are global to libreoffice
that means that 2 panels or desk can't have same order index (or at 
least i did not test that case regarding the existing. I may verify if 2 
panels or Decks can have same orderIndex)
setting setOrderIndex(0) as movreFirst() on one panel, the the other 
would disturb non displayed panels (even on non visible decks) or would 
require to re-arrange all the Decks/Panels each time

i personnaly do not like this architecture despite i understand the 
reusability goal. i think there are cleanir things to be done (but as a 
first round i did not want to destray all the existing structure)

> > +
> > +interface XDecks
> > +
> > +{
> > +    interface com::sun::star::container::XIndexAccess;
> > +    interface com::sun::star::container::XNameAccess;
>
> The downside of reusing such generic interfaces is that it doesn't make
> it clear that the returned ANYs are actually of type XDeck (I assume).
> So at least document that, or, if the full set of XIndex+XNameAccess is
> not really necessary and/or useful, consider replacing this with a
> (handful of) more specific method(s).

XIndexAccess may not be that useful (and may be cinfusing with orderIndex)
XNameAccess is usefull
will document and may be replace XIndexAccess

> > +
> > +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
> > \ No newline at end of file
> > diff --git a/offapi/com/sun/star/ui/XPanel.idl
> > b/offapi/com/sun/star/ui/XPanel.idl
> > +
> > + module com {  module sun {  module star {  module ui {
> > +
> > +/** provides access to Desk */
>
> What does the above mean?  And please add a @since tag here.

typo, should read Panel

> There is also css.ui.XSidebarPanel; can you clarify why there's two?

yep, will do. "old" API
this returns ui::XUIElement getRealInterFace() which is the panel 
graphical content, not the Panel object itself