GNOME Bugzilla – Bug 426345
Critical errors when setting GdkColor properties values to NULL
Last modified: 2010-12-15 18:43:21 UTC
it seems there are problems when loading the gnome-druid pages
I can confirm this bug.
*** Bug 439296 has been marked as a duplicate of this bug. ***
*** Bug 429613 has been marked as a duplicate of this bug. ***
*** Bug 448615 has been marked as a duplicate of this bug. ***
stacktrace: 0x00002b49f50f26fc in gnome_druid_page_standard_set_property (object=<value optimized out>, prop_id=5, value=0x7fffbc7759f0, pspec=0x7008f0) at gnome-druid-page-standard.c:388 388 gnome-druid-page-standard.c: No such file or directory. in gnome-druid-page-standard.c (gdb) backtrace
+ Trace 141762
*** Bug 407737 has been marked as a duplicate of this bug. ***
*** Bug 417471 has been marked as a duplicate of this bug. ***
The crash happens when we set one of GnomeDruidPageStandard's color properties to NULL. GnomeDruidPageStandard doesn't like to receive NULL GdkColor values. This occurs when we load a glade file that contains a GnomeDruidPageStandard with no color property values explicitly set. So when loading the project, we thus have to create GladeProperty objects with the default value of boxed param specs, which happens to be NULL. Other widgets (such as GtkColorButton, GtkColorSelectionDialog) also give critical errors when we try and set a NULL GdkColor value on them. GnomeDruidPageStandard is an exception in that the relevant setters are not protected by g_return_if_fail(color != NULL) guards. A related problem is that we currently hardcode the "default" color values for GnomeDruidPageStandard in the catalog xml. However, GnomeDruidPageStandard has no default color values, as these values are first set after the widget is realized and the colors are queried from the current GTK+ theme in style_set(). We end up saving the hardcoded color values to the glade XML, which means that the druids may look out of place on user's machines. Here's a sketch of how we might solve these problems. The sketch runs all the way from creating/loading a new widget onwards. 1. Create a GladeProperty for the GdkColor property with a with the default (NULL) GdkColor value. Set the flag `property->modified' to FALSE. This is a new flag which helps us when we cannot determine what the actual "default" value of a property is. So it's lets us see whether to save to the xml or not. 2. Do _not_ sync it with with the widget at creation time (i.e. not setting the NULL value to the widget). This avoids the crash or overriding the color values obtained from the theme. We can see from the above stack trace that `glade_widget_sync_custom_props' is being called when the properties are being initially created. 3. When the widget is realized, we somehow need to sync the GladeProperty with the new color value in the widget. Maybe a g_signal_connect (widget, "notify",,) would do the trick. 4. If the GladeProperty has been explicitly modified by the user, set `property->modified' to TRUE. 5. If `property->modified == TRUE' then we can save the color value to the XML. Otherwise we must never save the color value to the XML.
I dont think this problem warrents added complexity to the glade core - the current core can handle this problem in the following ways: By what I read here the property is NULL by default (default is the pspec default), so the default glade value is NULL. Setting NULL on the object will crash in this particular case; so we can use an adaptor->set_property() override to ensure that we never set NULL on the object - or simply set "ignore" on that property so that it never gets set on the object. If the user never explicitly set the colour, then it should remain NULL, and thus not be set in the glade file (this would require us to remove the bogus hardcoding of colours in our gnome catalog) For other instances where the app does not crash because of setting colour=NULL (but fires critical warnings instead), we can still use the adaptor->set_proeprty() override to avoid that or just set ignore=True for those properties. As an added "nicety"; we could even set colours to "Black" in the adaptor->set_property() in the case that the glade value is NULL - in which case the user will see black - and the glade data will still be NULL). As an added note, properties that are usually automatically set up to dynamic values at load time should probably be marked "optional" with "optional-default=False", so as not to encourage the user to set these values.
(In reply to comment #9) > I dont think this problem warrents added complexity to > the glade core - the current core can handle this problem > in the following ways: > Ok, I think your plans are sensible. I guess I didn't have a full understanding of how adaptor functions could be used to solve these sort of problems. I will cook up a patch soonish. > As an added note, properties that are usually automatically set > up to dynamic values at load time should probably be marked > "optional" with "optional-default=False", so as not to encourage > the user to set these values. Sure, I think I will do that in this case. Particularly as GnomeDruidPageStandard also tries to mimic this behaviour by having gboolean "background-set", "foreground-set", etc, properties which make custom colors only optionally applied.
Well, I don't see why we necessarily have to have special case code to get around problems like this, when there is *absolutely* no reason to sync the GladeProperty with the widget in the first place. The GladeProperty was not specified in the glade file, and so it is at it's default value. Anyway, it just seems more valid that glade only sync's properties which were specified in the file, and doesn't needlessly bother itself with the rest (and no doubt save a lot of wasted cpu cycles and thus make loading quicker).
Usually special case code would not come into play just because you are setting a property to its default value at load time. This case is special because there is no way in the Glade UI to actually set a color value to the default property NULL (and that in itself is a bug (bug 457969), because you should be allowed to "unset" a color loaded from a previously loaded glade file). So, I have no real objections to your proposal of not syncing widgets to their default values at load time, but I dont think that that is a solution to _this_ problem... because as a rule, if a property can be set to its default value at load time, it can also be set to its default value by the user, if the user inputs a value that makes the property crash - the related plugin and/or widget itself should be fixed to properly swallow the value. We always have the "ignore" switch to work around properties that dont behave well so that we dont have to write special case code for widgets that misbehave (IMO we should always use "ignore" on problematic properties except when we find it especially important to sync the object with the property, probably not very important on old deprecated widget catalogs I'd say...).
This is fixed now because the colour editor widget doesnt allow direct setting the entry text (you need to use the color button to set it).