Re: couple gtktreedatalist.c comments

[jrb redhat com]

Tim Janik <timj gtk org> writes:

> hey jrb,
> 
> i saw a few issues while browsing you union storage stuff in gtktreedatalist.c:

Ugh.  I rewrote this code this afternoon, and committed it a couple of
hours ago, so a lot of your comments can probably be replaced by a
different list of things I did wrong.  Nevertheless, I'll comment. (-:

> /* node allocation
>  */
> struct _GAllocator /* from gmem.c */
> {
>   gchar           *name;
>   guint16          n_preallocs;
>   guint            is_unused : 1;
>   guint            type : 4;
>   GAllocator      *last;
>   GMemChunk       *mem_chunk;
>   GtkTreeDataList *free_nodes;
> };
> 
> 
> G_LOCK_DEFINE_STATIC (current_allocator);
> static GAllocator *current_allocator = NULL;
> 
> do _not_ do this,
> 1) GAllocator is private in GLib for a reason
> 2) GAllocator is just there for the glib functions that support it
> 3) will become obsolete pretty soon anyways
> 
> smply use a memchunk like all other code does as well and
> be done with it.

Okay.  Will change this.  I filed a bug (#51590)

> void
> _gtk_tree_data_list_free (GtkTreeDataList *list,
>                           GType           *column_headers)
> {
> ...
> 
> i didn't investigate too deep, but aren't you missing boxed here?
> also, you have to switch on G_TYPE_FUNDAMENTAL(column_headers [i])
> or you can forget about any kind of derived type (GTK_TYPE_IDENTIFIER,
> G_TYPE_CLOSURE, GTK_TYPE_WIDGET...) for columns.

Yup.  Added in the new code.

> void
> _gtk_tree_data_list_value_to_node (GtkTreeDataList *list,
>                                    GValue          *value)
> {
>   switch (value->g_type)
>     {
>     case G_TYPE_BOOLEAN:
>       list->data.v_int = g_value_get_boolean (value);
>       break;
>     case G_TYPE_CHAR:
>       list->data.v_char = g_value_get_char (value);
>       break;
>     case G_TYPE_UCHAR:
>       list->data.v_uchar = g_value_get_uchar (value);
>       break;
>     case G_TYPE_INT:
>       list->data.v_int = g_value_get_int (value);
>       break;
>     case G_TYPE_UINT:
>       list->data.v_uint = g_value_get_uint (value);
>       break;
>     case G_TYPE_POINTER:
>       list->data.v_pointer = g_value_get_pointer (value);
>       break;
>     case G_TYPE_FLOAT:
>       list->data.v_float = g_value_get_float (value);
>       break;
>     case G_TYPE_STRING:
>       list->data.v_pointer = g_value_dup_string (value);
>       break;
>     default:
>       if (g_type_is_a (G_VALUE_TYPE (value), G_TYPE_OBJECT))
>         list->data.v_pointer = g_value_dup_object (value);
>       else if (g_type_is_a (G_VALUE_TYPE (value), G_TYPE_BOXED))
>         list->data.v_pointer = g_value_dup_boxed (value);
>       else
>         g_warning ("Unsupported type (%s) stored.", g_type_name (value->g_type));
>       break;
>     }
> }
> 
> since GValue*value is supplied by the user, it should be const,
> and you definitely need to switch on G_TYPE_FUNDAMENTAL (G_VALUE_TYPE(value)),
> user values will get passed in as GTK_TYPE_WIDGET...
> also, value->g_type is _private_, just use G_VALUE_TYPE() and nothing
> else to figure a values type.
> also, it'd be good to support G_TYPE_DOUBLE as well, for model frontends
> that e.g. use vararg interfaces, all values are supplied as double (default
> promotion in ANSI C) there's no good reason to enfore those to do lossage
> prone conversions to float due to a limitation in the model.
> also, G_TYPE_OBJECT and G_TYPE_BOXED are fundamental types, that means
> they are enum IDs and you can directly switch() on them.
> 
> as a side note, though you won't need that after you switch on fundamental IDs,
> you can use G_VALUE_HOLDS (value, GTK_TYPE_WIDGET) instead of the is_a checks.
> for the fundamentals, there are even compund macros G_VALUE_HOLDS_BOXED(value)
> etc...

It's not necessarily supplied by the user -- it's normally generated by
gtk_list_store_set... but point taken.  I removed the switch statement
and am using g_type_is_a () instead.  Are the G_VALUE_HOLDS functions a
better idea?

Additionally, I added G_TYPE_DOUBLE this afternoon.

> 
> upon fnction entries that take values, such as:
> 
> void
> gtk_list_store_set_cell (GtkListStore *list_store,
>                          GtkTreeIter  *iter,
>                          gint          column,
>                          GValue       *value)
> {
>   GtkTreeDataList *list;
>   GtkTreeDataList *prev;
>   GtkTreePath *path;
> 
>   g_return_if_fail (list_store != NULL);
>   g_return_if_fail (GTK_IS_LIST_STORE (list_store));
>   g_return_if_fail (iter != NULL);
>   g_return_if_fail (column >= 0 && column < list_store->n_columns);
> 
>   prev = list = G_SLIST (iter->user_data)->data;
> 
>   while (list != NULL)
>     {
>       if (column == 0)
>         {
>           path = gtk_list_store_get_path (GTK_TREE_MODEL (list_store), iter);
>           _gtk_tree_data_list_value_to_node (list, value);
> 
> 
> you should put:
> g_return_if_fail (G_IS_VALUE (value));

I'm testing for NULL here, but you're right.

> unless you also know the type that you expect, e.g. for string you can use
> g_return_if_fail (G_VALUE_HOLDS_STRING (value));
> right away, the macros will deal with NULL values, they are there to be
> used in return_if_fail statements.

I used:
        g_return_if_fail (g_type_is_a (G_VALUE_TYPE (type), list_store->column_headers[column]));

again here.  Is that Okay?

If you have a few seconds to look at the new stuff, that would be great.

Thanks,
-Jonathan