GNOME Bugzilla – Bug 778137
empty-view: port away from manual type decls
Last modified: 2017-02-15 09:15:39 UTC
See bug 771777.
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.
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.
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.
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.
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.
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.
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.
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
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.
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.
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.
Review of attachment 345457 [details] [review]: LGTM, thanks!
Attachment 345457 [details] pushed as 24c0222 - empty-view: Port to G_DECLARE* type declaration