Bug 29358

Created attachment 37521 [details] [review]
Introspect TpError

Might help if I didn't just attach random files from my source directory.

> + * tp_error_get_dbus_name: (skip)

This should probably be visible to g-i: it's quite useful. Annotate its return as (transfer none) and it should work fine.

> + * Since: 0.11.12

When adding API, please always say "Since: 0.11.UNRELEASED" (with capitalization), to avoid having false documentation if a release happens before a branch is merged. Replacing "UNRELEASED" is part of the release process.

I'd prefer it if tp_error_quark() was literally just "return tp_errors_quark()", to avoid the possibility that they don't match.

As implemented, I suspect that calling tp_error_quark();tp_errors_quark() will also cause warnings/criticals in dbus-glib, as we try to register the same error domain twice (the two functions currently use different variables for their "once" guard). Implementing one in terms of the other would also fix this.

Not putting tp_error_quark in telepathy-glib-sections.txt will break the build. You don't need to actually document it - just put it under <SUBSECTION Standard> next to tp_errors_quark.

I opened a bug against GLib to expand the GError documentation to hopefully prevent misnamings like this in the future: https://bugzilla.gnome.org/show_bug.cgi?id=625827

Apparently the introspection scanner is broken if adding a "Rename to:" annotation doesn't work for renaming tp_errors_quark() to tp_error_quark() (which it didn't). I don't know if you want to hold on this patch until the scanner is fixed, to save introducing new API unnecessarily. I've filed https://bugzilla.gnome.org/show_bug.cgi?id=625839 about the scanner problem.

For this work to be most useful for libfolks, the generated VAPI file needs to interpret the error enum correctly as an error domain. That requires this bug to be fixed: https://bugzilla.gnome.org/show_bug.cgi?id=625837. That shouldn't block this bug though.

Should I cook up an updated patch, or do you want to wait on the introspection scanner being fixed?

(In reply to comment #3)
> Apparently the introspection scanner is broken if adding a "Rename to:"
> annotation doesn't work for renaming tp_errors_quark() to tp_error_quark()
> (which it didn't). I don't know if you want to hold on this patch until the
> scanner is fixed, to save introducing new API unnecessarily.

Every time we add a D-Bus method we gain 2 extern symbols, so I think adding tp_error_quark() would be the least of our worries :-)

> For this work to be most useful for libfolks, the generated VAPI file needs to
> interpret the error enum correctly as an error domain. That requires this bug
> to be fixed: https://bugzilla.gnome.org/show_bug.cgi?id=625837. That shouldn't
> block this bug though.

You say "most useful". Does it still improve libfolks (a bit) if we add tp_error_quark(), even before 625837 is fixed?

If it does, tp_error_quark is probably worthwhile to merge; if it doesn't, wait and see whether 625839 gets fixed before 625837.

Created attachment 37584 [details] [review]
Introspect TpError (updated)

Updated patch.

bgo#625827 has been fixed.

Turns out that g-ir-scanner hides the error quark function (tp_error_quark()) from the GIR file once it works out that it's an error quark function. This means that bgo#625837 needs to be fixed for this to be useful to libfolks.

Review of attachment 37584 [details] [review]:

> Closes: bfo#29358

We conventionally call this bugzilla "fd.o", and usually use this form:

    fd.o #29358: introspect TpError

    We need to [etc.]

::: telepathy-glib/errors.c
@@ +50,3 @@
+ * 0.10.x, use %TP_ERRORS.
+ *
+ * Since: 0.11.UNRELEASED

TP_ERROR existed in a release already, I think, it just wasn't documented there?

Created attachment 37586 [details] [review]
Introspect TpError (updated again)

TP_ERROR has been there since 0.11.7, apparently.

Ship it.

Sorry, missed merging this for 0.11.12 (I keep forgetting that you don't have commit access). Fixed in git for 0.11.13.