Created attachment 345554 [details] [review]
image-properties-page: port away from manual type decls

I'm a little bit confused about how to decide if a type is derivable, or a final type, since this decision is made by the Nautilus Developers. 

It's easy to figure out if another type inherits from it. But in the other cases it isn't so clear for me.

So in this patch I decided declare it as a final type, it makes more sense, at least for me.

Anyway if I'm wrong, only tell me.

> I'm a little bit confused about how to decide if a type is derivable, or a
> final type, since this decision is made by the Nautilus Developers. 

The code decides first and foremost. If the code does not tell anything conclusive, you choose and build a case for your choice. The Nautilus developers then decide whether or not to accept the change, based on your arguments.

This is rather simplified, but that is how I see it.

> It's easy to figure out if another type inherits from it. But in the other
> cases it isn't so clear for me.

That is a partial answer to your question. If you see it being derived from anywhere in the code, it definitely is derivable. If you see virtual methods, you should check if it is being derived from, then make the executive decision. I usually lean towards final types in such cases of ambiguity, as it removes confusion when trying to figure out what the class hierarchy looks like (there is also the benefit of not having to write extra code to access the private instance data).

A parallel can be drawn to C++ and/or other languages. You simply do not plaster every class with virtual methods “just in case”.

(This comment is not a review of your patch)

Review of attachment 345554 [details] [review]:

Thanks for working on this! However, Nautilus crashes with your patch.

::: src/nautilus-image-properties-page.c
@@ +46,1 @@
 {

There’s something missing here. I suggest that you compile /and/ run Nautilus with your changes.

@@ +491,3 @@
     {
+        exif_loader_unref (page->exifldr);
+        page->exifldr = NULL;

Nice catch above with the g_clear_object (). See if you can use g_clear_pointer () here.

@@ +498,3 @@
     {
+        xmp_free (page->xmp);
+        page->xmp = NULL;

Maybe here as well.

::: src/nautilus-image-properties-page.h
@@ +30,1 @@
 GType nautilus_image_properties_page_get_type (void);

This is no longer needed.

(In reply to Ernestas Kulik from comment #3)
> ::: src/nautilus-image-properties-page.c
> @@ +46,1 @@
>  {
> 
> There’s something missing here. I suggest that you compile /and/ run
> Nautilus with your changes.

What a error, I forgot to add the parent object to the structure. Thanks for don't tell me what was my mistake. I took some time to find what fails, but I improved my skills with gdb. I tested now and runs ok.

> Nice catch above with the g_clear_object (). See if you can use
> g_clear_pointer () here.

With g_clear_pointer, I have to define a new function to destroy the pointer. Although I can use 

g_clear_pointer(&(page->xmp),g_free);

but  that adds a new invoke to the stack, so I think the current solution is ok. 

However, I uploaded two patches with the different solutions.

Created attachment 345826 [details] [review]
images-properties without g_clear_pointer

images-properties without g_clear_pointer

Created attachment 345827 [details] [review]
images-properties-with-g_clear-pointer

patch with g_clear_pointer ()

Review of attachment 345826 [details] [review]:

Let’s focus on the main issue here and separate the changes. Reattach this patch without the change to g_clear_object () and we’ll go from there.

(In reply to Kevin Lopez from comment #4)
> but  that adds a new invoke to the stack, so I think the current solution is
> ok.

Sure, but g_clear_object () does the exact same.

> However, I uploaded two patches with the different solutions.

For that reason, the changes should be separated as I said earlier. Half of your work is mergeable and merging it will result in smaller patches later.

Created attachment 346416 [details] [review]
patch to image-properties

(In reply to Ernestas Kulik from comment #7)
> Review of attachment 345826 [details] [review] [review]:
> 
> Let’s focus on the main issue here and separate the changes. Reattach this
> patch without the change to g_clear_object () and we’ll go from there.

Waiting your review to upload the other patch with g_clear_*.
Regards!

Review of attachment 346416 [details] [review]:

Looks fine to me. Just remove a line and we’ll be able to move on to other things. :)

::: src/nautilus-image-properties-page.c
@@ +743,1 @@
 

Left a bit too much vertical whitespace here.

Created attachment 346558 [details] [review]
Patch to image-properties-page

Review of attachment 346558 [details] [review]:

LGTM, thanks!

this is done