GNOME Bugzilla – Bug 777609
column-chooser: port from old-style type declarations
Last modified: 2017-02-15 09:15:53 UTC
See bug 771777.
Created attachment 344710 [details] [review] column-chooser: Remove unnecessary macro-declaration and port to GDeclare type definiiton
Review of attachment 344710 [details] [review]: Thanks for working on this. :) I grepped the sources and it appears that no other type derives from ColumnChooser, so it would make sense to make it final. Your commit message also does not follow the format suggested here: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines I won’t highlight the issues, because that usually means that the text in the link is never read. :p ::: src/nautilus-column-chooser.h @@ +31,1 @@ +G_DECLARE_DERIVABLE_TYPE (NautilusColumnChooser, nautilus_column_chooser, NAUTILUS, COLUMN_CHOOSER, GtkBox); It doesn’t look that this type is ever derived from, so try declaring it as final first. @@ +40,3 @@ GtkWidget *nautilus_column_chooser_new (NautilusFile *file); void nautilus_column_chooser_set_settings (NautilusColumnChooser *chooser, + char **visible_columns, I can’t see what you’ve changed here, but leave it as it was.
Created attachment 344723 [details] [review] column-chooser: Convert NautilusColumnChooser to final,Port to GDeclare type definition This patch also improves code readability and reduces the no. of macros being used.
Review of attachment 344723 [details] [review]: Almost there! Your commit message is still not what we’re looking for. The subject line is way too long and the body is way too vague. I implore you to read the tips section in the link I provided. ::: src/nautilus-column-chooser.c @@ -106,3 @@ oclass->constructed = nautilus_column_chooser_constructed; - signals[CHANGED] = g_signal_new Why remove the signals? ::: src/nautilus-column-chooser.h @@ +33,3 @@ GtkWidget *nautilus_column_chooser_new (NautilusFile *file); void nautilus_column_chooser_set_settings (NautilusColumnChooser *chooser, + char **visible_columns, Oh, trailing whitespace. In any case, leave it be. If you want to perform style changes, please do so in a separate patch.
Created attachment 344736 [details] [review] column-chooser: Port to G_DECLARE* type declaration, Refactor NautilusColumnChooser This patch reduces the no. of macros being used. It removes NautilusColumnChooserClass struct and moves the details from NautilusColumnChooserDetails to _NautilusColumnChooser struct. It changes the signals[CHANGED] and signals[USE_DEFAULT] class_offset parameter to 0 accordingly.
Review of attachment 344736 [details] [review]: Picking some more nits. The subject line of the commit message should ideally fit in 50 characters. Since it’s not that much of a refactoring, “Port to G_DECLARE* type declaration” should work well enough. In the message body, refrain from retelling the diff. Try something like “Currently, the type declaration is done manually, which hurts readability blah blah. This patch does this and that, because reasons.”. Focusing on the “why” is more important. Try using more general language when explaining the “what”. ::: src/nautilus-column-chooser.c @@ +111,3 @@ G_TYPE_FROM_CLASS (chooser_class), G_SIGNAL_RUN_LAST, + 0,NULL, NULL, Add a space after the zero. @@ +119,3 @@ G_TYPE_FROM_CLASS (chooser_class), G_SIGNAL_RUN_LAST, + 0,NULL, NULL, Here as well.
Created attachment 344739 [details] [review] column-chooser: 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 removes NautilusColumnChooserClass struct and changes the NautilusColumnChooser's type to final as there is no need for data-hiding here, and it is not being subclassed anywhere.
Review of attachment 344739 [details] [review]: Okay, this will be all from me, I promise. ::: src/nautilus-column-chooser.c @@ +195,3 @@ gboolean visible; + You added an extra newline here. @@ +408,1 @@ GTK_ICON_SIZE_SMALL_TOOLBAR); Align the second argument. @@ +416,1 @@ GTK_ICON_SIZE_SMALL_TOOLBAR); Align the second argument.
Created attachment 344740 [details] [review] column-chooser: 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 removes NautilusColumnChooserClass struct and changes the NautilusColumnChooser's type to final as there is no need for data-hiding here, and it is not being subclassed anywhere.
Review of attachment 344740 [details] [review]: LGTM now, thanks!
Attachment 344740 [details] pushed as ca16997 - column-chooser: Port to G_DECLARE* type declaration