Re: [Gnash-commit] /srv/bzr/gnash/trunk r10819: add Tomeu Vizoso's patch to make Gnash be a gtk widget and python module.

[Benjamin Wolsey]

I don't think this code is yet quite ready to be used, particularly not
as the default. It looks like it does lots of good things, but breaks
gtk-gnash completely for me. It seems that only GnashCanvas is used in
the normal gtk gui, so that's the only code that's a problem for me at
the moment. 

The patch doesn't use Gnash's coding or naming style
(http://wiki.gnashdev.org/CodingStyle) and is (in a small number of
places) ill-formed C++. I see that the gtk widget should keep to the gtk
naming style for consistency with their API, but not the bits that are
only Gnash-related.

gnash-canvas.{h,cpp} introduces yet another file naming style. Can we
stick to either gnash_canvas.cpp or GnashCanvas.cpp? The latter reflects
the class name, and has been used for recent code.

One bug from current Gnash that the GnashCanvas may be able to fix is
the size_request one. A normal GtkWidget can only be given a requested
size by calling gtk_widget_set_size_request, but then it keeps this as a
minimum size so the widget can't made smaller. It would be good if Gnash
could set the correct initial size on the GnashCanvas but still allow
shrinking. I see gnash_canvas_size_allocate does something like this
already, but can't yet test to see what it does.

> +
> +#ifndef __GNASH_CANVAS_H__
> +#define __GNASH_CANVAS_H__
> +
This style of header guard is not allowed in C++ (though they do keep
getting added to Gnash). GNASH_CANVAS_H is fine.

> +typedef struct _GnashCanvas            GnashCanvas;
> +typedef struct _GnashCanvasClass       GnashCanvasClass;

C++ names aren't allowed to start with _ in the global namespace or ever
with a _ followed by an upper-case letter.
 
> +struct _GnashCanvasClass {
> +    GtkDrawingAreaClass base_class;
> +};
> +

I don't see any need to use the C-style "typedef struct _GnashCanvas {}
GnashCanvas" either, so "struct GnashCanvas {};" is better.

> +struct _GnashView {
> +     GtkBin base_instance;
> +
> +    GnashCanvas *canvas;
> +
> +     std::auto_ptr<gnash::media::MediaHandler> media_handler;
> +    boost::shared_ptr<gnash::sound::sound_handler> sound_handler;
> +
> +    /// Handlers (for sound etc) for a libcore run.
> +    //
> +    /// This must be kept alive for the entire lifetime of the movie_root
> +    /// (currently: of the Gui).
> +    std::auto_ptr<gnash::RunInfo> run_info;
> +
> +    std::auto_ptr<gnash::movie_definition> movie_definition;
> +    std::auto_ptr<gnash::movie_root> stage;
> +    std::auto_ptr<gnash::SystemClock> system_clock;
> +    std::auto_ptr<gnash::InterruptableVirtualClock> virtual_clock;
> +    guint advance_timer;
> +};

Storing auto_ptrs as public members of a struct seems to be inviting
disaster, as copying them will set the original pointer to null. If the
struct isn't designed to be copied, it should be non-copyable, or the
default cctor will reset all the std::auto_ptr members in the original.
And preferably the auto_ptrs should be private members with accessors.

> +    gnash::gnashInit();
> +    gnash::LogFile& dbglogfile = gnash::LogFile::getDefaultInstance();
> +    dbglogfile.setVerbosity();
> +    dbglogfile.setVerbosity();
> +    dbglogfile.setVerbosity();

dbglogfile.setVerbosity(3).

bwy

--
The current release of Gnash is 0.8.5
http://www.gnu.org/software/gnash/

Benjamin Wolsey, Software Developer - http://benjaminwolsey.de

signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil