GNOME Bugzilla – Bug 770785
Add GTK-Doc API reference and GObject Introspection support
Last modified: 2016-12-01 21:20:44 UTC
There is currently no Gir support for libdevhelp, it would be really cool to have a native GObject Introspection support
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)
Devhelp also needs to be documented with GTK-Doc, to add the GI annotations.
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.
Created attachment 338057 [details] [review] Enable GObject introspection
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.
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 :)
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.
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.
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.
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).
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.
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.
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.
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.
Here is the second branch adding documentation https://git.gnome.org/browse/devhelp/log/?h=wip/tintou/more-annotations
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).
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
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.
Oh yeah you're right, I forgot about it. It seems like even Gtk is not annotating creation methods so I removed the annotations.
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.
Okay, it's done
Thanks. Pushed. commit 6e745075da7a5713646fa9f02831c0775fbac500 commit 720c0779337d80d9ee86d98c726aa8942c444153
Here is the new branch enabling GObject Introspection on the project https://git.gnome.org/browse/devhelp/log/?h=wip%2Ftintou%2Fgobject-introspection
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.
I updated my branch, and added some other niceties in different commits.
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.
Branch updated
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'
And you forgot the commit for the fix in src/dh-settings.h. It'll be for the next patchset.
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
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).
(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.
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.
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
I don't know why warnings are configured as fatal, but the warnings are being addressed in bug #775261.
I would assume a debug version check on the minor number as part of configure adding -Werror to cflags (--enable-Werror/--disable-Werror).