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 744060 - Update GObject tutorial documentation to use G_DECLARE_FINAL_TYPE and G_DECLARE_DERIVABLE_TYPE
Update GObject tutorial documentation to use G_DECLARE_FINAL_TYPE and G_DECLA...
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: docs
2.43.x
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2015-02-05 17:22 UTC by Philip Withnall
Modified: 2015-08-21 15:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
docs: Change tutorial encodings from ISO-8859-1 to UTF-8 (3.40 KB, patch)
2015-02-20 16:37 UTC, Philip Withnall
committed Details | Review
docs: Remove redundant header examples from GObject tutorial (3.72 KB, patch)
2015-02-20 16:37 UTC, Philip Withnall
committed Details | Review
docs: Update GObject how-to for G_DECLARE_*_TYPE macros (12.96 KB, patch)
2015-02-20 16:37 UTC, Philip Withnall
none Details | Review
docs: Add missing <function> elements to GObject how-to (8.72 KB, patch)
2015-02-20 16:37 UTC, Philip Withnall
committed Details | Review
docs: Remove commented out sections from GObject how-to (19.64 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
committed Details | Review
docs: Various wording changes in the GObject how-to (19.87 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
none Details | Review
docs: Update property handling in GObject how-to examples (4.08 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
none Details | Review
docs: Update instance private data in GObject how-to examples (3.11 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
committed Details | Review
docs: Update interfaces in GObject how-to examples (6.99 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
none Details | Review
docs: Add vfunc NULL checks to GObject how-to examples (4.30 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
none Details | Review
docs: Rename a parameter in a GObject how-to example (1.18 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
committed Details | Review
docs: Use generic marshallers in GObject how-to examples (1.41 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
committed Details | Review
docs: Update code examples in GObject tutorial (5.34 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
none Details | Review
docs: Miscellaneous formatting and wording fixes to GObject tutorial (13.84 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
none Details | Review
docs: Remove pointless copy of GObject headers from tutorial (2.15 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
committed Details | Review
docs: Mention g_clear_object() in the GObject tutorial (1.69 KB, patch)
2015-02-20 16:38 UTC, Philip Withnall
none Details | Review
docs: Link to the GObject how-to from the GType tutorial (2.98 KB, patch)
2015-02-20 16:39 UTC, Philip Withnall
committed Details | Review
docs: General cleanups and rewording in the GObject concepts docs (67.31 KB, patch)
2015-02-23 15:33 UTC, Philip Withnall
none Details | Review
docs: Port GObject concepts to use G_DECLARE_FINAL_TYPE (12.23 KB, patch)
2015-02-23 15:33 UTC, Philip Withnall
committed Details | Review
docs: Clarify costs of using the generic GObject C closure marshaller (3.14 KB, patch)
2015-02-24 08:51 UTC, Philip Withnall
committed Details | Review
docs: Update GObject how-to for G_DECLARE_*_TYPE macros (21.60 KB, patch)
2015-03-03 17:28 UTC, Philip Withnall
none Details | Review
docs: Various wording changes in the GObject how-to (19.89 KB, patch)
2015-03-03 17:28 UTC, Philip Withnall
committed Details | Review
docs: Update GObject how-to for G_DECLARE_*_TYPE macros (21.60 KB, patch)
2015-08-19 11:43 UTC, Philip Withnall
committed Details | Review
docs: Update property handling in GObject how-to examples (3.49 KB, patch)
2015-08-19 11:43 UTC, Philip Withnall
committed Details | Review
docs: Update interfaces in GObject how-to examples (7.15 KB, patch)
2015-08-19 11:44 UTC, Philip Withnall
committed Details | Review
docs: Add vfunc NULL checks to GObject how-to examples (4.30 KB, patch)
2015-08-19 11:44 UTC, Philip Withnall
committed Details | Review
docs: Update code examples in GObject tutorial (5.43 KB, patch)
2015-08-19 11:45 UTC, Philip Withnall
committed Details | Review
docs: Miscellaneous formatting and wording fixes to GObject tutorial (13.84 KB, patch)
2015-08-19 11:45 UTC, Philip Withnall
committed Details | Review
docs: Mention g_clear_object() in the GObject tutorial (1.69 KB, patch)
2015-08-19 11:45 UTC, Philip Withnall
committed Details | Review
docs: General cleanups and rewording in the GObject concepts docs (66.13 KB, patch)
2015-08-19 11:45 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2015-02-05 17:22:58 UTC
The GObject tutorials should definitely be updated to use the new G_DECLARE_[FINAL|DERIVABLE]_TYPE macros. The more bad examples of GObject usage we have around, the more people will continue to use GObject badly.

Also, while we’re at it, we should make sure they’re using G_DEFINE_TYPE_WITH_PRIVATE and friends.

https://developer.gnome.org/gobject/unstable/gtype-conventions.html
https://developer.gnome.org/gobject/unstable/howto-gobject.html
https://developer.gnome.org/gobject/unstable/howto-interface.html
Comment 1 Philip Withnall 2015-02-18 18:29:08 UTC
I’m working on this here: http://cgit.collabora.com/git/user/pwith/glib.git/log/?h=gobject-examples-docs

All very WIP at the moment.
Comment 2 Philip Withnall 2015-02-20 16:37:40 UTC
Created attachment 297430 [details] [review]
docs: Change tutorial encodings from ISO-8859-1 to UTF-8

ISO-8859-1 — that’s a blast from the past.
Comment 3 Philip Withnall 2015-02-20 16:37:45 UTC
Created attachment 297431 [details] [review]
docs: Remove redundant header examples from GObject tutorial

These are in the header boilerplate section, but are actually source
boilerplate which is covered in later sections.
Comment 4 Philip Withnall 2015-02-20 16:37:50 UTC
Created attachment 297432 [details] [review]
docs: Update GObject how-to for G_DECLARE_*_TYPE macros

Restructure the section of the how-to which covers the header and source
code boilerplate for declaring and defining GObjects to use the new
G_DECLARE_*_TYPE macros. Present both final and derivable types.

Trim various supporting paragraphs.
Comment 5 Philip Withnall 2015-02-20 16:37:55 UTC
Created attachment 297433 [details] [review]
docs: Add missing <function> elements to GObject how-to

Break the text up a little with some formatting.
Comment 6 Philip Withnall 2015-02-20 16:38:00 UTC
Created attachment 297434 [details] [review]
docs: Remove commented out sections from GObject how-to

Unused, outdated, and unsalvagable.
Comment 7 Philip Withnall 2015-02-20 16:38:05 UTC
Created attachment 297435 [details] [review]
docs: Various wording changes in the GObject how-to

 • Consistently make all titles sentence case
 • Fix various typos
 • Remove an unnecessary footnote
 • Remove first person phrasing
Comment 8 Philip Withnall 2015-02-20 16:38:10 UTC
Created attachment 297436 [details] [review]
docs: Update property handling in GObject how-to examples

Use named enums for the properties so that we can use -Wswitch or
-Wswitch-enum to detect properties missing from get_property() or
set_property().
Comment 9 Philip Withnall 2015-02-20 16:38:14 UTC
Created attachment 297437 [details] [review]
docs: Update instance private data in GObject how-to examples

Use get_instance_private().
Comment 10 Philip Withnall 2015-02-20 16:38:19 UTC
Created attachment 297438 [details] [review]
docs: Update interfaces in GObject how-to examples

Use G_DECLARE_INTERFACE and G_DEFINE_INTERFACE. Fix a couple of typos.
Add some comments to empty functions to make it obvious they’re
intentionally empty.
Comment 11 Philip Withnall 2015-02-20 16:38:24 UTC
Created attachment 297439 [details] [review]
docs: Add vfunc NULL checks to GObject how-to examples

Not setting a pure vfunc is a programmer error, so can be handled with a
g_return_if_fail() rather than needing a g_warning().
Comment 12 Philip Withnall 2015-02-20 16:38:29 UTC
Created attachment 297440 [details] [review]
docs: Rename a parameter in a GObject how-to example

Make it obvious the parameter is not related to AClass.
Comment 13 Philip Withnall 2015-02-20 16:38:35 UTC
Created attachment 297441 [details] [review]
docs: Use generic marshallers in GObject how-to examples

They’re the new vogue for handling signals.
Comment 14 Philip Withnall 2015-02-20 16:38:40 UTC
Created attachment 297442 [details] [review]
docs: Update code examples in GObject tutorial

Use G_DECLARE_FINAL_TYPE, simplify property handling (and use enums for
it), and remove some unnecessary braces.
Comment 15 Philip Withnall 2015-02-20 16:38:45 UTC
Created attachment 297443 [details] [review]
docs: Miscellaneous formatting and wording fixes to GObject tutorial

Convert a few sections to use the passive voice, and add some more
<function> elements.
Comment 16 Philip Withnall 2015-02-20 16:38:50 UTC
Created attachment 297444 [details] [review]
docs: Remove pointless copy of GObject headers from tutorial

Remove a copy of the refcounting functions from gobject.h from the
GObject tutorial. It suffices to link to the functions in the API
reference.
Comment 17 Philip Withnall 2015-02-20 16:38:55 UTC
Created attachment 297445 [details] [review]
docs: Mention g_clear_object() in the GObject tutorial

As an alternative to g_object_unref().
Comment 18 Philip Withnall 2015-02-20 16:39:00 UTC
Created attachment 297446 [details] [review]
docs: Link to the GObject how-to from the GType tutorial

So that first-time users don’t fall into the trap of reading about the
gory memory layout details of GType and GObject when all they wanted to
do was derive a class.
Comment 19 Philip Withnall 2015-02-23 15:33:47 UTC
Created attachment 297678 [details] [review]
docs: General cleanups and rewording in the GObject concepts docs

 • Remove copies of function declarations from the explanation — if
   people want those, they can follow links to the reference manual.
 • Add markup to make C code more defined.
 • Remove use of first person and irrelevant name dropping.
Comment 20 Philip Withnall 2015-02-23 15:33:52 UTC
Created attachment 297679 [details] [review]
docs: Port GObject concepts to use G_DECLARE_FINAL_TYPE

And G_DECLARE_INTERFACE.
Comment 21 Philip Withnall 2015-02-23 22:44:26 UTC
Some comments from IRC:
gregier:  the generic marshaller can also be used by passing NULL and iirc is what is normally done now
gregier:  also nice because it makes it more explicit for "nothing special", i.e. it doesn't do magic like gtk+'s draw signal 
gregier:  didn't check if there is a mention about performance 
gregier:  but I believe it checks for a non-generic version and uses it if possible 
gregier:  so generic should almost always be used
Comment 22 Emmanuele Bassi (:ebassi) 2015-02-23 22:49:15 UTC
It may very well be worth mentioning the performance impact of the generic marshaller, though if you're doing performance sensitive code I wouldn't not recommend to shove a signal in the middle of it, since they are a) blocking and b) can have an arbitrary number of side effects.
Comment 23 Philip Withnall 2015-02-24 08:51:46 UTC
Created attachment 297738 [details] [review]
docs: Clarify costs of using the generic GObject C closure marshaller

The libffi one is slower than type-specific generated ones, but is
generally better to use.
Comment 24 Allison Karlitskaya (desrt) 2015-02-26 18:58:41 UTC
Review of attachment 297430 [details] [review]:

Assuming there was no non-ascii content in these files, this is fine.
Comment 25 Allison Karlitskaya (desrt) 2015-02-26 18:59:37 UTC
Review of attachment 297431 [details] [review]:

ok
Comment 26 Allison Karlitskaya (desrt) 2015-02-26 19:19:15 UTC
Review of attachment 297432 [details] [review]:

::: docs/reference/gobject/tut_howto.xml
@@ +8,3 @@
     <para>
       This chapter tries to answer the real-life questions of users and presents
+      the most common scenario use cases in order from most likely to least

drop 'scenario'

@@ +32,3 @@
       definitions. Each of these elements is nothing but a convention which
+      is followed by almost all users of GObject, and has been refined over
+      multiple years of experience developing GObject-based code.

add something about "If you are writing a library, it is particularly important for you to adhere closely to these conventions; users of your library will assume that you have.  Even if not writing a library, it will help other people who want to work on your project." or similar

@@ +67,3 @@
     <para>
+      Final types cannot be subclassed further, and should be the default choice
+      for new types — changing a final type to be derivable will not break API,

"break API" is probably a bit too complex of a concept to drop into two words in a beginners tutorial, without further explanation.  maybe better to say something like "is always a change that will be compatible with existing users of the code, but the converse will often cause problems"

@@ +159,2 @@
     <para>
+      The convention for header includes is to add a number of

The usual convention is to include only as much as is required to get that one header file to parse without errors.

Your statement could be read that way (ie: it is not necessary to #include other headers first) but it could also be read in the way of "this one header will get you everything your entire C file will need" which is closer to how I understood it when I read it.

@@ +206,3 @@
+    <para>
+      Call the <function>G_DEFINE_TYPE</function> macro (or
+      <function>G_DEFINE_TYPE_WITH_PRIVATE</function> if your class needs

There should probably be some more explicit discussion of the fact that private data doesn't make sense with final types.

@@ +492,2 @@
       <para>
+        This is the preferred way to create polymorphic GObjects. Define the

I know about as much about GObject as anyone and I find this paragraph to be completely impenetrable.

Drop the fancy "polymorphism" thing and use language that today's programmers are more likely to be familiar with: "virtual function", "override", etc.

Maybe also switch to point form to describe the things needed for a vfunc:

 * field in class struct
 * wrapper func in .c, with associated prototype in header
 * "real" implementation
 * initialiser in class_init

It might also be worth discussing the signal-vs-vfunc duality that we seem to have in GObject, or maybe we just decide that (with how easy it is to do subclasses these days) that this is just a dead issue?
Comment 27 Allison Karlitskaya (desrt) 2015-02-26 19:30:30 UTC
Review of attachment 297433 [details] [review]:

simple enough
Comment 28 Allison Karlitskaya (desrt) 2015-02-26 19:31:05 UTC
Review of attachment 297434 [details] [review]:

who am i to argue?
Comment 29 Allison Karlitskaya (desrt) 2015-02-26 19:43:21 UTC
Review of attachment 297435 [details] [review]:

::: docs/reference/gobject/tut_howto.xml
@@ +339,2 @@
       of a type only after the properties passed to the constructors have been
       set. This is possible through the use of the <function>constructor()</function>

probably we should drop mentions of constructor() entirely

@@ +352,2 @@
   <sect1 id="howto-gobject-destruction">
+    <title>Object destruction</title>

I appreciate your hatred of Title Case.

@@ +438,3 @@
       run and before finalize runs. GObject does not consider this to be a
       program error: you must gracefully detect this and neither crash nor
+      warn the user, by having a disposed instance revert to an inert state.

The discussion about disposal leaves a lot to be desired.  Number one question, unanswered: under which circumstances (other than finalize) might disposal occur?

@@ +532,3 @@
         implementation for this class method in the object's
         <function>class_init</function> function: initialize the
         <function>klass-&gt;do_action</function> field to a pointer to the

might be worth cutting out the krap.  I always gag a bit when I see that...

@@ +673,3 @@
         provides a default implementation.</para></listitem>
         <listitem><para>Child class B re-implements method <function>foo</function>.</para></listitem>
+        <listitem><para>B’s implementation of <function>foo</function> calls its parent class A’s implementation of <function>foo</function>.</para></listitem>

'...calls ("chains up to") its parent..." will help explain the use of the term in the text below

@@ +1283,3 @@
 file = g_object_new (MAMAN_FILE_TYPE, NULL);
 
+g_signal_connect (file, "changed", (GCallback) changed_event, NULL);

why did you change away from the more familiar syntax?

@@ +1328,3 @@
     <para>
+      The C signal marshaller should always be
+      <function>g_cclosure_marshal_generic</function>, which implements generic

no.

Use a specific marshaller if at all possible.
Comment 30 Allison Karlitskaya (desrt) 2015-02-26 19:44:34 UTC
One thought: these names are just awful.. MamanIbaz?  I mean, seriously?

Maybe we can think of some names that are easier to identify with...
Comment 31 Philip Withnall 2015-03-03 17:26:35 UTC
Attachment 297430 [details] pushed as 88e011a - docs: Change tutorial encodings from ISO-8859-1 to UTF-8
Attachment 297431 [details] pushed as 3295658 - docs: Remove redundant header examples from GObject tutorial
Comment 32 Philip Withnall 2015-03-03 17:28:04 UTC
Created attachment 298456 [details] [review]
docs: Update GObject how-to for G_DECLARE_*_TYPE macros

Restructure the section of the how-to which covers the header and source
code boilerplate for declaring and defining GObjects to use the new
G_DECLARE_*_TYPE macros. Present both final and derivable types.

Trim various supporting paragraphs.

Rename ‘class functions’ to ‘virtual functions’ to use consistent,
modern terminology.
Comment 33 Philip Withnall 2015-03-03 17:28:23 UTC
Created attachment 298457 [details] [review]
docs: Various wording changes in the GObject how-to

 • Consistently make all titles sentence case
 • Fix various typos
 • Remove an unnecessary footnote
 • Remove first person phrasing
Comment 34 Philip Withnall 2015-03-03 17:35:46 UTC
Thanks for the reviews. I’ve rebased my branch locally but haven’t uploaded all the patches except the two needs-work ones, since they haven’t changed much.

(In reply to Ryan Lortie (desrt) from comment #26)
> Review of attachment 297432 [details] [review] [review]:
> 
> ::: docs/reference/gobject/tut_howto.xml
> @@ +8,3 @@
>      <para>
>        This chapter tries to answer the real-life questions of users and
> presents
> +      the most common scenario use cases in order from most likely to least
> 
> drop 'scenario'

Whoops, done.

> @@ +32,3 @@
>        definitions. Each of these elements is nothing but a convention which
> +      is followed by almost all users of GObject, and has been refined over
> +      multiple years of experience developing GObject-based code.
> 
> add something about "If you are writing a library, it is particularly
> important for you to adhere closely to these conventions; users of your
> library will assume that you have.  Even if not writing a library, it will
> help other people who want to work on your project." or similar

Good idea. Done.

> @@ +67,3 @@
>      <para>
> +      Final types cannot be subclassed further, and should be the default
> choice
> +      for new types — changing a final type to be derivable will not break
> API,
> 
> "break API" is probably a bit too complex of a concept to drop into two
> words in a beginners tutorial, without further explanation.  maybe better to
> say something like "is always a change that will be compatible with existing
> users of the code, but the converse will often cause problems"

Fair point. Done.

> @@ +159,2 @@
>      <para>
> +      The convention for header includes is to add a number of
> 
> The usual convention is to include only as much as is required to get that
> one header file to parse without errors.
> 
> Your statement could be read that way (ie: it is not necessary to #include
> other headers first) but it could also be read in the way of "this one
> header will get you everything your entire C file will need" which is closer
> to how I understood it when I read it.

I’m not entirely sure what you mean, but I have tried to clarify the paragraph a bit.

> @@ +206,3 @@
> +    <para>
> +      Call the <function>G_DEFINE_TYPE</function> macro (or
> +      <function>G_DEFINE_TYPE_WITH_PRIVATE</function> if your class needs
> 
> There should probably be some more explicit discussion of the fact that
> private data doesn't make sense with final types.

Added.

> @@ +492,2 @@
>        <para>
> +        This is the preferred way to create polymorphic GObjects. Define the
> 
> I know about as much about GObject as anyone and I find this paragraph to be
> completely impenetrable.
> 
> Drop the fancy "polymorphism" thing and use language that today's
> programmers are more likely to be familiar with: "virtual function",
> "override", etc.

Fair point. The polymorphism thing was in the original version and I didn’t change it before, but have done so now.

> Maybe also switch to point form to describe the things needed for a vfunc:
> 
>  * field in class struct
>  * wrapper func in .c, with associated prototype in header
>  * "real" implementation
>  * initialiser in class_init

Done.

> It might also be worth discussing the signal-vs-vfunc duality that we seem
> to have in GObject, or maybe we just decide that (with how easy it is to do
> subclasses these days) that this is just a dead issue?

Not sure what you mean here. How is it a dead issue? If it is to be discussed, I suspect it would be best discussed in the section on signals, and a link added from here to there.

(In reply to Ryan Lortie (desrt) from comment #29)
> Review of attachment 297435 [details] [review] [review]:

Not so many changes made to this one (as you pointed out on IRC) — I have some questions below.

> ::: docs/reference/gobject/tut_howto.xml
> @@ +339,2 @@
>        of a type only after the properties passed to the constructors have
> been
>        set. This is possible through the use of the
> <function>constructor()</function>
> 
> probably we should drop mentions of constructor() entirely

Why?

> @@ +438,3 @@
>        run and before finalize runs. GObject does not consider this to be a
>        program error: you must gracefully detect this and neither crash nor
> +      warn the user, by having a disposed instance revert to an inert state.
> 
> The discussion about disposal leaves a lot to be desired.  Number one
> question, unanswered: under which circumstances (other than finalize) might
> disposal occur?

I will handle this as a separate patch.

> @@ +532,3 @@
>          implementation for this class method in the object's
>          <function>class_init</function> function: initialize the
>          <function>klass-&gt;do_action</function> field to a pointer to the
> 
> might be worth cutting out the krap.  I always gag a bit when I see that...

It’s used for a good reason though. What would you suggest instead?

> @@ +673,3 @@
>          provides a default implementation.</para></listitem>
>          <listitem><para>Child class B re-implements method
> <function>foo</function>.</para></listitem>
> +        <listitem><para>B’s implementation of <function>foo</function>
> calls its parent class A’s implementation of
> <function>foo</function>.</para></listitem>
> 
> '...calls ("chains up to") its parent..." will help explain the use of the
> term in the text below

Good idea. Done.

> @@ +1283,3 @@
>  file = g_object_new (MAMAN_FILE_TYPE, NULL);
>  
> +g_signal_connect (file, "changed", (GCallback) changed_event, NULL);
> 
> why did you change away from the more familiar syntax?

Personal preference — I think the G_CALLBACK() macro pointlessly obscures the fact that it’s just a cast. It doesn’t (can’t) perform any checking like object cast macros, and implies there’s more magic going on than there actually is.

> @@ +1328,3 @@
>      <para>
> +      The C signal marshaller should always be
> +      <function>g_cclosure_marshal_generic</function>, which implements
> generic
> 
> no.
> 
> Use a specific marshaller if at all possible.

This somewhat contradicts the discussion on IRC mentioned in comment #21. This is an area of GLib I haven’t really touched — what would your recommendations be?
Comment 35 Philip Withnall 2015-03-03 17:39:07 UTC
(In reply to Philip Withnall from comment #34)
> > @@ +438,3 @@
> >        run and before finalize runs. GObject does not consider this to be a
> >        program error: you must gracefully detect this and neither crash nor
> > +      warn the user, by having a disposed instance revert to an inert state.
> > 
> > The discussion about disposal leaves a lot to be desired.  Number one
> > question, unanswered: under which circumstances (other than finalize) might
> > disposal occur?
> 
> I will handle this as a separate patch.

Actually, having just looked at this, it can be explained by an xref to gobject-memory-cycles. I’ll add that in the next iteration of this patch.
Comment 36 Philip Withnall 2015-03-03 17:41:50 UTC
(In reply to Ryan Lortie (desrt) from comment #30)
> One thought: these names are just awful.. MamanIbaz?  I mean, seriously?
> 
> Maybe we can think of some names that are easier to identify with...

Bother, forgot to reply to this comment.

Yes, I agree. I think this is best addressed as a single commit at the end of the patch stack. I’ll do that once everything else is committed, otherwise it will conflict with literally everything every time I rebase.
Comment 37 Philip Withnall 2015-03-28 10:35:33 UTC
Ping?
Comment 38 Philip Withnall 2015-06-10 13:52:15 UTC
Pingity ping? It would be nice to finally (after _years_ of it being outdated) get this up to date.
Comment 39 Emmanuele Bassi (:ebassi) 2015-08-19 09:38:48 UTC
Review of attachment 297436 [details] [review]:

::: docs/reference/gobject/tut_howto.xml
@@ +300,3 @@
 <informalexample><programlisting>
+typedef enum {
+  PROP_MAMAN = 1,

It'd be probably worth it to leave a comment as to why we're starting the enumeration from 1.

@@ +301,3 @@
+typedef enum {
+  PROP_MAMAN = 1,
+} MamanBarProperties;

Mmh, no. It'd definitely not idiomatic to name the enumeration.

@@ +1069,3 @@
 {
+  PROP_NAME = 1,
+} MamanBazProperties;

Nope.

@@ +1079,3 @@
   MamanBaz *baz = MAMAN_BAZ (object);
 
+  switch ((MamanBazProperties) prop_id)

Nope, not idiomatic.

I know that you're trying to handle the "oh I did not handle a property" at compile time with `-Wswitch-enum` but it makes the code look awful, and I honestly have not seen code doing this in 10+ years. Our code is also our documentation, in the sense that it's the source of our coding practices. I'm also worried that people will start filing patches to use this pattern in our code base.

We have a run-time warning, in the form of the G_OBJECT_WARN_INVALID_PROPERTY_ID macro; given that properties are run-time things, I'd rather keep pointing people at that.
Comment 40 Emmanuele Bassi (:ebassi) 2015-08-19 09:39:28 UTC
Review of attachment 297437 [details] [review]:

Okay.
Comment 41 Emmanuele Bassi (:ebassi) 2015-08-19 09:43:09 UTC
Review of attachment 297438 [details] [review]:

A couple of minor issues.

::: docs/reference/gobject/tut_howto.xml
@@ +496,3 @@
 <informalexample><programlisting>
 /* declaration in maman-bar.h. */
+G_DECLARE_DERIVABLE_TYPE (MamanBar, maman_bar, MAMAN, BAR, GObject)

You should add:

#define MAMAN_TYPE_BAR maman_bar_get_type()

above the G_DECLARE_DERIVABLE_TYPE as well.

@@ +791,3 @@
       </para></listitem>
       <listitem><para>
         The parent of the <type>MamanIbazInterface</type> is not

Left-over 'not'.

@@ +1176,3 @@
 maman_derived_baz_class_init (MamanDerivedBazClass *klass)
 {
+  /* Nothing here. */

"This space intentionally left blank" — IBM. ;-)
Comment 42 Emmanuele Bassi (:ebassi) 2015-08-19 09:45:52 UTC
Review of attachment 297439 [details] [review]:

::: docs/reference/gobject/tut_howto.xml
@@ +585,3 @@
+  klass = MAMAN_BAR_GET_CLASS (self);
+  if (klass->do_action_two != NULL)
+      klass->do_action_two (self, /* parameters */);

Coding style: additional indentation.
Comment 43 Emmanuele Bassi (:ebassi) 2015-08-19 09:46:20 UTC
Review of attachment 297440 [details] [review]:

Sure.
Comment 44 Emmanuele Bassi (:ebassi) 2015-08-19 09:46:55 UTC
Review of attachment 297441 [details] [review]:

Assuming you don't have performance requirements, yes.
Comment 45 Emmanuele Bassi (:ebassi) 2015-08-19 09:50:05 UTC
Review of attachment 297442 [details] [review]:

::: docs/reference/gobject/tut_gobject.xml
@@ +63,3 @@
 <informalexample><programlisting>
+#define MAMAN_TYPE_BAR maman_bar_get_type ()
+G_DECLARE_FINAL_TYPE (MamanBar, maman_bar, MAMAN, BAR, GObject)

You still need class and instance structures for MamanBar, even with a final type.

@@ +83,3 @@
 
+  /* Always chain up to the parent constructor */
+  obj = G_OBJECT_CLASS (maman_bar_parent_class)-&gt;constructor (gtype, n_properties, properties);

I'd actually drop the constructor override. It's rarely needed, and can be effectively replaced by constructed() overrides.

@@ +561,1 @@
+  g_object_class_install_property (object_class, PROP_NAME,

The new idiomatic way of installing properties (for faster notification) is to store the GParamSpec and use g_object_class_install_properties().
Comment 46 Emmanuele Bassi (:ebassi) 2015-08-19 09:52:50 UTC
Review of attachment 297443 [details] [review]:

::: docs/reference/gobject/tut_gobject.xml
@@ +175,2 @@
               <entry>
                 I have no real idea on how this can be used. If you have a good real-life

Please, drop this "I have no real idea" note. If the official docs of the library say "I have no idea" then how can the reader (who, we assume, is not proficient with GObject in the first place) know?
Comment 47 Emmanuele Bassi (:ebassi) 2015-08-19 09:53:14 UTC
Review of attachment 297444 [details] [review]:

Yep.
Comment 48 Emmanuele Bassi (:ebassi) 2015-08-19 09:54:46 UTC
Review of attachment 297445 [details] [review]:

::: docs/reference/gobject/tut_gobject.xml
@@ +260,3 @@
+        increase and decrease the reference count. These functions are
+        thread-safe.
+        <function><link linkend="g-object-clear">g_object_clear</link></function>

g_clear_object().
Comment 49 Emmanuele Bassi (:ebassi) 2015-08-19 09:55:39 UTC
Review of attachment 297446 [details] [review]:

Okay. At the very least, it'll force us to fix the tutorial.
Comment 50 Emmanuele Bassi (:ebassi) 2015-08-19 10:02:56 UTC
Review of attachment 297678 [details] [review]:

::: docs/reference/gobject/tut_gobject.xml
@@ +676,2 @@
         One should pay attention to the argument types and ranges when using them.
+        A known source of errors is to e.g. pass a <type>gfloat</type> instead

"""
A known source of errors is to pass a different type than what the property expects; for instance, passing an integer when the property expects a floating point value.
"""

This is far more common.

@@ +679,2 @@
         shifting all subsequent parameters by four bytes. Also forgetting the terminating
+        <literal>NULL</literal> will lead to unexpected behaviour.

"undefined", more than "unexpected".

::: docs/reference/gobject/tut_gsignal.xml
@@ +180,3 @@
       arbitrary application-specific events with any number of listeners.
       For example, in GTK+, every user event (keystroke or mouse move) is received
+      from the window manager and generates a GTK+ event in the form of a signal emission

"from the windowing system".
Comment 51 Emmanuele Bassi (:ebassi) 2015-08-19 10:03:57 UTC
Review of attachment 297679 [details] [review]:

Okay.
Comment 52 Emmanuele Bassi (:ebassi) 2015-08-19 10:04:43 UTC
Review of attachment 297738 [details] [review]:

Okay.
Comment 53 Emmanuele Bassi (:ebassi) 2015-08-19 10:08:00 UTC
Review of attachment 298456 [details] [review]:

Looks generally good.

::: docs/reference/gobject/tut_howto.xml
@@ +1816,1 @@
        supposedly more efficient.

"supposedly"? :-)

Another way to override the class handler in a subclass without the superclass using a function pointer is to use g_signal_override_class_handler(). It's probably a good thing to mention here.
Comment 54 Emmanuele Bassi (:ebassi) 2015-08-19 10:10:12 UTC
Review of attachment 298457 [details] [review]:

Okay.
Comment 55 Emmanuele Bassi (:ebassi) 2015-08-19 10:11:01 UTC
The patches look a bit out of order, and some of them could definitely be merged together.
Comment 56 Philip Withnall 2015-08-19 11:43:06 UTC
Created attachment 309558 [details] [review]
docs: Update GObject how-to for G_DECLARE_*_TYPE macros

Restructure the section of the how-to which covers the header and source
code boilerplate for declaring and defining GObjects to use the new
G_DECLARE_*_TYPE macros. Present both final and derivable types.

Trim various supporting paragraphs.

Rename ‘class functions’ to ‘virtual functions’ to use consistent,
modern terminology.
Comment 57 Philip Withnall 2015-08-19 11:43:23 UTC
Created attachment 309559 [details] [review]
docs: Update property handling in GObject how-to examples

Use named enums for the properties so that we can use -Wswitch or
-Wswitch-enum to detect properties missing from get_property() or
set_property().
Comment 58 Philip Withnall 2015-08-19 11:44:06 UTC
Created attachment 309560 [details] [review]
docs: Update interfaces in GObject how-to examples

Use G_DECLARE_INTERFACE and G_DEFINE_INTERFACE. Fix a couple of typos.
Add some comments to empty functions to make it obvious they’re
intentionally empty.
Comment 59 Philip Withnall 2015-08-19 11:44:57 UTC
Created attachment 309561 [details] [review]
docs: Add vfunc NULL checks to GObject how-to examples

Not setting a pure vfunc is a programmer error, so can be handled with a
g_return_if_fail() rather than needing a g_warning().
Comment 60 Philip Withnall 2015-08-19 11:45:09 UTC
Created attachment 309562 [details] [review]
docs: Update code examples in GObject tutorial

Use G_DECLARE_FINAL_TYPE, simplify property handling, and remove some
unnecessary braces.
Comment 61 Philip Withnall 2015-08-19 11:45:20 UTC
Created attachment 309563 [details] [review]
docs: Miscellaneous formatting and wording fixes to GObject tutorial

Convert a few sections to use the passive voice, and add some more
<function> elements.
Comment 62 Philip Withnall 2015-08-19 11:45:31 UTC
Created attachment 309564 [details] [review]
docs: Mention g_clear_object() in the GObject tutorial

As an alternative to g_object_unref().
Comment 63 Philip Withnall 2015-08-19 11:45:42 UTC
Created attachment 309565 [details] [review]
docs: General cleanups and rewording in the GObject concepts docs

 • Remove copies of function declarations from the explanation — if
   people want those, they can follow links to the reference manual.
 • Add markup to make C code more defined.
 • Remove use of first person and irrelevant name dropping.
Comment 64 Philip Withnall 2015-08-19 11:52:13 UTC
Updated patches attached which should address all the review comments unless explicitly mentioned below.

The patches are now somewhat out of order, but since the dependencies between them are not massive, I don’t think it’s worth re-uploading everything and setting all the a-c-n statuses again just to have them in order.

git branch here which _does_ have the patches in order: https://git.collabora.com/cgit/user/pwith/glib.git/log/?h=gobject-examples-docs

One thing still to do once all these patches have been reviewed: think about whether we want to retire Maman and use a less weird example namespace instead.

(In reply to Emmanuele Bassi (:ebassi) from comment #39)
> Review of attachment 297436 [details] [review] [review]:
>
> I know that you're trying to handle the "oh I did not handle a property" at
> compile time with `-Wswitch-enum` but it makes the code look awful, and I
> honestly have not seen code doing this in 10+ years. Our code is also our
> documentation, in the sense that it's the source of our coding practices.
> I'm also worried that people will start filing patches to use this pattern
> in our code base.
> 
> We have a run-time warning, in the form of the
> G_OBJECT_WARN_INVALID_PROPERTY_ID macro; given that properties are run-time
> things, I'd rather keep pointing people at that.

I disagree, but am not going to fight on this one. typedefs and casts removed.

(In reply to Emmanuele Bassi (:ebassi) from comment #44)
> Review of attachment 297441 [details] [review] [review]:
> 
> Assuming you don't have performance requirements, yes.

That’s covered in attachment #297738 [details].

(In reply to Emmanuele Bassi (:ebassi) from comment #45)
> Review of attachment 297442 [details] [review] [review]:
> 
> ::: docs/reference/gobject/tut_gobject.xml
> @@ +63,3 @@
>  <informalexample><programlisting>
> +#define MAMAN_TYPE_BAR maman_bar_get_type ()
> +G_DECLARE_FINAL_TYPE (MamanBar, maman_bar, MAMAN, BAR, GObject)
> 
> You still need class and instance structures for MamanBar, even with a final
> type.

You need the instance structure (defined just below that bit of the diff), but a class structure is defined automatically.

> @@ +83,3 @@
>  
> +  /* Always chain up to the parent constructor */
> +  obj = G_OBJECT_CLASS (maman_bar_parent_class)-&gt;constructor (gtype,
> n_properties, properties);
> 
> I'd actually drop the constructor override. It's rarely needed, and can be
> effectively replaced by constructed() overrides.

Replaced by constructed(); a much more common use case.

> @@ +561,1 @@
> +  g_object_class_install_property (object_class, PROP_NAME,
> 
> The new idiomatic way of installing properties (for faster notification) is
> to store the GParamSpec and use g_object_class_install_properties().

Eww, but changed anyway.(In reply to Emmanuele Bassi (:ebassi) from comment #46)
> Review of attachment 297443 [details] [review] [review]:
> 
> ::: docs/reference/gobject/tut_gobject.xml
> @@ +175,2 @@
>                <entry>
>                  I have no real idea on how this can be used. If you have a
> good real-life
> 
> Please, drop this "I have no real idea" note. If the official docs of the
> library say "I have no idea" then how can the reader (who, we assume, is not
> proficient with GObject in the first place) know?

Already dropped in one of the later commits.(In reply to Emmanuele Bassi (:ebassi) from comment #55)
> The patches look a bit out of order, and some of them could definitely be
> merged together.

Yeah, as per comment #34 the second round of patches were partially uploaded out of order. Some of them could be merged, but I don’t really want to waste time on that, as long as the end result is acceptable.
Comment 65 Emmanuele Bassi (:ebassi) 2015-08-21 13:37:34 UTC
Review of attachment 309558 [details] [review]:

Okay.
Comment 66 Emmanuele Bassi (:ebassi) 2015-08-21 13:38:30 UTC
Review of attachment 309559 [details] [review]:

The commit log does not match the patch any more; it should be fixed before pushing.
Comment 67 Emmanuele Bassi (:ebassi) 2015-08-21 13:39:09 UTC
Review of attachment 309560 [details] [review]:

Okay.
Comment 68 Emmanuele Bassi (:ebassi) 2015-08-21 13:39:34 UTC
Review of attachment 309561 [details] [review]:

Okay.
Comment 69 Emmanuele Bassi (:ebassi) 2015-08-21 13:40:15 UTC
Review of attachment 309562 [details] [review]:

Okay.
Comment 70 Emmanuele Bassi (:ebassi) 2015-08-21 13:40:43 UTC
Review of attachment 309563 [details] [review]:

Okay.
Comment 71 Emmanuele Bassi (:ebassi) 2015-08-21 13:41:12 UTC
Review of attachment 309564 [details] [review]:

Okay.
Comment 72 Emmanuele Bassi (:ebassi) 2015-08-21 13:42:28 UTC
Review of attachment 309565 [details] [review]:

Okay.
Comment 73 Philip Withnall 2015-08-21 14:19:30 UTC
Thanks for the reviews; all pushed. The one remaining issue is to decide about renaming Maman to something else. Anyone have any suggestions? In the absence of suggestions I’ll just close this bug, since I’m not too bothered about the naming.

57d0ec5 docs: Clarify costs of using the generic GObject C closure marshaller
e577417 docs: Port GObject concepts to use G_DECLARE_FINAL_TYPE
ab9b52e docs: General cleanups and rewording in the GObject concepts docs
a86ef24 docs: Link to the GObject how-to from the GType tutorial
cd0d605 docs: Mention g_clear_object() in the GObject tutorial
f9410b1 docs: Remove pointless copy of GObject headers from tutorial
92f6325 docs: Miscellaneous formatting and wording fixes to GObject tutorial
2aade94 docs: Update code examples in GObject tutorial
42baaa8 docs: Use generic marshallers in GObject how-to examples
01962b4 docs: Rename a parameter in a GObject how-to example
a76242b docs: Add vfunc NULL checks to GObject how-to examples
82abb80 docs: Update interfaces in GObject how-to examples
ffc2489 docs: Update instance private data in GObject how-to examples
b88ac15 docs: Update property handling in GObject how-to examples
2e4700d docs: Various wording changes in the GObject how-to
f1287a9 docs: Remove commented out sections from GObject how-to
0344e6c docs: Add missing <function> elements to GObject how-to
b6b0f5f docs: Update GObject how-to for G_DECLARE_*_TYPE macros
Comment 74 Philip Withnall 2015-08-21 15:01:32 UTC
(In reply to Philip Withnall from comment #73)
> Thanks for the reviews; all pushed. The one remaining issue is to decide
> about renaming Maman to something else. Anyone have any suggestions? In the
> absence of suggestions I’ll just close this bug, since I’m not too bothered
> about the naming.

Let’s call this FIXED. I’ve opened bug #753935 about changing the naming. Suggestions for better names welcome there.