After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 730745 - Port annotations from Vala
Port annotations from Vala
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 730746
 
 
Reported: 2014-05-26 07:02 UTC by Evan Nemerson
Modified: 2014-05-30 20:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtk: various introspection fixes ported from Vala bindings (62.23 KB, patch)
2014-05-26 07:02 UTC, Evan Nemerson
none Details | Review
gtk: port many nullability annotation fixes from Vala bindings (26.48 KB, patch)
2014-05-26 16:19 UTC, Evan Nemerson
committed Details | Review
gtk: fix many callback annotations to include closure information (7.81 KB, patch)
2014-05-26 16:27 UTC, Evan Nemerson
committed Details | Review
gtk: port missing array annotations from Vala bindings (5.06 KB, patch)
2014-05-26 16:30 UTC, Evan Nemerson
committed Details | Review
gtk: fix annotation syntax and missing some missing annotations (7.88 KB, patch)
2014-05-26 16:35 UTC, Evan Nemerson
committed Details | Review
gtk: fix several out argument annotations (7.40 KB, patch)
2014-05-26 16:40 UTC, Evan Nemerson
reviewed Details | Review
gtk: add missing type annotations ported from Vala bindings (7.34 KB, patch)
2014-05-26 16:42 UTC, Evan Nemerson
committed Details | Review
gtk: add missing ownership annotations ported from Vala (4.68 KB, patch)
2014-05-26 16:43 UTC, Evan Nemerson
committed Details | Review
gtk: fix several out argument annotations (7.56 KB, patch)
2014-05-27 15:08 UTC, Evan Nemerson
committed Details | Review

Description Evan Nemerson 2014-05-26 07:02:37 UTC
Created attachment 277177 [details] [review]
gtk: various introspection fixes ported from Vala bindings

I'm porting the Vala bindings to GObject Introspection, need lots G-I annotation fixes to avoid regressions.
Comment 1 Emmanuele Bassi (:ebassi) 2014-05-26 10:45:50 UTC
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. :-)
Comment 2 Evan Nemerson 2014-05-26 16:19:17 UTC
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.
Comment 3 Evan Nemerson 2014-05-26 16:27:27 UTC
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).
Comment 4 Evan Nemerson 2014-05-26 16:30:34 UTC
Created attachment 277225 [details] [review]
gtk: port missing array annotations from Vala bindings
Comment 5 Evan Nemerson 2014-05-26 16:35:19 UTC
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.
Comment 6 Evan Nemerson 2014-05-26 16:40:19 UTC
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.
Comment 7 Evan Nemerson 2014-05-26 16:42:00 UTC
Created attachment 277229 [details] [review]
gtk: add missing type annotations ported from Vala bindings
Comment 8 Evan Nemerson 2014-05-26 16:43:42 UTC
Created attachment 277230 [details] [review]
gtk: add missing ownership annotations ported from Vala

This is the last one (at least for this series).
Comment 9 Emmanuele Bassi (:ebassi) 2014-05-27 10:39:42 UTC
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)"
Comment 10 Emmanuele Bassi (:ebassi) 2014-05-27 10:41:14 UTC
Review of attachment 277224 [details] [review]:

looks good.
Comment 11 Emmanuele Bassi (:ebassi) 2014-05-27 10:43:12 UTC
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.
Comment 12 Emmanuele Bassi (:ebassi) 2014-05-27 10:45:31 UTC
Review of attachment 277227 [details] [review]:

looks good.
Comment 13 Emmanuele Bassi (:ebassi) 2014-05-27 10:46:59 UTC
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?
Comment 14 Emmanuele Bassi (:ebassi) 2014-05-27 10:48:47 UTC
Review of attachment 277229 [details] [review]:

looks okay.
Comment 15 Emmanuele Bassi (:ebassi) 2014-05-27 10:49:52 UTC
Review of attachment 277230 [details] [review]:

looks okay.
Comment 16 Evan Nemerson 2014-05-27 15:01:41 UTC
(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.
Comment 17 Evan Nemerson 2014-05-27 15:08:15 UTC
Created attachment 277309 [details] [review]
gtk: fix several out argument annotations
Comment 18 Matthias Clasen 2014-05-27 17:53:55 UTC
Review of attachment 277224 [details] [review]:

.
Comment 19 Matthias Clasen 2014-05-27 17:54:31 UTC
Review of attachment 277225 [details] [review]:

sure
Comment 20 Matthias Clasen 2014-05-27 17:56:37 UTC
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.
Comment 21 Matthias Clasen 2014-05-27 17:58:47 UTC
Review of attachment 277229 [details] [review]:

sure
Comment 22 Matthias Clasen 2014-05-27 17:59:50 UTC
Review of attachment 277230 [details] [review]:

ok
Comment 23 Matthias Clasen 2014-05-27 18:02:00 UTC
Review of attachment 277309 [details] [review]:

looks fine to me
Comment 24 Evan Nemerson 2014-05-28 22:40:53 UTC
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.
Comment 25 Matthias Clasen 2014-05-30 18:53:36 UTC
I agree with Emmanuele that it should be split up.
Comment 26 Evan Nemerson 2014-05-30 19:01:03 UTC
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.
Comment 27 Matthias Clasen 2014-05-30 19:48:48 UTC
oh, I see, I had missed that there was an even bigger initial patch
Comment 28 Matthias Clasen 2014-05-30 19:49:41 UTC
Review of attachment 277220 [details] [review]:

ok