Created attachment 336346 [details] [review]
bookmark-list: port to G_DECLARE* type declaration

The context of this change is the desire to make the code more easy
to read and understand for newcomers.

The problem is the boilerplate code that reduces the readability of the code.

To fix this use G_DECLARE* type.

Review of attachment 336346 [details] [review]:

LGTM, awesome work :)

Review of attachment 336346 [details] [review]:

Nice work Lavinia! 

It needs some work, here's the review. Feel free to ask any questions you might have.

About the commit message:
"The problem is the boilerplate code that reduces the readability of the code." It's actually more about being able to use g_autoptr and the new smart pointers, which are not possible with old classes.
Also, the context part lacks what we are doing right now. So I would write something like: "Currently we are using the old GObject class declarations, which have two problems." Then in the problem paragraph "One is that we cannot use smart pointers like g_autoptr. The other problem is that has more boilerplate, which makes the code less readable."

::: src/nautilus-bookmark-list.c
@@ +194,3 @@
                       G_TYPE_FROM_CLASS (object_class),
                       G_SIGNAL_RUN_LAST,
+                      0,

Why this change? We need the offset to be able to override the signal handler in subclasses (although currently we are declaring this final type, which means not sub classing allowed, we still have the public signal handler in the header). Also, the docs say:
"The closure to invoke on signal emission; may be NULL.". Although NULL == 0, we shouldn't rely on that and use NULL if necessary.

@@ +197,2 @@
                       NULL, NULL,
+                      NULL,

Why NULL to the marshal? Using NULL will let glib try to guess the marshaller, which is worse than explicitly tell what marshaller is using.

::: src/nautilus-bookmark-list.h
@@ +30,3 @@
 #include <gio/gio.h>
 
+G_BEGIN_DECLS

Although this is fine, it's not necessary for this change. In the docs of G_BEGIN_DECLS says for what it is, in case you didn't check yet.
Leave it in the patch since is nice to have it.

(In reply to Carlos Soriano from comment #3)
> Review of attachment 336346 [details] [review] [review]:
> 
> Nice work Lavinia! 
> 
> It needs some work, here's the review. Feel free to ask any questions you
> might have.
> 
> About the commit message:
> "The problem is the boilerplate code that reduces the readability of the
> code." It's actually more about being able to use g_autoptr and the new
> smart pointers, which are not possible with old classes.
> Also, the context part lacks what we are doing right now. So I would write
> something like: "Currently we are using the old GObject class declarations,
> which have two problems." Then in the problem paragraph "One is that we
> cannot use smart pointers like g_autoptr. The other problem is that has more
> boilerplate, which makes the code less readable."
> 
> ::: src/nautilus-bookmark-list.c
> @@ +194,3 @@
>                        G_TYPE_FROM_CLASS (object_class),
>                        G_SIGNAL_RUN_LAST,
> +                      0,
> 
> Why this change? We need the offset to be able to override the signal
> handler in subclasses (although currently we are declaring this final type,
> which means not sub classing allowed, we still have the public signal
> handler in the header).

With the class struct removed, the offset can be 0, as we are no longer associating any class method slot with the signal (from the docs, verbatim).

> Also, the docs say:
> "The closure to invoke on signal emission; may be NULL.". Although NULL ==
> 0, we shouldn't rely on that and use NULL if necessary.

What are you quoting, exactly? It is the offset, not the closure.

> @@ +197,2 @@
>                        NULL, NULL,
> +                      NULL,
> 
> Why NULL to the marshal? Using NULL will let glib try to guess the
> marshaller, which is worse than explicitly tell what marshaller is using.

GLib uses the generic marshaller in that case.
It is used in some places, but specified explicitly.

> ::: src/nautilus-bookmark-list.h
> @@ +30,3 @@
>  #include <gio/gio.h>
>  
> +G_BEGIN_DECLS
> 
> Although this is fine, it's not necessary for this change. In the docs of
> G_BEGIN_DECLS says for what it is, in case you didn't check yet.
> Leave it in the patch since is nice to have it.

Thanks Ernestas. Everything you said is right, sorry for the confusion. Except:

> 
> GLib uses the generic marshaller in that case.
> It is used in some places, but specified explicitly.
> 

As said I prefer a explicit marshal.

So Lavinia, just modify the marshal and the commit message and it looks good to me :)

Created attachment 336381 [details] [review]
bookmark-list: port to G_DECLARE* type declaration

Currently we are using the old GObject class declarations, which have two
problems.

One problem is that we cannot use smart pointers like g_autoptr. The other
problem is the boilerplate code generated that makes the code less readable,
so harder to understand.

To fix this use G_DECLARE* type.

Review of attachment 336381 [details] [review]:

Good job, thanks!

Pushed to master. Congrats on your first accepted contribution Lavinia!

Attachment 336381 [details] pushed as dcd4eb3 - bookmark-list: port to G_DECLARE* type declaration