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 770785 - Add GTK-Doc API reference and GObject Introspection support
Add GTK-Doc API reference and GObject Introspection support
Status: RESOLVED FIXED
Product: devhelp
Classification: Applications
Component: General
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: devhelp-maint
devhelp-maint
Depends on:
Blocks:
 
 
Reported: 2016-09-03 11:45 UTC by Corentin Noël
Modified: 2016-12-01 21:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GObject introspection support (6.33 KB, patch)
2016-09-03 11:46 UTC, Corentin Noël
none Details | Review
Enable GObject introspection (43.11 KB, patch)
2016-10-20 00:49 UTC, Corentin Noël
none Details | Review
Enable GObject introspection (46.76 KB, patch)
2016-10-20 00:52 UTC, Corentin Noël
needs-work Details | Review
Enable Gtk-Doc on the project (27.70 KB, patch)
2016-10-23 22:33 UTC, Corentin Noël
needs-work Details | Review

Description Corentin Noël 2016-09-03 11:45:20 UTC
There is currently no Gir support for libdevhelp, it would be really cool to have a native GObject Introspection support
Comment 1 Corentin Noël 2016-09-03 11:46:54 UTC
Created attachment 334708 [details] [review]
GObject introspection support

I'm unable to test it because I don't have the required Gtk version, but it should work (or require some minor tweaks)
Comment 2 Sébastien Wilmet 2016-09-03 15:33:29 UTC
Devhelp also needs to be documented with GTK-Doc, to add the GI annotations.
Comment 3 Corentin Noël 2016-10-20 00:49:45 UTC
Created attachment 338054 [details] [review]
Enable GObject introspection

Here is a patch that enable the GObject introspection when available, compile a .vapi file for the Vala bindings (also when Vala is available).
Plus some other changes to have a correct binding (like stronger typing in signals).
It also adds a lot of documentation to the project.
Comment 4 Corentin Noël 2016-10-20 00:52:53 UTC
Created attachment 338057 [details] [review]
Enable GObject introspection
Comment 5 Sébastien Wilmet 2016-10-23 08:52:10 UTC
Review of attachment 338057 [details] [review]:

Please create more commits, to review the changes more easily.

First: GTK-Doc support (and add a docs/reference/ directory to build the docs, to also see if there are problems in the GTK-Doc comments, there should be zero errors/warnings).
Adding GTK-Doc support can be split in several commits.

Second: GObject Introspection support.

Third: Generate vapi for Vala.

::: src/dh-book-tree.c
@@ +707,2 @@
                               G_TYPE_NONE,
+                              1, DH_TYPE_LINK);

Instead of sneaking in such changes, please create a separate commit.
Comment 6 Corentin Noël 2016-10-23 22:33:18 UTC
Created attachment 338308 [details] [review]
Enable Gtk-Doc on the project

Okay, so here is the first patch that enables the GTK-Doc part, and this without any warning (if you've the GLib, Gtk and WebKitGtk4 documentation installed otherwise there are small warnings about broken links)

So it's then possible to look the Devhelp documentation on Devhelp :)
Comment 7 Sébastien Wilmet 2016-10-25 13:09:41 UTC
Review of attachment 338308 [details] [review]:

(I'm not an official Devhelp maintainer, but I maintain other modules in gnome.)

In the diff view here in bugzilla docs/Makefile.am and docs/reference/Makefile.am are empty. I don't know why. But docs/Makefile.am should contain at least the SUBDIRS.

devhelp.types should not be necessary with newer GTK-Doc versions. The file should be deleted. It's normally explained in the GTK-Doc manual, somewhere:
https://developer.gnome.org/gtk-doc-manual/unstable/

Usually gtk-doc.make is not copied in GNOME modules.

::: configure.ac
@@ +95,3 @@
+],[
+AM_CONDITIONAL([ENABLE_GTK_DOC], false)
+])

I would call GTK_DOC_CHECK directly without checking that it is defined.

@@ +111,3 @@
+docs/Makefile
+docs/reference/Makefile
+docs/reference/devhelp/Makefile

I would put the gtk-doc files in docs/reference/, not docs/reference/devhelp/, because devhelp is the only thing.

::: docs/reference/devhelp/Makefile.am
@@ +88,3 @@
+endif
+
+-include $(top_srcdir)/git.mk

git.mk isn't used in Devhelp (but it would be welcome I think).

::: docs/reference/devhelp/devhelp-sections.txt
@@ +23,3 @@
+DH_IS_APP_CLASS
+DH_TYPE_APP
+DhApp

The DhApp type should not be in the Standard section, it should be placed just above dh_app_new. The same for the other classes. At least that's what GTK+ and other GNOME libraries do.

@@ +216,3 @@
+<FILE>dh-preferences</FILE>
+<TITLE>DhPreferences</TITLE>
+DH_PREFERENCES_CONST

DH_PREFERENCES_CONST should be in the Standard section.
Comment 8 Corentin Noël 2016-10-25 16:54:43 UTC
Okay, I created https://git.gnome.org/browse/devhelp/log/?h=wip/tintou/gtk-doc because I prefer branches over patches :)
I wonder why the Makefile.am weren't showing here
Anyway I've applied your suggestions.

The documentation annotations will come in another commit.
Comment 9 Sébastien Wilmet 2016-10-28 04:23:31 UTC
Thanks for the commit. Some comments:

At the end of a commit message, it's better to add the URL to the bug in bugzilla, so we can do the link (and vice-versa, add a comment with the URL to the commit once pushed).

See INST_H_FILES at src/Makefile.am, it's the list of public headers. IGNORE_HFILES of docs/reference/Makefile.am should contain the list of private headers.

Other Makefile.am files in Devhelp have a space around "=" for variable assignments. So it's better to do the same in docs/reference/Makefile.am.

In the docs of DH_ERROR, you should not talk about DhParser because it's a private class. Also, use correct English typography: end sentences with a dot. Another small detail, the comment ends with "**/". Try to be consistent and always end the gtk-doc comments with "*/" (that's what the gtk-doc manual explains here https://developer.gnome.org/gtk-doc-manual/unstable/documenting_syntax.html.en ).

Why did you do some changes like this:

> -        GtkDialog parent;
> +        GtkDialog parent_instance;

is it required for gtk-doc (for `make check`, maybe) ? Anyway, it's better to create a separate commit for that. There should be no code changes when adding gtk-doc comments.

In dh-window.h, why did you change:

> -        /* Signals */
> +        /*< public >*/

I don't think "public" is needed, it's the default, no? And it can be useful to know that the vfuncs are for the signals.
Comment 10 Corentin Noël 2016-10-29 16:09:52 UTC
Okay I modified what your requested, the parent_instance was required at some point but reverting it it seems that it no longer triggers any issue. I blacklisted the headers that are not part of the API.
I reverted all unnecessary changes but kept the documentation even on unparsed symbols (it's better if it's documented).
Comment 11 Sébastien Wilmet 2016-10-29 16:48:43 UTC
More comments:

The commit message should be:
> Enable GTK-Doc on the project
>
> https://bugzilla.gnome.org/show_bug.cgi?id=770785

The second line of a git commit message must always be empty. And the full URL is nicer than just a bug number. To have a longer explanation on how to format git commit messages, see for example:
https://git.gnome.org/browse/gtk+/tree/README.commits

In docs/reference/Makefile.am, it's better if IGNORE_HFILES contains one file per line, like in src/Makefile.am.

In docs/reference/Makefile.am, you should comment out the last lines because `make check` fails. Making `make check` to succeed with gtk-doc is a lot of work, and IMHO not very useful, I prefer checking that there are zero warnings when building the docs (`cd docs; make clean; make`). That's what we do in GtkSourceView, at least, and it works well.

Apart from those small details, I think it's a good first step.
Comment 12 Corentin Noël 2016-10-29 17:00:32 UTC
Okay, I fixed the small details.
The pro for bug #XXX is that there was a direct link with CGit and that's not the case with the complete URL.
Comment 13 Sébastien Wilmet 2016-10-29 18:19:38 UTC
Thanks, everything looks good now (the first commit).

(In reply to Corentin Noël from comment #12)
> The pro for bug #XXX is that there was a direct link with CGit and that's
> not the case with the complete URL.

Ah, ok. When seeing the git log in gnome-terminal, it's easier to click on the full URL.
Comment 14 Sébastien Wilmet 2016-10-29 18:31:51 UTC
Ok I've asked to Frédéric Péters on IRC, and he's Ok to push that first commit on master.

Pushed as commit a0ff43332ab81f942b98109605a35023a7e40d7c.
Comment 15 Corentin Noël 2016-10-29 23:17:34 UTC
Here is the second branch adding documentation
https://git.gnome.org/browse/devhelp/log/?h=wip/tintou/more-annotations
Comment 16 Sébastien Wilmet 2016-10-30 10:16:03 UTC
I didn't read in details every comments, the documentation can be improved over time, adding more information or improving a little the sentences.

In dh_app_peek_book_manager() (and at other similar places):
> Get the #DhBookManager associated with this.

A better sentence:
Get the associated #DhBookManager.

> Deprecated: 3.17.3: Not used anywhere

When referring to version numbers, please always use a stable minor version, here 3.18, without specifying the micro/patch version. 3.17.3 was a development/alpha version.

Nitpick, I see that there are still gtk-doc comments ending with "**/". Please end gtk-doc comments with "*/", for consistency.

Do not be afraid of creating more commits when you need to change the code. For example for the changes in dh-app.h and other headers. If for a reason or another, a commit needs to be reverted, it's better that a commit does only one thing (but does it well).

From the top of my head, I don't know if the transfer annotation is correct for the return value of dh_assistant_new(). I know that there is also the (transfer floating) annotation. Using (transfer floating) is more explicit, so it's better to use it, even if it is equivalent to another transfer annotation. (There are maybe other places where (transfer floating) can be used).

For _dh_window_display_uri and other private functions, instead of starting the comment with "/**", you can start with "/*< private >". Like it is done in gtk+/gsk/, it's a good practice, to easily distinguish public symbols from private ones (that are also documented).
Comment 17 Corentin Noël 2016-10-30 10:44:03 UTC
I updated my branch with your suggestions.
Regarding the commits, I'll take care to split even more my changes next time.
Regarding the transfer parameter, I'm sure the right transfer is full because by creating the object, the user has one reference of the object.
https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations#GObject-Introspection_annotations
Comment 18 Sébastien Wilmet 2016-10-30 11:39:30 UTC
floating: alias for none, can be used for floating objects.

DhAssistant is a GtkApplicationWindow, which is an indirect subclass of GtkWidget and thus also an indirect subclass of GInitiallyUnowned. dh_assistant_new() returns a floating reference.
Comment 19 Corentin Noël 2016-10-30 11:50:14 UTC
Oh yeah you're right, I forgot about it. It seems like even Gtk is not annotating creation methods so I removed the annotations.
Comment 20 Sébastien Wilmet 2016-10-30 13:16:14 UTC
Please create a separate commit to modify the parameter names in the headers.

Some help with git, if you need it:

To re-do the commits:
$ git reset HEAD^

$ git add <header>
or
$ git add -p <header>

etc.
Comment 21 Corentin Noël 2016-10-30 19:56:43 UTC
Okay, it's done
Comment 22 Sébastien Wilmet 2016-10-30 20:14:42 UTC
Thanks. Pushed.

commit 6e745075da7a5713646fa9f02831c0775fbac500
commit 720c0779337d80d9ee86d98c726aa8942c444153
Comment 23 Corentin Noël 2016-10-30 21:13:05 UTC
Here is the new branch enabling GObject Introspection on the project
https://git.gnome.org/browse/devhelp/log/?h=wip%2Ftintou%2Fgobject-introspection
Comment 24 Sébastien Wilmet 2016-10-31 09:20:06 UTC
No need to copy introspection.m4, we can assume that gobject-introspection is installed. For Vala it'll be different, we cannot assume that Vala is installed so for generating the vapi the m4 file will need to be copied. GTK+ already depends on gobject-introspection, gtk-doc etc, so we can assume it's installed.

Instead of DISTCHECK_CONFIGURE_FLAGS, AM_DISTCHECK_CONFIGURE_FLAGS should be used.

Please fix the problem in src/dh-settings.h in a previous commit. I don't want to say that for each review… Please learn from previous reviews.

In src/Makefile.am, it is not great to duplicate the list of source files in introspection_sources. It means that when creating a new file, it must be added at two different places. Look at gspell or GtkSourceView to see how it is done, there are variables that list the different kind of source files. A previous commit could create those variables (devhelp_public_headers, devhelp_public_c_files, etc). Note that in Devhelp there is both a library and an application. dh-main.c is used only for the application, so it can be kept under the devhelp_SOURCES variable.
Comment 25 Corentin Noël 2016-10-31 10:54:47 UTC
I updated my branch, and added some other niceties in different commits.
Comment 26 Sébastien Wilmet 2016-10-31 12:50:44 UTC
In src/Makefile.am, please align the backslashes on the same column (the column can be different for each variable, but keep some space for a bit longer items). The indentation in the build system is with tabs of length 8 chars (the normal length for tabs).

In the second commit, for the introspection_sources variable, instead of ${...}, use the $(...) syntax when referring to variables.

Other than that, looks good.
Comment 27 Corentin Noël 2016-10-31 16:28:36 UTC
Branch updated
Comment 28 Sébastien Wilmet 2016-10-31 16:54:04 UTC
Thanks, pushed on master.

commit dcac2cce9fcde473e68948370b478ae0c29960d7
commit 8061565a3702333944a9915a691d98e4a2c459ba
commit a7f4d4ed544c9649b69cb1229cd7f1c7427e1909
commit 4d26d8ba97a95e26cfd5b5899ac62bbbc14ea4e9

When generating the gir file, there are some warnings:

dh-language.c:49: Warning: Devhelp: dh_language_new: return value: Invalid non-constant return of bare structure or union; register as boxed type or (skip)
dh-language.c:49: Warning: Devhelp: dh_language_new: return value: Invalid non-constant return of bare structure or union; register as boxed type or (skip)
<unknown>:: Warning: Devhelp: (Signal)open-link: argument flags: Unresolved type: 'DhOpenLinkFlags'
Comment 29 Sébastien Wilmet 2016-10-31 16:57:30 UTC
And you forgot the commit for the fix in src/dh-settings.h. It'll be for the next patchset.
Comment 30 Corentin Noël 2016-10-31 21:20:31 UTC
Here is the last one before enabling conditional vapi compilation
https://git.gnome.org/browse/devhelp/?h=wip%2Ftintou%2Fmore-gobject
It fixes the warnings with DhLanguage
Comment 31 Sébastien Wilmet 2016-11-01 09:11:24 UTC
I've pushed the first commit, but I've modified the commit message: instead of "Removed [...]", replace by "Remove [...]". A commit message should describe the action, not the action already done. And also add the URL to the bug, don't forget it.

commit 47303ea23c686ba2976984573fd530623b30748e

For the second commit, "Register DhLanguage as Boxed", I think it makes more sense to make DhLanguage a GObject class instead.

(I've created the wip/tintou/more-gobject-2 branch with some fixes for the second commit, but if we go the GObject route the commit will anyway need to be redone).
Comment 32 Sébastien Wilmet 2016-11-01 09:29:10 UTC
(In reply to Sébastien Wilmet from comment #31)
> (I've created the wip/tintou/more-gobject-2 branch with some fixes for the
> second commit

I want to mention one important thing that was wrong in the original commit, in dh_language_copy():

> g_return_val_if_fail (language != NULL, -1);

-1 is not a valid return value (the return type is "DhLanguage *"), NULL must be returned instead.

The other fixes were: align parameters on the opening parenthesis, space out more the code, for better readability.
Comment 33 Sébastien Wilmet 2016-11-01 09:50:07 UTC
This bug starts to become hard to follow, let's close it because what the title says is now done.

Please open two new bugs: GObjectify DhLanguage, and generate the vapi.
Comment 34 Christian Hergert 2016-12-01 20:50:24 UTC
This broke the flatpak builds:

dh-language.c:49: Warning: Devhelp: dh_language_new: return value: Invalid non-constant return of bare structure or union; register as boxed type or (skip)
dh-language.c:49: Warning: Devhelp: dh_language_new: return value: Invalid non-constant return of bare structure or union; register as boxed type or (skip)
<unknown>:: Warning: Devhelp: (Signal)open-link: argument flags: Unresolved type: 'DhOpenLinkFlags'
<unknown>:: Fatal: Devhelp: warnings configured as fatal
<unknown>:: Fatal: Devhelp: warnings configured as fatal

/usr/share/gobject-introspection-1.0/Makefile.introspection:155: recipe for target 'Devhelp-3.0.gir' failed
make[3]: *** [Devhelp-3.0.gir] Error 1


http://sdkbuilder1.gnome.org//logs/build-2016-12-01-090313/build-gnome-apps-nightly-master-org.gnome.Devhelp-x86_64.txt
Comment 35 Sébastien Wilmet 2016-12-01 21:00:45 UTC
I don't know why warnings are configured as fatal, but the warnings are being addressed in bug #775261.
Comment 36 Christian Hergert 2016-12-01 21:20:44 UTC
I would assume a debug version check on the minor number as part of configure adding -Werror to cflags (--enable-Werror/--disable-Werror).