On Tuesday, April 15, 2014 23:15 BST, Thorsten Behrens <t...@documentfoundation.org> wrote: > Zolnai Tamás wrote: > > --- a/slideshow/source/engine/shapes/appletshape.cxx > > +++ b/slideshow/source/engine/shapes/appletshape.cxx > > @@ -96,11 +96,11 @@ namespace slideshow > > virtual bool implRender( const ::basegfx::B2DRange& > > rCurrBounds ) const SAL_OVERRIDE; > > virtual void implViewChanged( const UnoViewSharedPtr& rView ) > > SAL_OVERRIDE; > > virtual void implViewsChanged() SAL_OVERRIDE; > > - virtual bool implStartIntrinsicAnimation() SAL_OVERRIDE; > > - virtual bool implEndIntrinsicAnimation() SAL_OVERRIDE; > > - virtual bool implPauseIntrinsicAnimation() SAL_OVERRIDE; > > - virtual bool implIsIntrinsicAnimationPlaying() const > > SAL_OVERRIDE; > > - virtual void implSetIntrinsicAnimationTime(double) > > SAL_OVERRIDE; > > + virtual void play() SAL_OVERRIDE; > > + virtual void stop() SAL_OVERRIDE; > > + virtual void pause() SAL_OVERRIDE; > > + virtual bool isPlaying() const SAL_OVERRIDE; > > + virtual void setMediaTime(double) SAL_OVERRIDE; > > > There's some point in having all those methods go through the base > class first - you have exactly one central place to check invariants, > log stuff, stick breakpoints in etc. It is now also needlessly > deviating from implViewsChanged() and others. C.f. NVI pattern. > > > commit 2a594eb22bfed62fdbcef51a56c2c180bea0283f > > Author: Zolnai Tam??s <tamas.zol...@collabora.com> > > Date: Mon Apr 14 16:23:06 2014 +0200 > > > > Slideshow: Remove unneded ExternalMediaShape > > > > ExternalShapeBase is the base class of MediaShape and > > AppletShape so it's nonsense that ExternalMediaShape > > to be the base of ExternalShapeBase. > > Actually this class does nothing, anyway. > > > Well - it *did* provide a minimal interface for "media shapes" to > calling code. What client code now sees is ExternalShapeBase, that > drags in a chunk of private member definitions, irrelevant e.g. to the > code in AnimationCommandNode. Maybe naming it IExternalMediaShape > would have made my intention clearer. ;) > > If this is about the extra vtable - there's SAL_NO_VTABLE. > > Zolnai Tamás wrote: > > commit 50b60c5508b3ba5a0b8dc05eac511d7edaa5a343 > > Author: Zolnai Tam??s <tamas.zol...@collabora.com> > > Date: Tue Apr 15 22:23:42 2014 +0200 > > > > Slideshow: these methods override public methods > > > > So make them public too. > > > That is true, but for leaf classes that are supposed to be used via > polymorphic pointers to their base, it is still idiomatic to have > overridden virtuals to be private. In this case, there's really no > danger in anyone using this implementation class - it is defined and > visible only in this one cxx file. > > Cheers, > > -- Thorsten
Hi Thorsten, Thanks for the information, it seems I need some to learn. :) I reverted these changes and just rename ExternalMediaShape which was confusing for me. Regards, Tamás _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice