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

_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Reply via email to