Created attachment 147579 [details] [review]
Implement GTK_ENABLE_DIAGNOSTIC, GTK_PARAM_DEPRECATED, GTK_OBJECT_WARN_UNSUPPORTED_PROPERTY

This patch implements a what could be diagnostic mode. It's not usable as-is and mainly serves to evaluate the idea.

The environment variable GTK_ENABLE_DIAGNOSTIC is used to determine whether diagnostic mode should be enabled.

A new GParamFlag GTK_PARAM_DEPRECATED is introduced that can be used when installing properties to flag them as deprecated. In the patch it's used in GtkNotebook as an example.

The macro GTK_OBJECT_WARN_UNSUPPORTED_PROPERTY is to be inserted in _set_property and _get_property implementations. It will expand to a warning if diagnostic mode is enabled and either a deprecated property is being used or properties of an internal child are being modified.

GtkContainer emits a warning if an internal child is added to or removed. This needs work in individual widgets to unset the GTK_COMPOSITE_CHILD flag to prevent false positives.

GtkStatusbar is modified to mark its children as composite children. In conjunction with the above, it will emit warnings when for example Totem or Midori is run since they alter statusbar packing.

punting to 3.0

Christian, could we please get GTK_PARAM_DEPRECATED + GTK_OBJECT_WARN_UNSUPPORTED_PROPERTY as a patch for glib. Then I could also do gtk-doc support for deprecated properties and people can use that (e.g. in gstreamer).

I also wonder if you would maybe want to hide
if (g_getenv ("GTK_ENABLE_DIAGNOSTIC"))
behine a macros, so that later on one could e.g. also add a commandlne option or cache the result in a global variable.

Created attachment 164009 [details] [review]
Introduce G_PARAM_DEPRECATED and G_ENABLE_DIAGNOSTIC

This patch introduces G_PARAM_DEPRECATED and generates the warning internally in GObject.

We will still need something in GTK+ if we want composite container warnings.

Review of attachment 164009 [details] [review]:

::: gobject/gobject.c
@@ +1029,3 @@
+      enable_diagnostic = g_getenv ("G_ENABLE_DIAGNOSTIC");
+    {
+  if (G_UNLIKELY (!enable_diagnostic))

why an 'else if'? it would be more clear if the conditions where split - the first one to initialize enable_diagnostic and the second one checking it.

also, I wonder if this block might go under a #if G_ENABLE_DEBUG ... #endif guard; though I'm not definitely sure.

::: gobject/gparam.h
@@ +184,2 @@
 #define	G_PARAM_USER_SHIFT	(8)
  */

shouldn't this be bumped to 9?

Created attachment 164029 [details] [review]
Introduce G_PARAM_DEPRECATED and G_ENABLE_DIAGNOSTIC #2

Separated the if and bumped G_PARAM_USER_SHIFT.

I think the feature should always be there unconditionally because people who develop applications want to use it with their normal GLib installation and likely don't have a development build.

(In reply to comment #6)

the patch looks good to me.

> I think the feature should always be there unconditionally because people who
> develop applications want to use it with their normal GLib installation and
> likely don't have a development build.

yeah, good point - I confused this with Clutter, which defines ENABLE_DEBUG even with the "minimum" debugging level.

small tip: the message should say "any more", not "anymore". :-)

Review of attachment 164029 [details] [review]:

::: gobject/gparam.h
@@ +183,3 @@
  * #GParamSpec.flags.
  */
+#define	G_PARAM_USER_SHIFT	(9)

This is an abi break, unfortunately

After discussion on IRC and looking for code out there extending GParamSpec flags, Gimp with its 8 + G_PARAM_USER_SHIFT appears to be on the extreme side, aside from being a rarity to do it at all.

So I updated the commit to use 1  << 31 and to leave G_PARAM_USER_SHIFT alone. I added a note to the docs that 30 is the limit.

In the happiness I closed the report while we still need to see about GtkContainer warnings. Apologies for the annoyance.

Created attachment 165132 [details] [review]
Introduce GTK_ENABLE_DIAGNOSTIC for containers

This is the GTK+ part split from the original patch. If the environment variable GTK_ENABLE_DIAGNOSTIC is set, gtk_container_add and gtk_container_remove will complain if called with composite children.
Included is a change to GtkStatusbar to use gtk_widget_push_composite_child(). Most other widgets already do that, so support will come "for free".

Created attachment 165136 [details] [review]
Introduce G_SIGNAL_DEPRECATED flag

This patch introduces the new signal flag G_SIGNAL_DEPRECATED. If specified, connecting to the signal results in a warning much like the one for deprecated properties.

Review of attachment 165132 [details] [review]:

::: gtk/gtkcontainer.c
@@ +1204,3 @@
+      g_warning ("A %s is being added to a %s. "
+    if (GTK_WIDGET_COMPOSITE_CHILD (widget))
+  if (g_getenv ("GTK_ENABLE_DIAGNOSTIC"))

I think you mean:

  Adding internal widgets to containers is not supported. Use gtk_widget_set_parent() instead.

@@ +1242,3 @@
+    if (GTK_WIDGET_COMPOSITE_CHILD (widget))
+  if (g_getenv ("GTK_ENABLE_DIAGNOSTIC"))
+

probably adding:

  Use gtk_widget_unparent() instead.

wouldn't hurt.

Review of attachment 165136 [details] [review]:

looks good to me.

::: gobject/gsignal.c
@@ +2099,3 @@
 }
+signal_diagnostics (const gchar* detailed_signal,
+static void

I'd have probably inlined this, but could be premature optimization

@@ +2114,3 @@
+                    GSignalFlags flags)
+signal_diagnostics (const gchar* detailed_signal,
+static void

"and shouldn't be used any more, as it will be removed in a future version."

(In reply to comment #14)
> Review of attachment 165132 [details] [review]:
> 
> ::: gtk/gtkcontainer.c
> @@ +1204,3 @@
> +      g_warning ("A %s is being added to a %s. "
> +    if (GTK_WIDGET_COMPOSITE_CHILD (widget))
> +  if (g_getenv ("GTK_ENABLE_DIAGNOSTIC"))
> 
> I think you mean:
> 
>   Adding internal widgets to containers is not supported. Use
> gtk_widget_set_parent() instead.

Hm.. I agree with the better wording, but I am not sure pointing to gtk_widget_set_parent is right to recommend. If an application developer sees that, he might think he used the wrong API call, while in fact he should not do this at all.

(In reply to comment #16)
> (In reply to comment #14)
> > Review of attachment 165132 [details] [review] [details]:
> > 
> > ::: gtk/gtkcontainer.c
> > @@ +1204,3 @@
> > +      g_warning ("A %s is being added to a %s. "
> > +    if (GTK_WIDGET_COMPOSITE_CHILD (widget))
> > +  if (g_getenv ("GTK_ENABLE_DIAGNOSTIC"))
> > 
> > I think you mean:
> > 
> >   Adding internal widgets to containers is not supported. Use
> > gtk_widget_set_parent() instead.
> 
> Hm.. I agree with the better wording, but I am not sure pointing to
> gtk_widget_set_parent is right to recommend. If an application developer sees
> that, he might think he used the wrong API call, while in fact he should not do
> this at all.

if an app developer went out of her way to call push_composite()/pop_composite() then she would probably appreciate the hint to the right function to call. on the other hand, if we don't want this behaviour *at all* then we probably want to add some wording to that effect to the warning.

Created attachment 165812 [details] [review]
Introduce G_SIGNAL_DEPRECATED flag #2

Updated wording.

Review of attachment 165812 [details] [review]:

looks good to me.

Created attachment 165816 [details] [review]
Introduce GTK_ENABLE_DIAGNOSTIC for containers #2

We definitely don't want to encourage developers to work around container warnings.

I added a sentence to the warning:
"See documentation on containers and composite children."

And I added a note about GTK_ENABLE_DIAGNOSTIC to gtk_widget_push_composite_child().

I avoided exact function names in the warning, since those would probably be confusing when not using C. Hopefully the wording is clear enough to find it in the reference.

Review of attachment 165816 [details] [review]:

looks good to me

Review of attachment 165816 [details] [review]:

looks good to me

Comment on attachment 164029 [details] [review]
Introduce G_PARAM_DEPRECATED and G_ENABLE_DIAGNOSTIC #2

This one got committed, afaics

Review of attachment 165812 [details] [review]:

Does the G_SIGNAL_FLAGS_MASK needs to be updated ?

Created attachment 173010 [details] [review]
Introduce G_SIGNAL_DEPRECATED flag #3

Updated the mask and Since.

Review of attachment 173010 [details] [review]:

I don't think that mask is correct.

$ bc
bc 1.06.95
Copyright 1991-1994, 1997, 1998, 2000, 2004, 2006 Free Software Foundation, Inc.
This is free software with ABSOLUTELY NO WARRANTY.
For details type `warranty'. 
ibase=16
obase=2
8F
10001111

Just noticed that bug, but I already proposed a new patch for G_SIGNAL_DEPRECATED in bug 601686.

We have G_PARAM_DEPRECATED and G_SIGNAL_DEPRECATED; I think this should be considered complete.

/usr/include/glib-2.0/gobject/gparam.h:166:26: error: result of ‘1 << 31’ requires 33 bits to represent, but ‘int’ only has 32 bits [-Werror=shift-overflow=]
   G_PARAM_DEPRECATED          = 1 << 31



Is it something glib could fix?

Not until the ISO C standards group decides to plug the gaping hole in the C spec, and specify how enumerations should be sized.