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 778137 - empty-view: port away from manual type decls
empty-view: port away from manual type decls
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: general
master
Other Linux
: Normal minor
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks: 771777
 
 
Reported: 2017-02-03 13:58 UTC by Ernestas Kulik
Modified: 2017-02-15 09:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
empty-view: Port to G_DECLARE* type declaration (5.09 KB, patch)
2017-02-06 07:30 UTC, Yash
none Details | Review
empty-view: Port to G_DECLARE* type declaration (4.75 KB, patch)
2017-02-08 18:57 UTC, Yash
none Details | Review
empty-view: Port to G_DECLARE* type declaration (4.75 KB, patch)
2017-02-10 11:55 UTC, Yash
none Details | Review
empty-view: Port to G_DECLARE* type declaration (4.75 KB, patch)
2017-02-10 11:58 UTC, Yash
none Details | Review
empty-view: Port to G_DECLARE* type declaration (4.31 KB, patch)
2017-02-10 12:01 UTC, Yash
none Details | Review
empty-view: Port to G_DECLARE* type declaration (4.31 KB, patch)
2017-02-10 12:18 UTC, Yash
none Details | Review
empty-view: Port to G_DECLARE* type declaration (4.31 KB, patch)
2017-02-10 13:45 UTC, Yash
committed Details | Review

Description Ernestas Kulik 2017-02-03 13:58:19 UTC
See bug 771777.
Comment 1 Yash 2017-02-06 07:30:37 UTC
Created attachment 345016 [details] [review]
empty-view: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces the no. of macros being used and improves readability. It makes the details private in accord with data-hiding.
Comment 2 Ernestas Kulik 2017-02-06 08:46:17 UTC
Review of attachment 345016 [details] [review]:

Thanks for the patch. However, there is some work to do.

I commented on specific portions of the code, but you also left the declaration of the _get_type () function in, which is unneeded.
You also don’t use the right formatting for the commit message. Aim for 50/72, the meaning of which can be found here: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines

::: src/nautilus-empty-view.c
@@ +40,3 @@
                                                   const char        *uri);
 
+G_DEFINE_TYPE_WITH_PRIVATE (NautilusEmptyView, nautilus_empty_view, NAUTILUS_TYPE_FILES_VIEW)

If we make the type final, you could just move the field to the type struct.

@@ +294,3 @@
 nautilus_empty_view_init (NautilusEmptyView *empty_view)
 {
+    priv = nautilus_empty_view_get_instance_private (empty_view);

You use a pointer to the private data and assign it here. That’s fine, but you reassign the pointer in every other method.

::: src/nautilus-empty-view.h
@@ +4,2 @@
    Copyright (C) 2006 Free Software Foundation, Inc.
+

Please make style changes separately.

@@ +28,2 @@
 #define NAUTILUS_TYPE_EMPTY_VIEW nautilus_empty_view_get_type()
+G_DECLARE_DERIVABLE_TYPE (NautilusEmptyView, nautilus_empty_view, NAUTILUS, EMPTY_VIEW, NautilusFilesViewClass);

I don’t see this type being subclassed anywhere, it would make sense to make it final.

@@ +34,1 @@
 } NautilusEmptyView;

This struct does not make sense in the context of derivable types.
Comment 3 Yash 2017-02-08 18:57:15 UTC
Created attachment 345260 [details] [review]
empty-view: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces the no. of macros being used and improves readability. It declares NautilusEmptyView as final as it is not subclassed anywhere.
Comment 4 Ernestas Kulik 2017-02-10 11:37:57 UTC
Review of attachment 345260 [details] [review]:

Cool, thanks. :)

Only a few minor things. Also, split your commit message, so it doesn’t extend over 72 chars.

::: src/nautilus-empty-view.c
@@ +30,3 @@
 #include <eel/eel-vfs-extensions.h>
 
+struct _NautilusEmptyView {

Move the brace to a new line.

@@ +40,3 @@
                                                   const char        *uri);
 
+G_DEFINE_TYPE (NautilusEmptyView, nautilus_empty_view, NAUTILUS_TYPE_FILES_VIEW);

No real need for a semicolon here.

::: src/nautilus-empty-view.h
@@ +30,1 @@
 GType nautilus_empty_view_get_type (void);

This is no longer needed.
Comment 5 Yash 2017-02-10 11:55:22 UTC
Created attachment 345438 [details] [review]
empty-view: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces
the no. of macros being used and improves readability.
It declares NautilusEmptyView as final as it is not subclassed anywhere.
Comment 6 Yash 2017-02-10 11:58:33 UTC
Created attachment 345439 [details] [review]
empty-view: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces
the no. of macros being used and improves readability.
It declares NautilusEmptyView as final as it is not subclassed anywhere.
Comment 7 Yash 2017-02-10 12:01:32 UTC
Created attachment 345442 [details] [review]
empty-view: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces
the no. of macros being used and improves readability.
It declares NautilusEmptyView as final as it is not subclassed anywhere.
Comment 8 Ernestas Kulik 2017-02-10 12:11:49 UTC
Review of attachment 345442 [details] [review]:

Missed one thing.

::: src/nautilus-empty-view.h
@@ +26,3 @@
 #include "nautilus-files-view.h"
 
+G_DECLARE_FINAL_TYPE (NautilusEmptyView, nautilus_empty_view, NAUTILUS, EMPTY_VIEW, NautilusFilesViewClass)

NautilusFilesView, not NautilusFilesViewClass
Comment 9 Yash 2017-02-10 12:18:35 UTC
Created attachment 345445 [details] [review]
empty-view: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces
the no. of macros being used and improves readability.
It declares NautilusEmptyView as final as it is not subclassed anywhere.
Comment 10 Ernestas Kulik 2017-02-10 12:40:52 UTC
Review of attachment 345445 [details] [review]:

Drats, missed another thing.

::: src/nautilus-empty-view.h
@@ -26,3 @@
 #include "nautilus-files-view.h"
 
-#define NAUTILUS_TYPE_EMPTY_VIEW nautilus_empty_view_get_type()

You should leave this one in.
Comment 11 Yash 2017-02-10 13:45:23 UTC
Created attachment 345457 [details] [review]
empty-view: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces
the no. of macros being used and improves readability.
It declares NautilusEmptyView as final as it is not subclassed anywhere.
Comment 12 Ernestas Kulik 2017-02-11 10:37:49 UTC
Review of attachment 345457 [details] [review]:

LGTM, thanks!
Comment 13 Ernestas Kulik 2017-02-15 09:15:33 UTC
Attachment 345457 [details] pushed as 24c0222 - empty-view: Port to G_DECLARE* type declaration