Bug 44790

Comment on attachment 55588 [details] [review]
glib: various minor introspection improvements

Review of attachment 55588 [details] [review]:
-----------------------------------------------------------------

Thanks for the patch! just a few comments

::: glib/poppler-attachment.h
@@ +39,2 @@
>   * @count: number of bytes in @buf.
> + * @data: (closure closure): user data passed to poppler_attachment_save_to_callback()

This should be (closure), (closure closure) is the callback.

@@ +41,1 @@
>   * @error: GError to set on error, or NULL

We should probably annotate this as (out), no?

::: glib/poppler-document.cc
@@ +2497,4 @@
>   * Returns the #PopplerFormField for the given @id. It must be freed with
>   * g_object_unref()
>   *
> + * Return value: (transfer full): a new #PopplerFormField or NULL if

Use %NULL, please.

::: glib/poppler-media.h
@@ +38,2 @@
>   * @count: number of bytes in @buf.
> + * @data: (closure closure): user data passed to poppler_media_save_to_callback()

Same here, this should be (closure).

@@ +40,1 @@
>   * @error: GError to set on error, or NULL

And here we could annotate this as (out)

Created attachment 55609 [details] [review]
glib: various minor introspection and documentation improvements

(In reply to comment #1)
> Comment on attachment 55588 [details] [review] [review]
> glib: various minor introspection improvements
> 
> Review of attachment 55588 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! just a few comments

Thanks for the quick review.

> ::: glib/poppler-attachment.h
> @@ +39,2 @@
> >   * @count: number of bytes in @buf.
> > + * @data: (closure closure): user data passed to poppler_attachment_save_to_callback()
> 
> This should be (closure), (closure closure) is the callback.

Oops.

> @@ +41,1 @@
> >   * @error: GError to set on error, or NULL
> 
> We should probably annotate this as (out), no?

No, that's not necessary. GObject introspection will do the right thing here.

> ::: glib/poppler-document.cc
> @@ +2497,4 @@
> >   * Returns the #PopplerFormField for the given @id. It must be freed with
> >   * g_object_unref()
> >   *
> > + * Return value: (transfer full): a new #PopplerFormField or NULL if
> 
> Use %NULL, please.

I guess there is no reason not to fix this while fixing something else on the same line...

> ::: glib/poppler-media.h
> @@ +38,2 @@
> >   * @count: number of bytes in @buf.
> > + * @data: (closure closure): user data passed to poppler_media_save_to_callback()
> 
> Same here, this should be (closure).

Again, oops.
 
> @@ +40,1 @@
> >   * @error: GError to set on error, or NULL
> 
> And here we could annotate this as (out)

Yes, but it's unnecessary. However s/NULL/%NULL/. Actually, I'll just fix all the results from grep -P '^ \*.+ NULL' *.{cc,h}

Comment on attachment 55609 [details] [review]
glib: various minor introspection and documentation improvements

Review of attachment 55609 [details] [review]:
-----------------------------------------------------------------

Pushed to git master and poppler-0.18. Thank you very much.