- Treat enums like enums and not values - Avoid invalid free, in case of

2008-03-07  Johan Dahlin  <johan@gnome.org>

    * 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

@@ -1,3 +1,16 @@
+2008-03-07  Johan Dahlin  <johan@gnome.org>
+
+	* 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  <carlos@imendio.com>
 
 	* gtk/gtkiconfactory.c (gtk_icon_factory_buildable_custom_tag_end):

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,

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;
     }
 

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,

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;
+  gint size = -1;
-  GtkTextDirection direction = -1;
+  gint direction = -1;
-  GtkStateType state = -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 <sources>", element_name);
+	  error_msg = g_strdup_printf ("Unexpected element %s, expected <sources>", element_name);
-	  return;
+	  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 <source>", element_name);
+	  error_msg = g_strdup_printf ("Unexpected element %s, expected <source>", element_name);
-	  return;
+	  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,
+          if (!_gtk_builder_enum_from_string (GTK_TYPE_ICON_SIZE,
-					       values[i],
+                                              values[i],
-					       &size,
+                                              &size,
-					       error))
+                                              error))
 	      return;
 	}
       else if (strcmp (names[i], "direction") == 0)
 	{
-	  if (!_gtk_builder_flags_from_string (GTK_TYPE_TEXT_DIRECTION,
+          if (!_gtk_builder_enum_from_string (GTK_TYPE_TEXT_DIRECTION,
-					       values[i],
+                                              values[i],
-					       &direction,
+                                              &direction,
-					       error))
+                                              error))
 	      return;
 	}
       else if (strcmp (names[i], "state") == 0)
 	{
-	  if (!_gtk_builder_flags_from_string (GTK_TYPE_STATE_TYPE,
+          if (!_gtk_builder_enum_from_string (GTK_TYPE_STATE_TYPE,
-					       values[i],
+                                              values[i],
-					       &state,
+                                              &state,
-					       error))
+                                              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 ("<source> requires a stock_id and a filename");
+      error_msg = g_strdup_printf ("<source> requires a stock_id and a filename");
-      return;
+      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);
     }
 }
 

tests/buildertest.c

@@ -28,6 +28,17 @@
 #include <gdk/gdkkeysyms.h>
 #include <gtk/gtkprintjob.h>
 
+/* 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)
     "    </sources>"
     "  </object>"
     "</interface>";
+  const gchar buffer2[] =
+    "<interface>"
+    "  <object class=\"GtkIconFactory\" id=\"iconfactory1\">"
+    "    <sources>"
+    "      <source stock-id=\"sliff\" direction=\"rtl\" state=\"active\""
+    "              size=\"menu\" filename=\"sloff.png\"/>"
+    "      <source stock-id=\"sliff\" direction=\"ltr\" state=\"selected\""
+    "              size=\"dnd\" filename=\"slurf.png\"/>"
+    "    </sources>"
+    "  </object>"
+    "</interface>";
+#if 0
+  const gchar buffer3[] =
+    "<interface>"
+    "  <object class=\"GtkIconFactory\" id=\"iconfactory1\">"
+    "    <invalid/>"
+    "  </object>"
+    "</interface>";
+  const gchar buffer4[] =
+    "<interface>"
+    "  <object class=\"GtkIconFactory\" id=\"iconfactory1\">"
+    "    <sources>"
+    "      <invalid/>"
+    "    </sources>"
+    "  </object>"
+    "</interface>";
+  const gchar buffer5[] =
+    "<interface>"
+    "  <object class=\"GtkIconFactory\" id=\"iconfactory1\">"
+    "    <sources>"
+    "      <source/>"
+    "    </sources>"
+    "  </object>"
+    "</interface>";
+  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