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

[Tomeu Vizoso]

On Wed, Apr 22, 2009 at 18:49, Rob Savoye <address@hidden> wrote:
> I doubt you're one this list for commits to Gnash. Some of these issues
> I just fixed by renaming things.

Thanks, just got subscribed. Some comments inline below.

>        - rob -
>
> -------- Original Message --------
> Subject: Re: [Gnash-commit] /srv/bzr/gnash/trunk r10819: add Tomeu
> Vizoso's patch to make Gnash be a gtk widget and python module.
> Date: Wed, 22 Apr 2009 14:53:38 +0200
> From: Benjamin Wolsey <address@hidden>
> To: address@hidden
> CC: address@hidden
> References: <address@hidden>
>
> 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.

Should be fixed now.

> 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.

Ok, so Gnash code inside Gtk widgets should follow Gnash conventions
and the Gtk stuff Gtk coding conventions, right?

> 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.

Yeah, I agree all file names in gui should follow Gnash naming conventions.

> 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.

Yeah, gtk_widget_set_size_request is evil and shouldn't be used. Will
give it a look.

>> +
>> +#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.

It's a Gtk+ convention, can change it to GNASH_CANVAS_H.

>> +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.

What do you suggest here? It's also boilerplate code in Gtk+, but I
don't think stuff will break by changing this one, though other
conventions are required in order for the Gtk+ macros to do its magic.

>> +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.

Ok, will check if anything breaks.

>> +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.

I'm quite ignorant of C++, what type of references would you use
there? All those members are private and owned by GnashCanvas.

> And preferably the auto_ptrs should be private members with accessors.

This isn't a C++ class but a struct containing the private stuff of a
Gtk widget.

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

I guess I should take that out.

Thanks!

Tomeu

> bwy
>
> --
> The current release of Gnash is 0.8.5
> http://www.gnu.org/software/gnash/
>
> Benjamin Wolsey, Software Developer - http://benjaminwolsey.de
>
>