Created attachment 111924 [details] [review]
like this?

Btw, totemplparser-marshal.[ch] are generated files, but also are in svn. Shouldn't they not be in svn?

(In reply to comment #2)
> Btw, totemplparser-marshal.[ch] are generated files, but also are in svn.
> Shouldn't they not be in svn?

No idea how they got in there. You can remove them.

(In reply to comment #1)
> Created an attachment (id=111924) [edit]
> like this?
> 

Sorry, no.  It does not help much to know it is a hash table.  What type are keys and values?  Could be anything, not just char* -> char*...  I can't assume anything.  And pygobject does not allow per-signal-parameter conversion functions, only per-GType conversion functions.

It would be better to register a TOTEM_TYPE_METATADA or TOTEM_TYPE_STR_TO_STR_HASH_TABLE type, which be otherwise identical to G_TYPE_HASH_TABLE.

(In reply to comment #4)
> (In reply to comment #1)
> > Created an attachment (id=111924) [edit]
> > like this?
> > 
> 
> Sorry, no.  It does not help much to know it is a hash table.  What type are
> keys and values?  Could be anything, not just char* -> char*...  I can't assume
> anything.  And pygobject does not allow per-signal-parameter conversion
> functions, only per-GType conversion functions.
> 
> It would be better to register a TOTEM_TYPE_METATADA or
> TOTEM_TYPE_STR_TO_STR_HASH_TABLE type, which be otherwise identical to
> G_TYPE_HASH_TABLE.

Currently, all the types available outside the internal API will be strings, but there's no telling that this will stay that way...

(In reply to comment #4)
> (In reply to comment #1)
> > Created an attachment (id=111924) [edit]
> > like this?
> Sorry, no. 
Ok. However I think the patch is correct and should be applied, even if it doesn't fix this particular problem.

	* plparse/totem-pl-parser.c: (totem_pl_parser_class_init),
	(totem_pl_parser_add_url_valist):
	* plparse/totemplparser-marshal.list: Use G_TYPE_HASH_TABLE in the
	signals to transport the hash table, and use g_hash_table_unref
	instead of g_hash_table_destroy so that the signal handler may
	usefully keep a reference to the hash table. Patch from bug #536060.

(In reply to comment #7)
>         * plparse/totem-pl-parser.c: (totem_pl_parser_class_init),
>         (totem_pl_parser_add_url_valist):
>         * plparse/totemplparser-marshal.list: Use G_TYPE_HASH_TABLE in the
>         signals to transport the hash table, and use g_hash_table_unref
>         instead of g_hash_table_destroy so that the signal handler may
>         usefully keep a reference to the hash table. Patch from bug #536060.
> 

Would you accept a patch from me that registered a boxed type TOTEM_TYPE_PL_PARSER_METADATA, otherwise identical to G_TYPE_HASH_TABLE, and use it for that signal?  That would allow me to close the gnome-python-desktop bug (and this one too).

(In reply to comment #8)
<snip>
> Would you accept a patch from me that registered a boxed type
> TOTEM_TYPE_PL_PARSER_METADATA, otherwise identical to G_TYPE_HASH_TABLE, and
> use it for that signal?  That would allow me to close the gnome-python-desktop
> bug (and this one too).

Sure, if you think that's going to fix the problem...

Created attachment 113642 [details] [review]
patch to replace G_TYPE_HASHTABLE

+  static GType type_id = 0;
+  if (!type_id)

Should use g_once_init_enter/leave here, just like any other G_DEFINE_TYPE does.

Why don't you derive this new type from G_TYPE_HASHTABLE instead of just from G_TYPE_BOXED ?

Is this a totem-pl-parser specific problem, or does this sort of problem occur in other places too? I think there ought to be gobject support for registering the key and value GTypes of a G_TYPE_HASHTABLE derived type...

(In reply to comment #11)
> Why don't you derive this new type from G_TYPE_HASHTABLE instead of just from
> G_TYPE_BOXED ?

Hmm, so apparently it's not possible to further derive a type from a boxed type, at least none that I can see that would also do the extra work that g_boxed_type_register_static does... 

(In reply to comment #11)
> +  static GType type_id = 0;
> +  if (!type_id)
> 
> Should use g_once_init_enter/leave here, just like any other G_DEFINE_TYPE
> does.

Hm.. well, I copied the type registration code straight from GLib, g_hash_table_get_type, with only some renaming being done.  But I'll do it anyway.

> 
> Why don't you derive this new type from G_TYPE_HASHTABLE instead of just from
> G_TYPE_BOXED ?

Apparently not possible.

> 
> Is this a totem-pl-parser specific problem, or does this sort of problem occur
> in other places too? I think there ought to be gobject support for registering
> the key and value GTypes of a G_TYPE_HASHTABLE derived type...
> 

For Python bindings, it is impossible to do anything with G_TYPE_HASHTABLE, anywhere, because G_TYPE_HASHTABLE does not completely define the type, so it cannot be safely interpreted unless further information is provided to the signal marshaling code.

Well, it's not strictly impossible, but very hard, as it would require pygobject core changes to pass the signal class and signal ID, and would require registration of conversion functions on a (Object GType, Signal ID, Para GType) tuple basis.  But why make things so complicated on language bindings... :P

Created attachment 113647 [details] [review]
patch v2, uses g_once_enter/leave

I'd rather you added casts to the functions in g_boxed_type_register_static() and did away with the one-line function above in your patch.

Looks good to commit after that.

Fixed it myself.

2008-06-29  Gustavo J. A. M. Carneiro  <gjc@gnome.org>

	* plparse/totem-pl-parser.c (totem_pl_parser_metadata_get_type),
	* plparse/totem-pl-parser.h: Define a new
	TOTEM_TYPE_PL_PARSER_METADATA, identical to G_TYPE_HASH_TABLE; Use
	it instead of G_TYPE_HASH_TABLE so that Python bindings can safely
	convert metadata signal parameters.  Closes #536060.