GNOME Bugzilla – Bug 777700
nautilus_files_view:port from old-style type declarations
Last modified: 2017-02-10 13:50:09 UTC
Port to G_DECLARE Style. See https://bugzilla.gnome.org/show_bug.cgi?id=771777.
Created attachment 344158 [details] [review] nautilus-files-view: port to G_DECLARE* type declaration
Review of attachment 344158 [details] [review]: Thanks for working on this! You’re almost there. The issues are fairly minor. :) It’s not visible in the diff, but you also need to remove the call to g_type_class_add_private (). ::: src/nautilus-files-view.c @@ +342,3 @@ + NautilusFilesViewDetails *details; + +}NautilusFilesViewPrivate; Just rename the NautilusFilesViewDetails struct, this is unnecessary. @@ +9481,3 @@ + priv = nautilus_files_view_get_instance_private (view); + + priv->details = G_TYPE_INSTANCE_GET_PRIVATE (view, NAUTILUS_TYPE_FILES_VIEW, Remove this one, since we’ll be using nautilus_files_view_get_instance_private (). ::: src/nautilus-files-view.h @@ +45,1 @@ +typedef struct NautilusFilesViewDetails NautilusFilesViewDetails; This is unnecessary, since the NautilusFilesView struct declaration does not include this and the private data struct is only used in the source file, anyway. @@ +246,3 @@ void (* check_empty_states) (NautilusFilesView *view); + + gpointer padding[10]; We don’t really need this, since there is no concern for ABI compatibility. @@ +249,3 @@ }; /* GObject support */ Remove this line.
Created attachment 344208 [details] [review] nautilus-files-view: port to G_DECLARE* type declaration
Review of attachment 344208 [details] [review]: Just some style issues, but looks good to me otherwise, thanks! Since this is the last of the issues on the code side, let’s take a look at the commit message. The subject line is mostly fine, just remove the “nautilus-” part, since the namespace is already obvious. You also need to add some more text in the actual body of the message. You can find some examples in the commit log and here: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines ::: src/nautilus-files-view.c @@ +168,3 @@ static GHashTable *script_accels = NULL; +typedef struct Some spacing issues here. @@ +279,3 @@ gulong stop_signal_handler; gulong reload_signal_handler; +}NautilusFilesViewPrivate; You could take that second space from above and put it here. @@ +280,3 @@ gulong reload_signal_handler; +}NautilusFilesViewPrivate; + Looks like some useless whitespace was left here. @@ +338,3 @@ static gboolean nautilus_files_view_is_read_only (NautilusFilesView *view); +static void nautilus_files_view_iface_init (NautilusViewInterface *view); Why move this? ::: src/nautilus-files-view.h @@ +243,3 @@ * By default it shows a widget overlay on top of the view */ void (* check_empty_states) (NautilusFilesView *view); + You added a blank line here.
Created attachment 344212 [details] [review] files-view: port to G_DECLARE* type declaration This patch ports the files-view to G_DECLARE* type declaration to avoid various manual definition of macros and makes the code easier to read. Since the files-view is a derivable class we use G_DECLARE_DERIVABLE_TYPE in the header file.The structure NautilusFilesViewDetails has been renamed to NautilusFilesViewPrivate, since these are the only contents the private structure requires. Also every instance of view->details has been replaced by since the private structure is now the NautilusFilesViewDetails.
Review of attachment 344212 [details] [review]: Yup, better now, thanks! We’ll work some more on the commit message when you send another patch. :) Just so you know, my main issue with it is that the last three sentences effectively repeat what is already said in the diff (see tip #2 in the tips section in the link I provided).
Carlos, please review my review when you feel better.
Review of attachment 344212 [details] [review]: Looks good to me! I saw lot of possible style changes due to the rename of the variables, but it's fine if it stays like it is now. However there are few ones that are really obvious, maybe we can change those? Here they are: ::: src/nautilus-files-view.c @@ +4555,3 @@ + priv->subdirectory_list = g_list_prepend ( + priv->subdirectory_list, directory); this doest'n really make sense in two lines anymore @@ +4569,2 @@ + priv->subdirectory_list = g_list_remove ( + priv->subdirectory_list, directory); ditto @@ +8074,3 @@ */ + if (priv->slot == NULL || + !priv->active) ditto @@ +8153,3 @@ */ + if (priv->slot == NULL || + !priv->active) ditto
Created attachment 344863 [details] [review] files-view: port to G_DECLARE* type declaration This patch ports the files-view to G_DECLARE* type declaration to avoid various manual definition of macros and makes the code easier to read. Since the files-view is a derivable class we use G_DECLARE_DERIVABLE_TYPE in the header file.The structure NautilusFilesViewDetails has been renamed to NautilusFilesViewPrivate, since these are the only contents the private structure requires. Also every instance of view->details has been replaced by since the private structure is now the NautilusFilesViewDetails.
Created attachment 344864 [details] [review] files-view: port to G_DECLARE* type declaration This patch reduces the manual definition of macros and ports it to G_Declare type declaration, which makes the code cleaner and easier to read and understand.
Created attachment 344867 [details] [review] files-view: port to G_DECLARE* type declaration This patch reduces the manual definition of macros and ports it to G_Declare type declaration, which makes the code cleaner and easier to read and understand.
Review of attachment 344867 [details] [review]: Looks good. :) If you could also rebase the patch on master, it would be better, since it doesn’t apply cleanly. The conflict is minor, so it will be good practice. ::: src/nautilus-files-view.c @@ +9431,3 @@ nautilus_files_view_init (NautilusFilesView *view) { + Left a newline here.
Created attachment 344964 [details] [review] files-view: port to G_DECLARE* type declaration This patch reduces the manual definition of macros and ports it to G_Declare type declaration, which makes the code cleaner and easier to read and understand.
Review of attachment 344964 [details] [review]: One last nitpick and that will be it from me. :) ::: src/nautilus-files-view.c @@ +2280,2 @@ containing_directory, common_prefix); Realign the arguments.
Created attachment 345426 [details] [review] files-view: port to G_DECLARE* type declaration This patch reduces the manual definition of macros and ports it to G_Declare type declaration, which makes the code cleaner and easier to read and understand.
Created attachment 345443 [details] [review] files-view: port to G_DECLARE* type declaration This patch reduces the manual definition of macros and ports it to G_Declare type declaration, which makes the code cleaner and easier to read and understand.
Review of attachment 345443 [details] [review]: LGTM, thanks. :)
Attachment 345443 [details] pushed as 0bea857 - files-view: port to G_DECLARE* type declaration