From 424cc287f15a271c1068d2d892de07a080d57701 Mon Sep 17 00:00:00 2001 From: Johan Dahlin Date: Fri, 7 Mar 2008 20:03:35 +0000 Subject: [PATCH] - Treat enums like enums and not values - Avoid invalid free, in case of 2008-03-07 Johan Dahlin * gtk/gtkbuilder.c: * gtk/gtkbuilderparser.c: * gtk/gtkbuilderprivate.h: * gtk/gtkiconfactory.c: * tests/buildertest.c: - Treat enums like enums and not values - Avoid invalid free, in case of more than two sources - Add better error messages - Add much improved tests (#520979, Christian Persch) svn path=/trunk/; revision=19732 --- ChangeLog | 13 +++++ gtk/gtkbuilder.c | 7 +-- gtk/gtkbuilderparser.c | 6 ++- gtk/gtkbuilderprivate.h | 4 ++ gtk/gtkiconfactory.c | 105 ++++++++++++++++++++++++++++++---------- tests/buildertest.c | 95 +++++++++++++++++++++++++++++++++++- 6 files changed, 196 insertions(+), 34 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6f1bf98cb3..cac78065bd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +2008-03-07 Johan Dahlin + + * gtk/gtkbuilder.c: + * gtk/gtkbuilderparser.c: + * gtk/gtkbuilderprivate.h: + * gtk/gtkiconfactory.c: + * tests/buildertest.c: + - Treat enums like enums and not values + - Avoid invalid free, in case of more than two sources + - Add better error messages + - Add much improved tests + (#520979, Christian Persch) + 2008-03-07 Carlos Garnacho * gtk/gtkiconfactory.c (gtk_icon_factory_buildable_custom_tag_end): diff --git a/gtk/gtkbuilder.c b/gtk/gtkbuilder.c index 11641035dc..dcea632844 100644 --- a/gtk/gtkbuilder.c +++ b/gtk/gtkbuilder.c @@ -50,11 +50,6 @@ static void gtk_builder_get_property (GObject *object, GParamSpec *pspec); static GType gtk_builder_real_get_type_from_name (GtkBuilder *builder, const gchar *type_name); -static gboolean _gtk_builder_enum_from_string (GType type, - const gchar *string, - gint *enum_value, - GError **error); - enum { PROP_0, @@ -1280,7 +1275,7 @@ gtk_builder_value_from_string_type (GtkBuilder *builder, return ret; } -static gboolean +gboolean _gtk_builder_enum_from_string (GType type, const gchar *string, gint *enum_value, diff --git a/gtk/gtkbuilderparser.c b/gtk/gtkbuilderparser.c index d6d69a46cf..1b56b39079 100644 --- a/gtk/gtkbuilderparser.c +++ b/gtk/gtkbuilderparser.c @@ -915,9 +915,13 @@ text (GMarkupParseContext *context, if (data->subparser && data->subparser->start) { + GError *tmp_error = NULL; + if (data->subparser->parser->text) data->subparser->parser->text (context, text, text_len, - data->subparser->data, error); + data->subparser->data, &tmp_error); + if (tmp_error) + g_propagate_error (error, tmp_error); return; } diff --git a/gtk/gtkbuilderprivate.h b/gtk/gtkbuilderprivate.h index 96f8fd4568..07d20e6d0d 100644 --- a/gtk/gtkbuilderprivate.h +++ b/gtk/gtkbuilderprivate.h @@ -118,6 +118,10 @@ void _free_signal_info (SignalInfo *info, gboolean _gtk_builder_boolean_from_string (const gchar *string, gboolean *value, GError **error); +gboolean _gtk_builder_enum_from_string (GType type, + const gchar *string, + gint *enum_value, + GError **error); gboolean _gtk_builder_flags_from_string (GType type, const char *string, guint *value, diff --git a/gtk/gtkiconfactory.c b/gtk/gtkiconfactory.c index 428aff2c87..3dfb855170 100644 --- a/gtk/gtkiconfactory.c +++ b/gtk/gtkiconfactory.c @@ -2752,20 +2752,23 @@ icon_source_start_element (GMarkupParseContext *context, gchar *stock_id = NULL; gchar *filename = NULL; gchar *icon_name = NULL; - GtkIconSize size = -1; - GtkTextDirection direction = -1; - GtkStateType state = -1; + gint size = -1; + gint direction = -1; + gint state = -1; IconFactoryParserData *parser_data; IconSourceParserData *source_data; - + gchar *error_msg; + GQuark error_domain; + parser_data = (IconFactoryParserData*)user_data; if (!parser_data->in_source) { if (strcmp (element_name, "sources") != 0) { - g_warning ("Unexpected element %s, expected ", element_name); - return; + error_msg = g_strdup_printf ("Unexpected element %s, expected ", element_name); + error_domain = GTK_BUILDER_ERROR_INVALID_TAG; + goto error; } parser_data->in_source = TRUE; return; @@ -2774,8 +2777,9 @@ icon_source_start_element (GMarkupParseContext *context, { if (strcmp (element_name, "source") != 0) { - g_warning ("Unexpected element %s, expected ", element_name); - return; + error_msg = g_strdup_printf ("Unexpected element %s, expected ", element_name); + error_domain = GTK_BUILDER_ERROR_INVALID_TAG; + goto error; } } @@ -2789,34 +2793,42 @@ icon_source_start_element (GMarkupParseContext *context, icon_name = g_strdup (values[i]); else if (strcmp (names[i], "size") == 0) { - if (!_gtk_builder_flags_from_string (GTK_TYPE_ICON_SIZE, - values[i], - &size, - error)) + if (!_gtk_builder_enum_from_string (GTK_TYPE_ICON_SIZE, + values[i], + &size, + error)) return; } else if (strcmp (names[i], "direction") == 0) { - if (!_gtk_builder_flags_from_string (GTK_TYPE_TEXT_DIRECTION, - values[i], - &direction, - error)) + if (!_gtk_builder_enum_from_string (GTK_TYPE_TEXT_DIRECTION, + values[i], + &direction, + error)) return; } else if (strcmp (names[i], "state") == 0) { - if (!_gtk_builder_flags_from_string (GTK_TYPE_STATE_TYPE, - values[i], - &state, - error)) + if (!_gtk_builder_enum_from_string (GTK_TYPE_STATE_TYPE, + values[i], + &state, + error)) return; } + else + { + error_msg = g_strdup_printf ("'%s' is not a valid attribute of <%s>", + names[i], "source"); + error_domain = GTK_BUILDER_ERROR_INVALID_ATTRIBUTE; + goto error; + } } if (!stock_id || !filename) { - g_warning (" requires a stock_id and a filename"); - return; + error_msg = g_strdup_printf (" requires a stock_id and a filename"); + error_domain = GTK_BUILDER_ERROR_MISSING_ATTRIBUTE; + goto error; } source_data = g_slice_new (IconSourceParserData); @@ -2828,6 +2840,33 @@ icon_source_start_element (GMarkupParseContext *context, source_data->state = state; parser_data->sources = g_slist_prepend (parser_data->sources, source_data); + return; + + error: + { + gchar *tmp; + gint line_number, char_number; + + g_markup_parse_context_get_position (context, + &line_number, + &char_number); + + tmp = g_strdup_printf ("%s:%d:%d %s", "input", + line_number, char_number, error_msg); +#if 0 + g_set_error (error, + GTK_BUILDER_ERROR, + error_domain, + tmp); +#else + g_warning (tmp); +#endif + g_free (tmp); + g_free (stock_id); + g_free (filename); + g_free (icon_name); + return; + } } static const GMarkupParser icon_source_parser = @@ -2886,6 +2925,7 @@ gtk_icon_factory_buildable_custom_tag_end (GtkBuildable *buildable, { icon_set = gtk_icon_set_new (); gtk_icon_factory_add (icon_factory, source_data->stock_id, icon_set); + gtk_icon_set_unref (icon_set); } icon_source = gtk_icon_source_new (); @@ -2900,18 +2940,26 @@ gtk_icon_factory_buildable_custom_tag_end (GtkBuildable *buildable, if (source_data->icon_name) gtk_icon_source_set_icon_name (icon_source, source_data->icon_name); if (source_data->size != -1) - gtk_icon_source_set_size (icon_source, source_data->size); + { + gtk_icon_source_set_size (icon_source, source_data->size); + gtk_icon_source_set_size_wildcarded (icon_source, FALSE); + } if (source_data->direction != -1) - gtk_icon_source_set_direction (icon_source, source_data->direction); + { + gtk_icon_source_set_direction (icon_source, source_data->direction); + gtk_icon_source_set_direction_wildcarded (icon_source, FALSE); + } if (source_data->state != -1) - gtk_icon_source_set_state (icon_source, source_data->state); + { + gtk_icon_source_set_state (icon_source, source_data->state); + gtk_icon_source_set_state_wildcarded (icon_source, FALSE); + } /* Inline source_add() to avoid creating a copy */ g_assert (icon_source->type != GTK_ICON_SOURCE_EMPTY); icon_set->sources = g_slist_insert_sorted (icon_set->sources, icon_source, icon_source_compare); - gtk_icon_set_unref (icon_set); g_free (source_data->stock_id); g_free (source_data->filename); @@ -2920,6 +2968,11 @@ gtk_icon_factory_buildable_custom_tag_end (GtkBuildable *buildable, } g_slist_free (parser_data->sources); g_slice_free (IconFactoryParserData, parser_data); + + /* TODO: Add an attribute/tag to prevent this. + * Usually it's the right thing to do though. + */ + gtk_icon_factory_add_default (icon_factory); } } diff --git a/tests/buildertest.c b/tests/buildertest.c index 65b90a99e7..f51fb1c5af 100644 --- a/tests/buildertest.c +++ b/tests/buildertest.c @@ -28,6 +28,17 @@ #include #include +/* Copied from gtkiconfactory.c; keep in sync! */ +struct _GtkIconSet +{ + guint ref_count; + GSList *sources; + GSList *cache; + guint cache_size; + guint cache_serial; +}; + + static GtkBuilder * builder_new_from_string (const gchar *buffer, gsize length, @@ -1805,8 +1816,45 @@ test_icon_factory (void) " " " " ""; + const gchar buffer2[] = + "" + " " + " " + " " + " " + " " + " " + ""; +#if 0 + const gchar buffer3[] = + "" + " " + " " + " " + ""; + const gchar buffer4[] = + "" + " " + " " + " " + " " + " " + ""; + const gchar buffer5[] = + "" + " " + " " + " " + " " + " " + ""; + GError *error = NULL; +#endif GObject *factory; GtkIconSet *icon_set; + GtkIconSource *icon_source; GtkWidget *image; builder = builder_new_from_string (buffer1, -1, NULL); @@ -1815,10 +1863,55 @@ test_icon_factory (void) icon_set = gtk_icon_factory_lookup (GTK_ICON_FACTORY (factory), "apple-red"); g_assert (icon_set != NULL); - gtk_icon_factory_add_default (GTK_ICON_FACTORY (factory)); image = gtk_image_new_from_stock ("apple-red", GTK_ICON_SIZE_BUTTON); g_assert (image != NULL); + + builder = builder_new_from_string (buffer2, -1, NULL); + factory = gtk_builder_get_object (builder, "iconfactory1"); + g_assert (factory != NULL); + + icon_set = gtk_icon_factory_lookup (GTK_ICON_FACTORY (factory), "sliff"); + g_assert (icon_set != NULL); + g_assert (g_slist_length (icon_set->sources) == 2); + + icon_source = icon_set->sources->data; + g_assert (gtk_icon_source_get_direction (icon_source) == GTK_TEXT_DIR_RTL); + g_assert (gtk_icon_source_get_state (icon_source) == GTK_STATE_ACTIVE); + g_assert (gtk_icon_source_get_size (icon_source) == GTK_ICON_SIZE_MENU); + g_assert (g_str_has_suffix (gtk_icon_source_get_filename (icon_source), "sloff.png")); + + icon_source = icon_set->sources->next->data; + g_assert (gtk_icon_source_get_direction (icon_source) == GTK_TEXT_DIR_LTR); + g_assert (gtk_icon_source_get_state (icon_source) == GTK_STATE_SELECTED); + g_assert (gtk_icon_source_get_size (icon_source) == GTK_ICON_SIZE_DND); + g_assert (g_str_has_suffix (gtk_icon_source_get_filename (icon_source), "slurf.png")); + + g_object_unref (builder); + +#if 0 + error = NULL; + gtk_builder_add_from_string (builder, buffer3, -1, &error); + g_assert (error != NULL); + g_assert (error->domain == GTK_BUILDER_ERROR); + g_assert (error->code == GTK_BUILDER_ERROR_INVALID_TAG); + g_error_free (error); + + error = NULL; + gtk_builder_add_from_string (builder, buffer4, -1, &error); + g_assert (error != NULL); + g_assert (error->domain == GTK_BUILDER_ERROR); + g_assert (error->code == GTK_BUILDER_ERROR_INVALID_TAG); + g_error_free (error); + + error = NULL; + gtk_builder_add_from_string (builder, buffer5, -1, &error); + g_assert (error != NULL); + g_assert (error->domain == GTK_BUILDER_ERROR); + g_assert (error->code == GTK_BUILDER_ERROR_INVALID_ATTRIBUTE); + g_error_free (error); +#endif + } static void