could you please split out this patch into multiple commits? a huge patch makes it much more likely to overlook something, and it makes Splinter slower at generating the review page. plus, you get to rise in the commit count. :-)

Created attachment 277220 [details] [review]
gtk: port many nullability annotation fixes from Vala bindings

The split isn't completely perfect because some of the changes overlap, but I've tried to only include the nullability fixes in this one.

(In reply to comment #1)
> plus, you get to rise in the commit count. :-)

FWIW I really don't care about that.  If you want me to just squash everything back together at the end to reduce noise in git I don't mind.

Created attachment 277224 [details] [review]
gtk: fix many callback annotations to include closure information

Without	this information introspection-based consumers don't realize 
they can include context information, but instead think that they  
receive	an extra gpointer argument (which they don't know how to       
handle).

Created attachment 277225 [details] [review]
gtk: port missing array annotations from Vala bindings

Created attachment 277227 [details] [review]
gtk: fix annotation syntax and missing some missing annotations

These changes clean up various errors and omissions resulting from
either slightly incorrect G-I/gtk-doc syntax or missing documentation
blocks.

Created attachment 277228 [details] [review]
gtk: fix several out argument annotations

These mostly just switch from allow-none to optional, nullable, or
both, as necessary.

Created attachment 277229 [details] [review]
gtk: add missing type annotations ported from Vala bindings

Created attachment 277230 [details] [review]
gtk: add missing ownership annotations ported from Vala

This is the last one (at least for this series).

Review of attachment 277220 [details] [review]:

looks generally good.

::: gtk/deprecated/gtkrc.c
@@ +1414,3 @@
+ *     specified and the default style should be used. The returned
+ *     value is owned by GTK+ as part of an internal cache, so you
+ *     must call g_object_ref() on the returned value if you want to

I know this is a patch for introducing annotations, but cleaning up the docs would also be good.

in this case, we should remove the mention to g_object_ref(): the (transfer none) annotation and the "the returned value is owned by GTK+" should be enough, and match what we do elsewhere.

plus, this function is very much deprecated.

::: gtk/gtkbuilder.c
@@ +1644,3 @@
  * @signal_name: name of the signal
  * @handler_name: name of the handler
+ * @connect_object: (nullable): a #GObject, if non-%NULL, use g_signal_connect_object()

the description should be cleaned up; I'd just use "a #GObject, to pass to g_signal_connect_object()".

::: gtk/gtkmenuitem.c
@@ +1312,3 @@
  * gtk_menu_item_set_submenu:
  * @menu_item: a #GtkMenuItem
+ * @submenu: (allow-none) (type Gtk.Menu): the submenu, or %NULL

shouldn't this be (nullable)?

::: gtk/gtkselection.c
@@ +1717,3 @@
+ *   contained a recognized image type and it could be converted to a
+ *   #GdkPixbuf, a newly allocated pixbuf is returned, otherwise
+ *   %NULL.  If the result is non-%NULL it must be freed with

remove the last sentence.

::: gtk/gtkwidget.c
@@ +10244,3 @@
  * gtk_widget_create_pango_layout:
  * @widget: a #GtkWidget
+ * @text: (nullable): text to set on the layout (can be %NULL)

you can drop the "(can be %NULL)"

Review of attachment 277224 [details] [review]:

looks good.

Review of attachment 277225 [details] [review]:

looks good.

::: gtk/deprecated/gtkcolorsel.h
@@ +60,3 @@
 /**
  * GtkColorSelectionChangePaletteWithScreenFunc:
  * @screen:

I'd add what the @screen is as well, but it's really up to you; after all, the function is very much deprecated.

Review of attachment 277227 [details] [review]:

looks good.

Review of attachment 277228 [details] [review]:

looks good.

::: gtk/deprecated/gtkstyle.c
@@ +4665,2 @@
  * @widget: a #GtkWidget
  * @path_length: (out) (allow-none): location to store the length of the

shouldn't this be (optional) as well?

Review of attachment 277229 [details] [review]:

looks okay.

Review of attachment 277230 [details] [review]:

looks okay.

(In reply to comment #9)
> Review of attachment 277220 [details] [review]:
> 
> looks generally good.
> 
> ::: gtk/deprecated/gtkrc.c
> @@ +1414,3 @@
> + *     specified and the default style should be used. The returned
> + *     value is owned by GTK+ as part of an internal cache, so you
> + *     must call g_object_ref() on the returned value if you want to
> 
> I know this is a patch for introducing annotations, but cleaning up the docs
> would also be good.

I plan to submit a couple more patches soon to clean up the docs with an eye towards making them more usable for languages other than C for all of GTK+ (as well as other libs), not just the bits which happened to be nearby when I needed to change something else.  As you noticed, this patch was already getting kind of big.
 
> in this case, we should remove the mention to g_object_ref(): the (transfer
> none) annotation and the "the returned value is owned by GTK+" should be
> enough, and match what we do elsewhere.

> plus, this function is very much deprecated.

Vala still needs accurate annotations for backwards compatibility, even for deprecated functions and functions which G-I can't handle and are marked as (skip).

> 
> ::: gtk/gtkbuilder.c
> @@ +1644,3 @@
>   * @signal_name: name of the signal
>   * @handler_name: name of the handler
> + * @connect_object: (nullable): a #GObject, if non-%NULL, use
> g_signal_connect_object()
> 
> the description should be cleaned up; I'd just use "a #GObject, to pass to
> g_signal_connect_object()".

That sounds good, but for one of those future patches.

> ::: gtk/gtkmenuitem.c
> @@ +1312,3 @@
>   * gtk_menu_item_set_submenu:
>   * @menu_item: a #GtkMenuItem
> + * @submenu: (allow-none) (type Gtk.Menu): the submenu, or %NULL
> 
> shouldn't this be (nullable)?

(allow-none) is an existing annotation—what I changed was the (type Gtk.Menu) part.  This should have been part of the attachment #277229 [details] instead of this patch.

I may go through and convert allow-none to nullable for in arguments, but I thought it would be a good idea to hold off for a while since converting them now would break support for old versions of g-i, but not really do any good.

> ::: gtk/gtkselection.c
> @@ +1717,3 @@
> + *   contained a recognized image type and it could be converted to a
> + *   #GdkPixbuf, a newly allocated pixbuf is returned, otherwise
> + *   %NULL.  If the result is non-%NULL it must be freed with
> 
> remove the last sentence.

Future patch.

> ::: gtk/gtkwidget.c
> @@ +10244,3 @@
>   * gtk_widget_create_pango_layout:
>   * @widget: a #GtkWidget
> + * @text: (nullable): text to set on the layout (can be %NULL)
> 
> you can drop the "(can be %NULL)"

Future patch.



(In reply to comment #11)
> Review of attachment 277225 [details] [review]:
> 
> looks good.
> 
> ::: gtk/deprecated/gtkcolorsel.h
> @@ +60,3 @@
>  /**
>   * GtkColorSelectionChangePaletteWithScreenFunc:
>   * @screen:
> 
> I'd add what the @screen is as well, but it's really up to you; after all, the
> function is very much deprecated.

TBH I'm not the best person to add new documentation like that.  My GTK+ knowledge is pretty basic, so I would probably just end up with something like "a #GdkScreen".  If the function weren't deprecated I'd probably make more of an effort, but as it is I really just want the annotations to avoid backwards compatibility breaks in Vala.


(In reply to comment #13)
> Review of attachment 277228 [details] [review]:
> 
> looks good.
> 
> ::: gtk/deprecated/gtkstyle.c
> @@ +4665,2 @@
>   * @widget: a #GtkWidget
>   * @path_length: (out) (allow-none): location to store the length of the
> 
> shouldn't this be (optional) as well?

Yes, I'll update the patch.

Created attachment 277309 [details] [review]
gtk: fix several out argument annotations

Review of attachment 277224 [details] [review]:

.

Review of attachment 277225 [details] [review]:

sure

Review of attachment 277227 [details] [review]:

looks fine, otherwise

::: gtk/gtkwidget.h
@@ -951,3 @@
 GDK_AVAILABLE_IN_3_14
 void                  gtk_widget_get_clip               (GtkWidget     *widget,
-                                                         GtkAllocation *allocation);  

This has already been corrected on master.

Review of attachment 277229 [details] [review]:

sure

Review of attachment 277230 [details] [review]:

ok

Review of attachment 277309 [details] [review]:

looks fine to me

Emmanuele, with my response in mind, are you okay with attachment #277220 [details] as-is?  I'm guessing Matthias held off on accepting it based on your comments, but I'd really like to get it in ASAP since it's holding up bug #730746.

I agree with Emmanuele that it should be split up.

I did split it up... attachment #277177 [details] was the original patch, which I split up into 7 pieces.  Attachment #277220 [details] is just about nullability annotations (nullable and optional).  I don't think I can really split it up more without something pretty arbitrary (like gtk[a-m]*.{c,h} and gtk[n-z].{c,h} or something).

What Emmanuele was saying is that he actually wanted the patch to do more, not less.  I was saying I should remove the documentation which is basically just a duplicate of what the annotations say, but that documentation was there before my patch.  I would like to do that for all of GTK+, not just the parts near where my patch touched, but I think it should be a separate patch.

oh, I see, I had missed that there was an even bigger initial patch

Review of attachment 277220 [details] [review]:

ok