Created attachment 276363 [details] [review]
Assorted introspection fixes ported from the Vala metadata

This patch is the result of me going through Vala's metadata and porting what I could to annotations.

Created attachment 277248 [details] [review]
introspection: Fix some g-ir-scanner warnings

Some more fixes, from things that g-ir-scanner was warning about.

FWIW, feel free to squash these all into a single commit to reduce noise.

Comment on attachment 276362 [details] [review]
Add missing (nullable) annotations, assorted minor introspection fixes

>- * Return value: (allow-none): the new #SoupAddress
>+ * Return value: (nullable): the new #SoupAddress

That means we lose the annotation with non-bleeding-edge versions of gobject-introspection though...

Maybe that's not a big deal since probably nothing uses it for return-value anyway, and it looks like this patch only touches return values? If so, then ok I guess.

Comment on attachment 276363 [details] [review]
Assorted introspection fixes ported from the Vala metadata

>- * @params: (element-type utf8 utf8) (out) (transfer full) (allow-none): return
>+ * @params: (element-type utf8 utf8) (out) (transfer full) (optional): return

Here, you definitely need to say "(allow-none) (optional)" for now though, I guess.

Though, in that case, there's not really any point, since both old and new g-i will treat that the same as just "(allow-none)" won't they?

>  * soup_buffer_new:
>  * @use: how @data is to be used by the buffer
>- * @data: data
>+ * @data: (array length=length) (element-type guint8): data
>  * @length: length of @data

This gets overwritten by soup_buffer_new_take() via (rename-to), so any annotations here are irrelevant. (Maybe add (skip) to make it explicit?)

Comment on attachment 277248 [details] [review]
introspection: Fix some g-ir-scanner warnings

>-/*
>+/**
>  * SECTION:soup-proxy-uri-resolver

What does g-i warn about here? This was intentional, to keep the docs out of the GIR file since SoupProxyURIResolver is deprecated now.

(In reply to comment #3)
> (From update of attachment 276362 [details] [review])
> >- * Return value: (allow-none): the new #SoupAddress
> >+ * Return value: (nullable): the new #SoupAddress
> 
> That means we lose the annotation with non-bleeding-edge versions of
> gobject-introspection though...
> 
> Maybe that's not a big deal since probably nothing uses it for return-value
> anyway, and it looks like this patch only touches return values? If so, then ok
> I guess.

allow-none was ignored on return values before nullable and optional were added.  I don't just mean nothing used them--the information wasn't even present in the GIR.  Switching to nullable shouldn't mean anything to old G-I, and to new G-I it means you're not using the deprecated syntax anymore.

(In reply to comment #4)
> (From update of attachment 276363 [details] [review])
> >- * @params: (element-type utf8 utf8) (out) (transfer full) (allow-none): return
> >+ * @params: (element-type utf8 utf8) (out) (transfer full) (optional): return
> 
> Here, you definitely need to say "(allow-none) (optional)" for now though, I
> guess.

You're right.

> Though, in that case, there's not really any point, since both old and new g-i
> will treat that the same as just "(allow-none)" won't they?

The only thing I don't like about just saying (allow-none) is that there are a lot of incorrect (allow-none) annotations on (out) arguments since it was formerly ambiguous whether it mean (optional) or (nullable), so I don't always trust the annotations.  It's not really a big deal, though--I'll attach a version of the patch which just doesn't include this change.

> >  * soup_buffer_new:
> >  * @use: how @data is to be used by the buffer
> >- * @data: data
> >+ * @data: (array length=length) (element-type guint8): data
> >  * @length: length of @data
> 
> This gets overwritten by soup_buffer_new_take() via (rename-to), so any
> annotations here are irrelevant. (Maybe add (skip) to make it explicit?)

We still have a binding for soup_buffer_new in Vala (we un-skip it in metadata), and it would be nice to make use of the annotations.  We also ignore (rename-to) because it would break all the existing bindings (remember, Vala is much older than G-I), and we support a lot of stuff G-I doesn't (like variadic functions) that people tend to add (rename-to) annotations for.

Also, as a C programmer I still prefer the annotations to be there since it makes the documentation a bit easier to read...  instead of having to actually read the documentation I can generally just glance at the annotations and know what is expected.  It's probably not a big deal for this function (transfer annotations are the really helpful ones), but the consistency is nice.

(In reply to comment #5)
> (From update of attachment 277248 [details] [review])
> >-/*
> >+/**
> >  * SECTION:soup-proxy-uri-resolver
> 
> What does g-i warn about here?

It's actually soup_proxy_uri_resolver_get_proxy_uri_async which causes the warning:

    soup-proxy-uri-resolver.h:52: Warning: Soup: soup_proxy_uri_resolver_get_proxy_uri_async: argument callback: Missing (scope) annotation for callback without GDestroyNotify (valid: call, async)

> This was intentional, to keep the docs out of
> the GIR file since SoupProxyURIResolver is deprecated now.

That doesn't really work.  Soup.ProxyURIResolver is still present in the GIR, and get_proxy_uri_sync is even perfectly usable.  get_proxy_uri_async is only unusable (marked as introspectable="0") because of the missing (scope) annotation, not because there are no gtk-doc comments for it.

If you want to hide something from the GIR, you should add (skip) annotations.  That said, they have already been exposed as public API in a stable release, so at this point it probably makes more sense to just make the comments real gtk-doc comments and add Deprecated tags.

Created attachment 278291 [details] [review]
Add missing array annotations for binary data

Switches that (optional) annotation back to (allow-none), nothing else changed.

> We still have a binding for soup_buffer_new in Vala

> Also, as a C programmer I still prefer the annotations to be there

OK, makes sense.

> > This was intentional, to keep the docs out of
> > the GIR file since SoupProxyURIResolver is deprecated now.
> 
> That doesn't really work.  Soup.ProxyURIResolver is still present in the GIR

I didn't want to keep the type itself out of the GIR, just its documentation. But that's not a huge deal I guess; the docs aren't in the gtk-doc-generated documentation, which is what everyone really sees anyway.

very belatedly pushed these. thanks

Attachment 276362 [details] pushed as a50ea58 - Add missing (nullable) annotations, assorted minor introspection fixes
Attachment 277248 [details] pushed as 4957820 - introspection: Fix some g-ir-scanner warnings
Attachment 278291 [details] pushed as b24ef29 - Add missing array annotations for binary data