Re: [Libreoffice-commits] core.git: 3 commits - sd/source slideshow/source

On Tuesday, April 15, 2014 23:15 BST, Thorsten Behrens <thb@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.zolnai@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.zolnai@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