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

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

Attachment: signature.asc
Description: Digital signature